Help with copy constructor & assignment Operator

[Bopaki]

When I run this code I get these errors:

allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
allocation error:
Front: 0
Back: 8
E, F, G, H, I, J, K, W, X, , ,
Invoking Assignment operator
Invoking copy constructor

Process returned 255 (0xFF)   execution time : 6.485 s
Press any key to continue.

Here is the code:

  Queue::Queue(const Queue &source)
{
     cout<<"Invoking copy constructor" <<endl;
     this->back = source.back;
     this>capacity = source.usedCapacity;
     arr = new string[source.capacity];
     copy(source.arr, source.arr + usedCapacity, this->arr);
}

Queue &Queue::operator=(const Queue &source)
{
     if(this == &source)
         return *this;
     cout <<"Invoking Assignment operator" <<endl;
  Queue temp(source);
   swap(temp.back, back);
   swap(temp.usedCapacity, capacity);
   swap(temp.arr, arr);
   return *this;
}

[Bopaki]

Here is the code that gives the allocation error:

void Queue::memory(int currentCapacity)
{
     if(currentCapacity >= capacity)
    {
         int newCapacity = (currentCapacity  * 3) / 2 + 1;
         string  *arr2 = new string[newCapacity];
         copy(arr, arr + usedCapacity, arr2);
         delete [] arr;
         capacity = newCapacity;
    }
    else
        cout << "allocation error" << endl;
}
        

[salem c]

Where's the rest of your code?

Is "allocation error:" one of your error messages?

Have you run the code in the debugger to try to find out where these messages come from?

[Repeater]

What's the problem? What help do you need?

We're not psychic. We can't just guess what your code is meant to do or what you don't like about it.

[salem c]

> if(currentCapacity >= capacity)
So figure out why you're calling this function such that this condition evaluates to false.

[Bopaki]

Here is the rest of the code:

#ifndef QUEUE_H_INCLUDED
#define QUEUE_H_INCLUDED
#include<iostream>
#include<cstring>
#include<algorithm>
#include<string>

using namespace std;

class Queue
{
public:
    Queue();  //Default constructor
    Queue(const Queue &source);  //copy constructor
    ~Queue();  //Destructor
    Queue operator=(const Queue &source);  //Assignment operator
    void enqueue(string value);  //add item to queue
    string dequeue(); //rempve item from queue
    void showFullQueue() const;

// memory allocation
void memory(int currentCapacity);

private:
    void shift();
    string *arr;
    const static int front = 0;
    int back;
    int capacity;
    int usedCapacity;
};
#endif // QUEUE_H_INCLUDED
#include "Queue.h"
#include<iostream>
using namespace std;
Queue::Queue()
{
    back = -1;
    capacity = 1;
    usedCapacity = 0;
    arr = new string[capacity];
}

Queue::Queue(const Queue &source)
{
    cout <<"Invoking copy constructor" <<endl;
    this->back = source.back;
    this->capacity = source.usedCapacity;
    arr = new string[capacity];
    copy(source.arr, source.arr + usedCapacity, this->arr);
}

Queue::~Queue()
{
    delete [] arr;
}

Queue Queue::operator= (const Queue &source)
{
    if(this == &source)
        return *this;
    cout << "Invoking Assignment operator" << endl;
    Queue temp(source);
    swap(temp.back, back);
    swap(temp.usedCapacity, capacity);
    swap(temp.arr, arr);
    return *this;
}
void Queue::enqueue(string value)
{
    ++back;
    arr[back] = value;
    usedCapacity++;
    memory(usedCapacity);
}
string Queue::dequeue()
{
    string returnValue = arr[front];
    shift();
    usedCapacity--;
    memory(usedCapacity);
    return returnValue;
}
void Queue::showFullQueue() const
{
    cout<<"Front: " << front << endl;
    cout<<"Back: " << back << endl;
    for(int i = 0; i < capacity; i++)
    {
        cout << arr[i] << ", ";
    }
    cout<<endl;
}
void Queue::memory(int currentCapacity)
{
    if(currentCapacity >= capacity)
       {
           int newCapacity = (currentCapacity * 3) / 2 + 1;
           string *arr2 = new string[newCapacity];
           copy(arr, arr + usedCapacity,arr2);
           delete [] arr;
           arr = arr2;
           capacity = newCapacity;
       }
    else
        cout <<"allocation error:" <<endl;
}
void Queue::shift()
{
    for (int i = front; i <= back; i++)
    {
        arr[i] = arr[i + 1];
    }
    --back;
}

//Driver program
int main()
{
    Queue names, names2;
    names.enqueue("A");
    names.enqueue("B");
    names.enqueue("C");
    names.dequeue();
    names.enqueue("D");
    names.enqueue("E");
    names.enqueue("F");
    names.enqueue("G");
    names.enqueue("H");
    names.enqueue("I");
    names.enqueue("J");
    names.enqueue("K");
    names.dequeue();
    names.dequeue();
    names.dequeue();
    names.enqueue("W");
    names.enqueue("X");

    names.showFullQueue();
    names2 = names;
    names2.showFullQueue();

    names2.enqueue("Tom");
    names2.enqueue("Alice");
    names2.enqueue("Josh");
    names2.enqueue("Bridget");
    names2.showFullQueue();

    return 0;

}

[jonnin]

constructor: set capacity to 1 and used to 0.
enqueue: increment used.
memory: if(currentCapacity >= capacity)
used+1 == 1. >= trips on =.
error printed.

looks to me like it needs be > not >=. do you agree?

also for your own sanity I would have memory do this:
newCapacity = max((currentCapacity * 3) / 2 + 1, 100); //skip constant reallocation for small amounts of input. typically, when you make a new container, the first thing you do is bomb it with a bunch of data, and you want that to go quickly. 100 items is tiny, but in the first 100 or so items, you reallocated around 10 times! It starts to spread out fast after that. 100ish to 1000ish costs MUCH less than 1-100. Better than the max thing, just adjust your starting values.

[coder777]

You can easily remove line 105/6. It is not a problem when currentCapacity < capacity.

You can remove line 81 as well since the memory does not shrink.

However you have a problem when back exceeds the capacity. And When dequeue() is called back points to the wrong postition. I don't see the use of back at all. usedCapacity and capacity already do what you want.

[dhayden]

void Queue::memory(int currentCapacity)
{
     if(currentCapacity >= capacity)
    {
         int newCapacity = (currentCapacity  * 3) / 2 + 1;
         string  *arr2 = new string[newCapacity];
         copy(arr, arr + usedCapacity, arr2);
         delete [] arr;
         capacity = newCapacity;
    }
    else
        cout << "allocation error" << endl;
}

You forgot to set arr = arr2; around line 9

[dhayden]

Other comments:
- Your copy constructor leaves usedCapacity uninitialized.
- Your assignment operator doesn't set capacity.
- enqueue() should call memory(usedCapacity+1) before assigning to arr
- Shouldn't showFullQueue() show to usedCapacity? or maybe you should have a separate method that shows the used queue.

Finally, your implementation is REALLY inefficient because every time someone dequeues an item, you shift the entire queue down one position. In an implementation like this, the idea is that the front and back pointers always point to the first and last items, but the items wrap around so arr[usedCapacity-1] is followed by arr[0]. When you try to insert an item and back == front, you increase the capacity.

[ne555]

> void memory(int currentCapacity);
terrible parameter name.

also, the function being public and having a parameter makes it similar to .resize(), but you don't allocate what the parameter ask for, but a 50% more...
if you intent to only using with .push(), then make it private and with no arguments.

> Here is the rest of the code
yay, it only took you a day

[jonnin]

terrible parameter name.

and unneeded. its just a copy of the class variable he already has access to!

[highwayman]

I’m so sorry for jumping in but for some reason I can’t post a topic any where, so I have to use comments, how can I get help for posting my topics?

[JLBorges]

Go to the Forum main page. Click on the 'New Topic' button on the top right. Write the first draft of your post. You may need to subsequently edit the post to add formatting.

[Bopaki]

I have made all the changes that have been given above but the allocation error still comes up

[MikeyBoy]

Show us the current state of your code. Otherwise, you're just asking us to guess what you might have written.

[Hammad Ali]

Hello @Bopaki

This will be a long answer. I hope you understand everything I am saying and I make no mistakes and mislead you. I won't write the correct code for you but I will guide you through the issues. You need to think and solve them

Issue Number 1

At end of second memory function call the Capacity=4 and usedcapacity=2 which is enqueue("B"). And when memory function is called third time which is for enqueue("C"), the current capacity=3 and capacity=4 so if codition gives allocation error. If instead of (currentcapacity*3)/2 +1 it should be just +1.
This will resolve a few allocation errors. Now You made the dequeue Function based on same logic.

If I change (currentcapacity*3)/2 +1 to currentcapacity + 1 and remove all dequeue function calls, than there are no errors upto first showfullqueue().

[Hammad Ali]

I dont know why you are using (currentcapacity*3)/2 +1 but if you can change it to just currentcapacity + 1 than using only enqueue will give no allocation error if you dont use a deqeue.
Your dequeue is based on your old memory function's logic. So changing (currentcapacity*3)/2 +1 to currentcapacity + 1 will not resolve three allocation errors because of three dequeue function calls


So please rethink your memory function based on what I said

[Hammad Ali]

Issue Number 2

Queue names, names2 will create these two queues.After the execution of this line their Default Constructors has been Called and They are created. Their Constructor Cannot be called again. Copy constructor and "=" operator are same but they are two different things. Copy constructor can only be used for creation of object and copying value of another object in it at the same time. "=" operator can be used after creation.

So do Queue names at start and Do Queue names2=names where you are doing names2=names.

Before changing this there was exception at names2=names.

After this The code will work till first enqeue for names2. at second enqueue now you have an exception.

[Hammad Ali]

Once you have changed your memory function. try the thing people commented above. Hope it solves the problem.


And There is much much much easier way to do this. Its using vector. Instead of creating a new array everytime. Its much more efficient and effective. Reply if you are interested in learning a new way. Vector will take only ten minutes to learn and you will no longer need all the usedcapacity ,capacity, shift and memory function etc.