Stack implementation using a singly linked list

Question

I've implemented a stack using a singly linked list and would like to know if there are any things I could improve on.

StackList.h:

#ifndef STACKLIST_H
#define STACKLIST_H

#include <iostream>

class Node
{
public:
    Node() : data(0), next(nullptr) {}
    Node(int theData, Node *newPtrToNext)
        : data(theData), next(newPtrToNext) {}
    Node* getNext() const { return next; }
    int getData() const { return data; }
    void setData(int theData) { data = theData; }
    void setNext(Node *newPtrToNext)
    {
        next = newPtrToNext;
    }
    ~Node() {}
private:
    int data;
    Node *next;
};

class StackList
{
public:
    //default constructor
    StackList();

    //copy constructor
    StackList(const StackList& otherStack);

    //overloaded assignment operator
    StackList& operator=(const StackList& otherStack);

    //destroyStack
    void destroyStack();

    //destructor
    ~StackList();

    //size
    int size() const;

    //push
    void push(int newVal);

    //pop 
    void pop();

    //top
    int top() const;

    //empty
    bool empty() const;

private:
    Node * topOfStack;
    int count;
};

#endif

StackList.cpp:

#include "StackList.h"

//default constructor
StackList::StackList()
{
    topOfStack = nullptr;
    count = 0;
}

//copy constructor
StackList::StackList(const StackList& otherStack)
{
    Node* temp = otherStack.topOfStack;
    topOfStack = new Node(temp->getData(), nullptr);
    Node * prev = topOfStack;
    temp = temp->getNext();

    while (temp != nullptr)
    {
        Node * current = new Node(temp->getData(), nullptr);
        prev->setNext(current);
        prev = prev->getNext();
        temp = temp->getNext();
    }
    count = otherStack.count;
}

//overloaded assignment operator
StackList& StackList::operator=(const StackList& otherStack)
{
    if (&otherStack != this)
    {
        destroyStack();
        Node* temp = otherStack.topOfStack;
        topOfStack = new Node(temp->getData(), nullptr);
        Node * prev = topOfStack;
        temp = temp->getNext();

        while (temp != nullptr)
        {
            Node * current = new Node(temp->getData(), nullptr);
            prev->setNext(current);
            prev = prev->getNext();
            temp = temp->getNext();
        }
        count = otherStack.count;
    }
    else
        std::cerr << "Error. Attempted assignment to itself." << std::endl;

    return *this;
}

//destroyStack
void StackList::destroyStack()
{
    Node  *temp = topOfStack;

    while (topOfStack != nullptr)
    {
        topOfStack = topOfStack->getNext();
        delete temp;
        temp = topOfStack;
    }

    count = 0;
}

//destructor
StackList::~StackList()
{
    destroyStack();
}

//size
int StackList::size() const
{
    return count;
}

//push
void StackList::push(int newVal)
{
    topOfStack = new Node(newVal, topOfStack);
    count++;
}

//top
int StackList::top() const
{
    return topOfStack->getData();
}

//pop
void StackList::pop()
{
    if (topOfStack != nullptr)
    {
        Node * temp = topOfStack;
        topOfStack = topOfStack->getNext();
        delete temp;
        count--;
    }
    else
        std::cerr << "Stack is empty. No value to pop." << std::endl;
}

//empty
bool StackList::empty() const
{
    return count == 0;
}

Answer 1

• This only works with int. Did you consider taking a template parameter so it can work with other types too?

• Don't compare to nullptr. Instead use if (node) or if (!node).

• You're very inconsistent with how you declare pointers. You have
  Node* foo, Node * foo and Node *foo. Generally in C++ this is seen as part of the type so you should prefer the first variant.

• You're using a member initialization list for Node but not for StackList. Why?

• Prefer an early out when possible to avoid the build-up of arrow code.

• Prefer prefix (++foo) over postfix (foo++) operator

• Your comments add nothing to the code and can be removed.

• Opinion based but you should not omit braces as it can lead to hard to find bugs.

• The destructor can be implemented in terms of pop().

• Node should be part of StackList as no one outside the class needs to access it. You could also simplify it by making it a struct and directly accessing the members.

• You're violating the rule of three/five/zero by having a destructor/copy assignment operator but no move constructor or operator.

• Your copy constructor does too much work. You can just iterate over the passed list and push(). Same for your assignment operator, look into the copy and swap idiom.