Method Names

T stackTop() andvoid clearStack should not contain the wordstack, because they already operate on a class calledStack.

Use const for methods where appropriate

As pointed out in the last code review, you should mark methods asconst that do not modify your object. You did this forgetSize(), but forgot this forstackTop(),isEmpty() andisFull().

Don't expose implementation details

There is no need thatstruct Node is accessible from outside the class (i.e.public). It is only needed within your stack, so it should beprivate. Same goes for thetop attribute. Iftop is public, I could manipulate it from the outside and could invalidate your entire stack. You don't want that!

Currently I can do this:

Stack<int> stack;stack.push(1);stack.push(2);stack.push(3);stack.top = nullptr;

Test your code!

If you run this code, you'll notice yourclear function does not behave as expected, you'll get

double free or corruption (out)

This is a serious bug!

Use the right loop

There are different loops available. While (no pun intended!) you can pretty much use any loop to do anything, some loops are better suited for a job than others.

You should use afor loop, when you iterate from one point to another. For example from beginning to end, or from 1 ton.

for(Node* tmp_ptr; top; tmp_ptr = top){    top = top->next;    delete tmp_ptr;}

This should be a while loop, because you're effectively running itwhile top is not pointing to null.

Also, this is the source of your bug, because you try to delete an uninitializedtmp_ptr.

You already wrote functions to delete an element and to check if the stack is empty. Use them! It is now very elegant to clear your stack.

while( !isEmpty() ){    pop();}

Use auto

When it is possible to deduce the type, you should useauto. This will allow you to change your code more easily without having to change all the respective types. There are other parts in your code where this is possible.

InsertStatus push(T const& t) {    //Node* node = new Node; OLD    auto* node = new Node;    node->value = t;    node->next = top;    top = node;    size++;    return OK;}

Also, consider using a constructor for yourNode, so you can donew Node(t, top);

isFull / max_size

Currently this method is useless and always returns false.

You can supply your constructor with a default value. Then you do not need to declare two constructors. Also you can use the initializer list.

Stack(int MAX_SIZE = 10000) : top(nullptr), max_size(MAX_SIZE){}

You should of course implement amax_size attribute. If you do not implement it, remove theMAX_SIZE altogether, because it's doing nothing at the moment (it's not even saved).

Finally

As discussed in the last example, use smart pointers. You said, it's a small example and not needed, but the fact that you programmeda serious bug is evidence enough for me, to use smart pointers even in this small example.

• 1
  \$\begingroup\$Thanks a lot for your great review and effort. I see It was a misunderstanding from me for the smart pointers. I will use it even with small simple codes:) (Actually, when the pointer is used in the code I shouldn't say that this is a simple code anyway :) ).\$\endgroup\$

  Omar_Hafez

  – Omar_Hafez

  2022-07-28 11:27:57 +00:00

  CommentedJul 28, 2022 at 11:27
• \$\begingroup\$The other bug caused by using raw pointers to own resources will be exposed any time you copy or assign aStack.\$\endgroup\$

  Toby Speight

  – Toby Speight

  2024-08-27 11:44:20 +00:00

  CommentedAug 27, 2024 at 11:44

The following are in addition toinfinitezero's answer:

Add some logic to yourNode class.

In C++, astruct and aclass are nearly identical concepts with the sole difference being whether members areprivate orpublic by default. So, you can add methods to astruct. A constructor for yourNode class would simplify some of yourStack methods.

class Node{    Node(T the_value, Node* the_next)    {        value = the_value;        next = the_next;    }}

Now,Stack::push(T value) can be simplified to this:

InsertStatus Stack::push(T value){    top = new Node(value, top);    ++size;    return OK;}

Additional benefits of smart pointers

Let's say that bothStack::top andNode::next were represented by astd::unique_ptr<T>. How doesStack::pop() change?

InsertStatus pop(){    if (isEmpty()) return FailedStackEmpty;    top = std::move(top->next);    size--;    return OK;}

There's a bunch of machinery under the hood ofstd::unique_ptr, but that one line does the correct actions: setstop to the second item in the stack and deletes the original top item.

Stack needs a destructor

When aStack instance goes out of scope, none of the items in theStack are deleted. The memory for those items will be uselessly used up until the program quits. This is a memory leak and needs to be fixed. Luckily, the fix is simple: create a destructor.

~Stack(){    clearStack();}

• \$\begingroup\$+1 for mentioning the destructor. Of course, this would also be a non-issue if smart-pointers were used :)\$\endgroup\$

  infinitezero

  – infinitezero

  2022-07-28 12:16:08 +00:00

  CommentedJul 28, 2022 at 12:16
• 1
  \$\begingroup\$@infinitezero I think a destructor is still needed. Letting the smart pointer handle the Stack destructor would lead to recursive calls to destroy each item in the Stack (delete top calls delete top->next which calls delete top->next->next which ... etc.). If the Stack is too deep, then the program will crash with a stack overflow.\$\endgroup\$

  Mark H

  – Mark H

  2022-07-28 12:32:33 +00:00

  CommentedJul 28, 2022 at 12:32
• \$\begingroup\$@MarkH +1 for the destructor. This is my first time noticing this important topic. Thanks a lot\$\endgroup\$

  Omar_Hafez

  – Omar_Hafez

  2022-07-28 22:12:42 +00:00

  CommentedJul 28, 2022 at 22:12

• c++
• stack