Basic Stack in C++ Using Array

Question

I wrote a stack class in C++ using arrays of fixed width. Could anyone review my code ? I didn't comment on any of the functions, because I thought class itself is self explanatory. Is it a wrong approach or which kind of comments can I write?

#include <stdlib.h>
#include <iostream>

template <class T>
class Stack {
  public:
    Stack(void);    //ctor
    void push(const T& item);
    T pop(void);
    void clear(void);
    T top(void) const;
    bool empty(void) const;
    bool full(void) const;
    void print(void) const;
  private:
    static const int MAX = 50;
    T list[MAX];
    int topPtr;
};

template <class T>
Stack<T>::Stack(void) {
    this->topPtr = -1;
}

template <class T>
void Stack<T>::push(const T& item) {

    if(this->topPtr == this->MAX - 1) {
        std::cerr << "Stack overflow. Can't push" << '\n';
    } else {
        this->topPtr += 1;
        this->list[topPtr] = item;
    }
}

template <class T>
T Stack<T>::pop(void) {
    if(this->topPtr < 0) {
        std::cerr << "Stack is empty. Can't pop" << '\n';
    } else {
        T r_value = this->list[this->topPtr];
        this->topPtr -= 1;
        return r_value;
    }
}

template <class T>
void Stack<T>::clear(void) {
    this->topPtr = -1;
}

template <class T>
T Stack<T>::top(void)const {
    if(this->topPtr < 0) {
        std::cerr << "Stack is empty. No top element." << '\n';
    } else {
        return this->list[topPtr];
    }
}

template <class T>
bool Stack<T>::empty(void) const {
    return (this->topPtr == -1);
}

template <class T>
bool Stack<T>::full(void) const {
    return (this->topPtr == this->MAX -1);
}

template <class T>
void Stack<T>::print(void) const {
    for (int i = this->topPtr; i >= 0; i--)
        std::cout << this->list[i] << '\n';
}

```

Answer 1

Seems like a nice implementation! Here are my thoughts,

• this is implicit. Consider removing this, i.e. use topPtr instead of this->topPtr. Similarly, void doesn't need to be used as an argument, consider removing void, i.e. top() instead of top(void).

• Consider using std::size_t for MAX. Also, consider naming MAX as MAX_SIZE since it is slightly more descriptive. If using std::size_t, make sure not to subtract one from MAX to avoid overflow. Instead, when checking for equality, add one to the other side.

• It might be nicer to use a T* instead of an int for topPtr. nullptr could be used instead of -1 initially for topPtr.

• For print consider passing a std::ostream object. This allows for decoupling and one could pass in a std::ostringstream instead of std::cout if desired.

• For top and pop, consider using assert instead if/else. An exception could also be used, but I think assert is a little nicer. To include a message, use something similar assert(!empty() && "Stack is empty. Can't pop");. See https://en.cppreference.com/w/cpp/error/assert. Maybe add a comment about the requirements of these functions.

• Consider adding documentation for any public function. Include information such as what the function returns, requires, and modifies. For example, top requires there are element in the stack.

Answer 2

• #include <stdlib.h> is unnecessary.

• C++ is not Java. All those this-> could be safely dropped.

• pop does not return anything if the stack is empty. This invokes an undefined behavior.

• Do not print from such low level utility methods. Printing tells nothing to the caller. Use success/failure return value.

• C++ containers are expected (in fact, required, see 23.2.1 for details) to destroy elements in as they are removed.