Copy Constructor glitch during inserting elements into the stack

Question

I've been given a Node and Stack class in my .h file. I have to implement the copy constructor, assignment operator and destructor and test them in a different test file. While testing the copy constructor after inserting 3 elements its displaying only one element. I don't know what's wrong; here's my .h file for your reference:

#ifndef _STACK_H
#define _STACK_H
#include <iostream>
#include <exception>
using std::ostream;
using std::cout;
using std::endl;
using std::range_error;
// Forward declarations
template <class T> class Stack;
template <class T> class Node;
template <class T> ostream& operator<<(ostream&, const Node<T>&);

// Node class for linked list
template <class T>
class Node {
    friend Stack<T>;
public:
    Node(T data = T(), Node<T>* next = nullptr) {
        _data = data;
        _next = next;
    }
    friend ostream& operator<< <T>(ostream& sout, const Node<T>& x);
private:
    T _data;
    Node* _next;
};
// Overloaded insertion operator.  Must be overloaded for the template
// class T, or this won't work!
template <class T>
ostream& operator<<(ostream& sout, const Node<T>& x) {
    sout << "Data: " << x._data;
    return sout;
}

// Stack class.  Linked-list implementation of a stack. Uses the Node
// class.
template <class T>
class Stack {
public:
    // Constructor
    Stack();

    // Copy constructor, assignment operator, and destructor
    // DO NOT IMPLEMENT HERE.  SEE BELOW.
    Stack(const Stack& rhs);
    const Stack& operator=(const Stack& rhs);
    ~Stack();

    void push(const T& data);
    const T& top() const;
    void pop();
    bool empty() const;  // Returns 'true' if stack is empty
    void dump() const;

    //Delete method used for destructor
    void nullify();

private:
    Node<T>* _head;
    Node<T>* _temp1;
    Node<T>* _temp2; //pointers
};

template <class T>
Stack<T>::Stack() {
    _head = nullptr;
}

template <class T>
Stack<T>::Stack(const Stack<T>& rhs) {
    if (rhs._head != nullptr) {
        _head = new Node<T>(rhs._head->_data);
        _temp1 = _head->_next;  //temp1 would be the next one after head
        //_temp2 = _temp2->_next; 
        while (_temp2 != nullptr) {
            _temp1 = new Node<T>(_temp2->_data);
            _temp1 = _temp1->_next;
            _temp2 = _temp2->_next; //temp2 should be the next node after temp1
        }
    }
    else
        _head = nullptr;
}

template <class T>
const Stack<T>& Stack<T>::operator=(const Stack<T>& rhs) {
    if (this != &rhs) {
        nullify();
        if (rhs._head != nullptr) {
            _head = new Node<T>(rhs._head->_data);
            _temp1 = _head->_next;  //temp1 would be the next one after head
            //_temp2 = _temp2->_next; 

            while (_temp2 != nullptr) {
                _temp1 = new Node<T>(_temp2->_data);
                _temp1 = _temp1->_next;
                _temp2 = _temp2->_next; //temp2 should be the next node after temp1
            }
        }
        else
            _head = nullptr;
    }
    return *this;
}

template <class T>
Stack<T>::~Stack() {
    nullify();
}

template <class T>
void Stack<T>::nullify() {
    while (!empty()) {
        pop();
    }
}

template <class T>
void Stack<T>::pop() {
    if (empty()) {
        throw range_error("Stack<T>::pop(): attempt to pop from an empty stack.");
    }

    Node<T>* tmpPtr = _head->_next;
    delete _head;
    _head = tmpPtr;
}

template <class T>
bool Stack<T>::empty() const {
    return _head == nullptr;
}

template <class T>
void Stack<T>::push(const T& data) {
    Node<T>* tmpPtr = new Node<T>(data);
    tmpPtr->_next = _head;
    _head = tmpPtr;
}

template <class T>
const T& Stack<T>::top() const {
    if (empty()) {
        throw range_error("Stack<T>::top(): attempt to read empty stack.");
    }

    return _head->_data;
}

template <class T>
void Stack<T>::dump() const {
    Node<T>* nodePtr = _head;
    while (nodePtr != nullptr) {
        cout << nodePtr->_data << endl;
        nodePtr = nodePtr->_next;
    }
}
#endif

While pushing 34, 67, 92 it shows only 92 for the copy constructor during output. Here's the code for which I'm testing my .h code:

#include "stack.h"
#include <iostream>
using namespace std;
using std::cout;
using std::endl;

int main()
{
    cout << "Testing default constructor\n";

    Stack<int> intStack;

    intStack.dump();
    cout << "Stack is empty initially\n\n";

    intStack.push(34);
    intStack.push(67);
    intStack.push(92);

    cout << "Testing copy constructor after inserting 92, 67 & 34: \n";
    Stack<int> test1(intStack);
    //cout << "Dumping intStack into Test1 & displaying it: \n";
    test1.dump();

    cout << "\nTesting destructor: \n";
    test1.nullify();
    test1.dump();
    cout << "Its empty\n\n";

    Stack<int> test2;

    test2.push(75);
    test2.push(56);
    test2.push(88);
    test2.push(69);

    cout << "Testing assignment operator after inserting 69, 88, 56 & 75: \n";

    Stack<int> test3;

    test3 = test2;
    test3.dump();

    cout << "\nTesting destructor: \n";
    test2.nullify();
    test2.dump();
    cout << "Its empty\n\n";

    return 0;
}

I'm still not used to C++ completely so sorry for any errors.

Answer 1

There are several things wrong with your Stack class.

First, the copy constructor doesn't initialize all the members, and neither does your default constructor. Those need to be fixed:

template <class T>
Stack<T>::Stack() : _head(nullptr), _temp1(nullptr), _temp2(nullptr) {}

template <class T>
Stack<T>::Stack(const Stack<T>& rhs) : _head(nullptr), _temp1(nullptr), _temp2(nullptr)
{
  //...
}

Once this is done, the copy constructor can be easily implemented using your other existing function, Stack::push. Your implementation is way too complicated.

template <class T>
Stack<T>::Stack(const Stack<T>& rhs) : _head(nullptr), _temp1(nullptr), _temp2(nullptr) {
    Node<T>* temp = rhs._head;
    while (temp)
    {
        push(temp->_data);
        temp = temp->_next;
    }
}

What is being done here? Simple -- all we are doing is taking the head of the passed-in Stack, and looping over the items calling Stack::push to add the data to the new Stack object. Since you have a push function already coded, you should be using it.

Second, note that we use a local temp variable. I doubt you need any of those _temp members in your class, but that is a different story.

Last, your assignment operator can easily be implemented, given you have a copy constructor and destructor for Stack:

template <class T>
const Stack<T>& Stack<T>::operator=(const Stack<T>& rhs) {
    if (this != &rhs) {
        Stack<T> temp = rhs;
        std::swap(temp._head, _head);
        std::swap(temp._temp1, _temp1);
        std::swap(temp._temp2, _temp2);
    }
    return *this;
}

That technique uses copy / swap. All that is being done is to create a temporary from the passed-in Stack object, and just swap out the current contents with the temporary's contents. Then the temporary dies off with the old contents.

Given all of this, the class should work correctly. Whether it is 100% correct with all of the other functions, that again is a different issue.

Edit:

Here is a fix for the copy constructor. Note we still use existing functions to make the copy:

  template <class T>
    Stack<T>::Stack(const Stack<T>& rhs) : _head(nullptr), _temp1(nullptr), _temp2(nullptr) {
        Node<T>* temp = rhs._head;
        Stack<T> tempStack;
        while (temp)
        {
            tempStack.push(temp->_data);
            temp = temp->_next;
        }
        while (!tempStack.empty())
        {
           push(tempStack.top()); 
           tempStack.pop();
        }
    }

This is not as efficient, but usually a stack data structure uses an underlying container such as vector where it is easy to reverse the underlying contents, and not based on a singly linked-list as you're using.