4
\$\begingroup\$

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';
}

```
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ All good comments, the issue with assert() it won't do anything when compiled in what you would call "Release" mode. \$\endgroup\$ – Harald Scheirich Aug 19 at 12:36
  • \$\begingroup\$ Thank you @Milo Hartsoe for the comments. I removed this, changed int MAX to std::size_t MAX_SIZE, changed int topPtr to T* topPtr and introduced new variable std::size_t size to keep stack size, passed std::ostreamobject to print function. I guess all lead to a better structure. \$\endgroup\$ – Erdem Tuna Aug 19 at 13:38
2
\$\begingroup\$
  • #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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestions. I changed the return type of pop to void, and removed this pointers. I want to ask can the content of a static array be removed? I made a search, but it looks like not possible. \$\endgroup\$ – Erdem Tuna Aug 20 at 7:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.