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.

• Deduplicator: Good criticism. Though I would add that auto also has many benefits.
• moooeeeep: I would further consider to introduce a NodePtr type to replace the Node* variants. And suggest to use std::unique_ptr<T> if possible.
• musicman523: I personally prefer to see if (ptr != nullptr) as opposed to if (!ptr), could you explain your reasoning for your second comment?
• yuri: @musicman523 What is your reasoning for using the verbose version? Enhanced readability due to not missing the negation? If so just use if (not ptr).
• musicman523: I guess I don't have a good reason, it's just what I've always done. To me it has been more clear because a pointer is not a boolean type and a conditional expression should result to a boolean