I’m learning C++, so I wrote a stack with the help of templates. I’m wondering if there is anything I can improve or if I did something vastly wrong.
Stack.h
#pragma once #include <ostream> template <class Type> struct Node { Node(Type data, Node<Type>* next) : data(data), next(next) {} Node* next; Type data; }; template <class Type> class Stack { public: Stack() : length(0), topNode(NULL) { } ~Stack() { while (!isEmpty()) { pop(); } } void push(Type data) { Node<Type>* newNode = new Node<Type>(data, topNode); topNode = newNode; ++length; } Type pop() { if (!isEmpty()) { Node<Type>* popped = topNode; Type poppedData = popped->data; topNode = popped->next; --length; delete popped; return poppedData; } throw new std::exception("Stack underflow!"); } bool isEmpty() { return length == 0; } void print() const { Node<Type>* tempTop = topNode; while (tempTop != NULL) { std::cout << tempTop->data << endl; tempTop = tempTop->next; } } int count() const { return length; } private: Node<Type>* topNode; int length; };
Main.cpp
#include <iostream> #include "Stack.h" using namespace std; int main() { Stack<int> myStack; // Push some values myStack.push(2); myStack.push(4); myStack.push(8); myStack.push(16); myStack.push(32); // We pop the 32 myStack.pop(); // Display count after the pop int lastPopped = myStack.pop(); cout << "Popped value: " << lastPopped << ", Count: " << myStack.count() << endl; // Print whole stack cout << endl << "Stack print: " << endl; myStack.print(); // Exit program when the 'any' key is pressed. system("PAUSE"); return 0; }
Answer
If you provide a destructor you should handle copying and assigning. In other words follow the Rule of 3 (or 5 if you care about move semantics). You can also disallow them but then there should be move constructor and assignment.
Otherwise you will get in trouble with double freeing when calling a function void foo(Stack<int> s)
.
empty()
doesn’t change the stack. So it should also be const
.
Consider adding a peek()
method that returns a reference to the top value of the stack (with const and non-const version).
T& peek()
{
return topNode->data;
}
const T& peek() const
{
return topNode->data;
}
The reason for using the reference is to avoid a unnecessary copy.
Having a print method interacting with the console is not a good idea. What if you want to show the contents in a gui instead?
Instead add iterators to be able to inspect what is on the stack without having to capture the output that print()
generates.
Attribution
Source : Link , Question Author : Caresi , Answer Author : ratchet freak