Stack Implementation

Question

I would like comments on my implementation of a stack:

template <class T>
class Stack {
 public:
 void pop(T& element);
 void push(const T& element); 
 Stack();
 private:
 int elementCount_;
 int stackTop_;
 static const int MAX_ELEMS = 10;
 T elementArray_[MAX_ELEMS]; 
};
template <class T>
Stack<T>::Stack() :
elementCount_(0),stackTop_(-1)
{
}
template <class T>
void Stack<T>::push(const T& element)
{
 if (stackTop_ == MAX_ELEMS-1){
 std::cout << "Stack is full" << "\n" ;
 return; 
 }
 elementArray_[++stackTop_] = element;
 elementCount_++;
}
template <class T>
void Stack<T>::pop(T& element)
{
 if (stackTop_ == -1){
 std::cout << "Stack is empty" << "\n";
 return;
 }
 elementCount_--;
 element = elementArray_[stackTop_--];
}

• cHao: You don't provide a way to get the top element. That makes the stack less than useful; any pushes and pops are basically busywork.
• Pradyot: I return it in pop via argument, but I can see why looking at the top without pop would be useful. Thanks
• cHao: Ahh. I missed the argument to pop, actually; i just saw that it returned void.
• Loki Astari: In C++ standard library the pop does not return a value (it just removes the top item). As a consequence there is call called top() that gives you a reference to the top value. See: stackoverflow.com/a/4892324/14065 for a discussion on the reasons. As a result I would also argue you should follow the standard C++ idiom and provide a top() method and make pop() not return a value.

Answer 1

Here are some things that may help you improve your code.

Throw errors rather than printing error messages

The user of your code may be creating a GUI with no command line available and will not appreciate having your code printing instead of indicating an error to the calling code. The two ways that C++ programs generally signal an error are either by throwing an exception (if the circumstance really is exceptional) or by returning a value that indicates an error.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Consider adding functionality

Since this is a fixed-size rather than a dynamically sized stack, it would make sense to either allow the user to specify that size (as perhaps with a default parameter for the constructor) and to query it. Also, consider adding functions such as isFull() and isEmpty().

• CodesInChaos: For a fixed size stack I'd go even further and make the capacity parameter required.

Answer 2

• As it stands, if the stack is empty, pop doesn't alter element, and doesn't let the code know that it failed. So the code using the stack thinks it popped whatever element was before the operation. Same with push if the stack is full; the code has no way of knowing that it didn't really just push an element.

  You should throw an exception if an operation fails. (Alternatively, you could return false. But C++ has exceptions, and error codes are too easy to ignore.)

• I'm not a fan of returning by reference without good reason. "Good reason" includes already returning a value (which you don't).

• stackTop_ and elementCount_ are (at least currently) redundant. You never alter one without altering the other the same way, so elementCount_ is always equal to stackTop_ + 1. I'd recommend eliminating elementCount_ and calculating it from stackTop_, or vice versa.

  Or, even better, change things so that stackTop_ is the element count -- that is, it is always the index of the next element to be pushed. That would simplify things quite a bit, actually, by making the preconditions feel less rickety.

  template <class T>
  void Stack<T>::push(const T& element) {
   // note how we're not checking against MAX_ELEMS-1 anymore.
   // since the stack top is the element count, now it's just
   // "if the stack already has MAX_ELEMS elements".
   if (stackTop_ == MAX_ELEMS) {
   throw std::runtime_error("Stack is full");
   }
   elementArray_[stackTop_++] = element;
  }
  template <class T>
  void Stack<T>::pop(T& element) {
   // same here. Turns "if the next index is -1" into "if there are 0 elements".
   if (stackTop_ == 0) {
   throw std::runtime_error("Stack is empty");
   }
   element = elementArray_[--stackTop_];
  }
  

• Just so it's said, elements aren't being disposed of once they're popped. This isn't a big deal with built-in types...but for non-trivial types, it could lead to things being held onto longer than they should be. (A Stack<std::string>, for example, would hold on to copies of each string pushed until you pop it and push something else to overwrite it, or until the stack is destroyed -- whichever comes first.)