Return to Revisions

• Do not use using namespace std.

• In the constructor and push(), it looks like you're calling new on a function. It should be node.

• Prefer nullptr to NULL and 0 for the node pointers if you're using C++11. Otherwise, you should choose either NULL or 0 for node pointers and keep it consistent.

• You don't need this-> everywhere as the code already has the current object in scope.

• It's needless to call pop() in the destructor. For one thing, pop() returns a value. You should just be using delete.

  Instead, have head point to the first element, loop through the stack (stopping when NULL is hit) and call delete on each node. With this, you won't need to delete head at the end.

• pop() and getTopElement() might be best as void. Here, a 0 is returned if the list is empty. Sure, no errors happening there. But then the calling code will take it as an element 0, never knowing whether or not the stack is empty. If you make them void, you can simply return if the stack is empty. As for retrieving an element, that's why you have getTopElement(). Once you get the value with getTopElement(), call pop() after that.

  Regarding checking for an empty stack, you could make a public empty() function for calling code. The two functions above should still check for an empty stack, but containers like this usually utilizes such a function. This could go in the header:

   bool empty() const { return num_elements == 0; }
  

• You could make your class more useful by using templates. This will allow you to populate the stack with any type instead of just ints.