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_--];
}

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().

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.)