Simple Stack implementation in C++ I've recently started teaching myself basic C++ and decided to implement a simple stack with pointers.

#include &lt;iostream&gt;

using namespace std;

struct StackElement {
    char value;
    StackElement* next;

    StackElement(char value, StackElement* next) : value(value), next(next) {}
};

struct Stack {
    StackElement* top = NULL;

    bool isEmpty() { return top == NULL; }

    void push(char value) {
        StackElement* newElement = new StackElement(value, top);

        top = newElement;
    }

    StackElement pop() {
        StackElement* toBeDeleted = top;
        StackElement toBeReturned = *top;

        top = top-&gt;next;
        delete toBeDeleted;
        return toBeReturned;
    }
};

int main() {
    Stack* stack = new Stack();
    cout &lt;&lt; "Created a stack at " &lt;&lt; &amp;stack &lt;&lt; endl;

    int number_of_inputs;
    cout &lt;&lt; "Enter the number of elements you want to push at the stack: ";
    cin &gt;&gt; number_of_inputs;

    for (int i = 0; i &lt; number_of_inputs; i++) {
        char input;
        cin &gt;&gt; input;
        stack-&gt;push(input);
    }

    cout &lt;&lt; "- - - - - - - - - - - - - - - - " &lt;&lt; endl;
    cout &lt;&lt; "Displaying content of the stack: " &lt;&lt; endl;

    while (!stack-&gt;isEmpty()) {
        cout &lt;&lt; stack-&gt;pop().value &lt;&lt; endl;
    }

    return 0;
}


My questions are:
- what could be generally done better here?
- is the pop() method written correctly? Does it create any memory leaks? Is there a way to write it shorter?

Thank you in advance! (And forgive use of using namespace std)
Your stack implementation is terrible, and so is @hc_dev: neither handles memory correctly.

Resource Handling

It is generally frowned upon to call new and delete directly, simply because doing it correctly is hard.

Proper resource handling requires:


Thinking about moves.
Thinking about copies.
Thinking about destruction.


This used to be called the Rule of 3 in C++03 (Copy Constructor, Copy Assignment Operator and Destructor) and is called the Rule of 5 since C++11 (+Move Constructor, +Move Assignment Operator).

Your current Stack implements neither of those 5 operations correctly -- it doesn't implement them at all, and the default generated operations are buggy due to your use of a raw pointer.

The best advice for resource handling, though, is to use the Rule of Zero: just delegate it to something that works!

In your case, look into std::unique_ptr and std::make_unique!

Corrected resource management:

struct StackElement {
    char value;
    std::unique_ptr&lt;StackElement&gt; next;

    StackElement(char value, std::unique_ptr&lt;StackElement&gt; next) : 
        value(value), next(std::move(next)) {}
};

struct Stack {
    std::unique_ptr&lt;StackElement&gt; top = nullptr;

    bool isEmpty() { return top == nullptr; }

    void push(char value) {
        top = std::make_unique&lt;StackElement&gt;(value, std::move(top));
    }

    char pop() {
        assert(!isEmpty());

        char toBeReturned = top-&gt;value;

        top = std::move(top-&gt;next);

        return toBeReturned;
    }
};


This struct is no longer copiable, as std::unique_ptr is not copiable.

Limited stack depth.

The previous rewrite is good, but its destructor suffers from stack overflow (!).

That is, when the destructor is executed, you get:


Call destructor of Stack
Call destructor of Stack::top
Call destructor of StackElement (stack-&gt;top)
Call destructor of StackElement::next.
Call destructor of StackElement (stack-&gt;top-&gt;next)
...


To handle this, create a clear method, and manually write the destructor.

struct Stack {
    // ...

    Stack(Stack&amp;&amp;) = default; // automatic generation is disabled when
                              // the destructor is explicit, so explicitly
                              // ask for it.

    Stack&amp; operator=(Stack&amp;&amp;) = default; // automatic generation...

    ~Stack() { clear(); }

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


General

Once you have the memory part correct, further improvements:


Encapsulation: do not expose your privates.
Generalization: make it work for any type.


This yields:

//  No need for a class here, it's internal.
template &lt;typename T&gt;
struct StackElement {
    StackElement(T value, std::unique_ptr&lt;StackElement&gt; next):
        value(std::move(value)), next(std::move(next)) {}

    T value;
    std::unique_ptr&lt;StackElement&lt;T&gt;&gt; next;
};

template &lt;typename T&gt;
class Stack {
public:
    ~Stack() { this-&gt;clear(); }

    Stack() = default;

    Stack(Stack&amp;&amp;) = default;
    Stack&amp; operator=(Stack&amp;&amp;) = default;

    bool isEmpty() const { return this-&gt;head == nullptr; }

    T const&amp; top() const {
        assert(!this-&gt;isEmpty());

        return this-&gt;head-&gt;value;
    }

    void clear() {
        while (!isEmpty()) {
            this-&gt;pop();
        }
    }

    void push(T value) {
        //  Create empty node first, in case moving `value` throws an exception.
        auto neo = std::make_unique&lt;StackElement&lt;T&gt;&gt;(std::move(value), nullptr);

        neo-&gt;next = std::move(this-&gt;head);
        this-&gt;head = std::move(neo);
    }

    T pop() {
        assert(!isEmpty());

        //  Pop top first, in case moving `current-&gt;value` throws an exception.
        auto current = std::move(this-&gt;head);
        this-&gt;head = std::move(current-&gt;next);

        return std::move(current-&gt;value);
    }

private:
    std::unique_ptr&lt;StackElement&lt;T&gt;&gt; head;
};


Miscellaneous

There are few nits in your main:


There is no need to allocate Stack on the heap, just Stack stack; works.
Don't use std::endl, just use '\n' or "\n".


std::endl both appends \n and calls flush, the latter kills all performance benefit of internally buffering.



With that in mind, the rewritten main is:

int main() {
    Stack&lt;char&gt; stack;
    std::cout &lt;&lt; "Created a stack at " &lt;&lt; &amp;stack &lt;&lt; "\n";

    int number_of_inputs;
    std::cout &lt;&lt; "Enter the number of elements you want to push at the stack: ";
    std::cin &gt;&gt; number_of_inputs;

    for (int i = 0; i &lt; number_of_inputs; i++) {
        char input;
        std::cin &gt;&gt; input;
        stack.push(input);
    }

    std::cout &lt;&lt; "- - - - - - - - - - - - - - - - " &lt;&lt; "\n";
    std::cout &lt;&lt; "Displaying content of the stack: " &lt;&lt; "\n";

    while (!stack.isEmpty()) {
        std::cout &lt;&lt; stack.pop() &lt;&lt; "\n";
    }

    return 0;
}