Copy Constructor issue in Queue Implementation using implemented LinkedList module

Question

Complete repository: https://github.com/mervynlee94/queue/tree/master

I have implemented LinkedList and tested my copy constructor without issue.

LinkedList<int> l1;
l1.insert(1,3);               // set value 3 in 1st position
l1.insert(2,7);               // set value 7 in 2nd position
l1.insert(3,9);               // set value 9 in 3rd position
LinkedList<int> l2(l1);       // copy constructor
l2.setEntry(2,5);             // set value 5 in position 2
cout<< l1.getEntry(2) <<endl; // output: 7
cout<< l2.getEntry(2) <<endl; // output: 5

My Queue implementation uses this LinkedList module as the data structure with code below.

#ifndef _LIST_QUEUE
#define _LIST_QUEUE
#include "QueueInterface.h"
#include "LinkedList.h"

template<class ItemType>
class ListQueue : public QueueInterface<ItemType> {
    private :
        LinkedList<ItemType> list; 
    public :
        ListQueue();
        ListQueue(const ListQueue& aQueue);
        ~ListQueue();
        bool isEmpty() const ;
        void enqueue( const ItemType& newEntry);
        void dequeue();
        ItemType peekFront() const;
};
#endif

template<class ItemType>
ListQueue<ItemType>::ListQueue() {

}

template<class ItemType>
ListQueue<ItemType>::ListQueue(const ListQueue& aQueue) {
    list = LinkedList<ItemType>(aQueue.list);
}

template<class ItemType>
ListQueue<ItemType>::~ListQueue() {}

template<class ItemType>
bool ListQueue<ItemType>::isEmpty() const {
    return list.isEmpty();
}

template<class ItemType>
void ListQueue<ItemType>::enqueue(const ItemType& newEntry) {
    list.insert(list.getLength()+1, newEntry);
}

template<class ItemType>
void ListQueue<ItemType>::dequeue() {
    list.remove(1);
}

template<class ItemType>
ItemType ListQueue<ItemType>::peekFront() const {
    return list.getEntry(1);
}

When I test the code out in main.cpp, I encountered segmentation error.

ListQueue<int> q1;
q1.enqueue(1);
q1.enqueue(2);
q1.enqueue(3);
ListQueue<int> q2(q1); // Set breakpoint here
q2.enqueue(5);
cout<<q2.peekFront()<<endl;
cout<<q1.peekFront()<<endl;

I set a debugging breakpoint on above and I realise one thing. The copy constructor in Queue implementation doesn't work properly.

The debugging value shown in copy constructor of aQueue.list and this.list varies.

Here is the LinkedList copy constructor implementation

template<class ItemType>
LinkedList<ItemType>::LinkedList(const LinkedList<ItemType>& aList) {
    Node<ItemType>* listPtr = aList.headPtr;
    if(listPtr == nullptr) {
        headPtr = nullptr;
    }
    else {
        headPtr = new Node<ItemType>(listPtr->getItem());
        Node<ItemType>* newPtr = headPtr;
        while(listPtr->getNext() != nullptr) {
            listPtr = listPtr->getNext();
            Node<ItemType>* newNode = new Node<ItemType>(listPtr->getItem());
            newPtr->setNext(newNode);
            newPtr = newPtr->getNext();
        }
    }
    itemCount = aList.getLength();
}

Answer 1

Note that since the copy-ctor of LinkedList is already implemented, you don't have to reimplement it - C++ will do it automatically for you.

Another important thing - copy constructor and copy assignment operator are distict.

You implemented Copy-ctor for your Queue, and in this copy-ctor you used the copy-assignment operator, and I suspect you did not implement it.

The easiest way to fix it is to simply remove the copy constructor and destructor, they are not necessary. The correct way to invoke a copy-constructor is as follows:

template<class ItemType>
ListQueue<ItemType>::ListQueue(const ListQueue& aQueue) 
    : list(aQuquq.list) {
}

I have three more points to make:

1. Rule of 3 - if you declare one of copy-constructor, copy-assignment or destructor, you almost surely have to implement them all.
2. There is a bit more complicated feature in C++ called "move semantics", in case you are familiar with the term, you should consider the Rule of 5 - the same as the previous rule, with addition of move-constructor, and move-assignment operator.
3. It seems as you are using inheritance, but I don't see the keyword override anywhere. If you are using C++11 or above, consider adding the override qualifier to overriding methods.

And, lastly, C++11 supports default ctors/dtors and assignment operators. If you write an empty destructor (as you do here), the cleaner syntax is to write

class ListQueue : public QueueInterface<ItemType> {
        ...
        ~ListQueue() = default;
        ...
};