List-based stack implementation in C++ I'm new to c++ and that is my list-based stack implementation.
It works, but i'm not sure that my code is good.  

Please help me to improve this code, any advices are welcome.

#include &lt;iostream&gt;

struct Node {
    int data;
    struct Node *next;
};

class Stack {
private:
    Node *top;
public:
    Stack()
    {
        top = nullptr;
    }

    void push(int value)
    {
        Node *n = new Node;
        n-&gt;data = value;
        n-&gt;next = top;
        top = n;
    };

    void pop()
    {
        if (top != nullptr)
        {
            Node *n = top;
            top = top-&gt;next;
            delete n;
        }
    }

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

    int getTop()
    {
        if (top != nullptr)
            return top-&gt;data;
        else return INT32_MIN; // my value is in range [-100; 100]
    }

    void display()
    {
        Node *n = top;
        while (n != nullptr)
        {
            if (n == top) std::cout &lt;&lt; "\t\t\tTOP ELEMENT" &lt;&lt; std::endl;
            else {
                std::cout &lt;&lt; "                             |" &lt;&lt; std::endl;
                std::cout &lt;&lt; "                            \\|/" &lt;&lt; std::endl;
            }
            // i need to print data, address of element and address to next element
            std::cout &lt;&lt; "---------------------------------------------------------" &lt;&lt; std::endl;
            std::cout &lt;&lt; "Data: " &lt;&lt; n-&gt;data &lt;&lt; "\t\tAddress: " &lt;&lt; n &lt;&lt;"\tNext: " &lt;&lt; n-&gt;next &lt;&lt; std::endl;
            std::cout &lt;&lt; "---------------------------------------------------------" &lt;&lt; std::endl;
            n = n-&gt;next;
        }
    }
};
First, I'll show you some essential feature your class is missing, then I'll show you a way to use modern features of C++ to automate parts of your code.

Fixing your current code

Use const to have the compiler check your logic

If a method does not modify the class, then it should be declared const so that other functions can call it when acting on an instance that is not allowed to be changed. For example,

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


Now, a function that takes a const Stack&amp; parameter can check if the Stack instance is empty. Similarly, the getTop() and display() methods should be marked const since neither modify the stack.

Clean up your resources

First, some sample code:

int main()
{
    int sizeOfStack;
    std::cin &gt;&gt; sizeOfStack; // ask user for a number
    if(sizeOfStack &lt; 10)
    {
        Stack stack;
        for(int i = 0; i &lt; sizeOfStack; ++i)
        {
            stack.push(i);
        }
        stack.display();
    }  // All data added to stack is leaked here
    else
    {
        std::cout &lt;&lt; "Too large for this example.\n";
    }

    // The memory taken up by the stack cannot be released
    // until the program exits.
}


As your code is now, when your program exits a scope (a part of code that is enclosed in curly braces like a function, loop, or if-else-block), all of the data you created with new will be leaked. This means that the data will still exist and take up memory and other resources, but there will be no way to access it to clean it up.

In C++, every class that has a data member that does not have its own destructor (usually pointers) needs a destructor to manually clean it up. This is a method named like a constructor but prefixed with a tilde: ~Stack(). For your Stack class, you would need something like

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


Write constructors to ensure data is always valid

In C++, a class and a struct are nearly identical concepts. The only difference is that the data in a class is private by default and data in a struct is public by default. Otherwise, anything you can write in a class you can write in a struct. So, you can write a constructor for your Node class.

Node(int inputData, Node* inputNext)
{
    data = inputData;
    next = inputNext;
}


A better way to write this is to use an initializer list:

Node(int inputData, Node* inputNext) : data(inputData), next(inputNext)
{
}


Similarly, for the Stack class:

Stack() : top(nullptr)
{
}


It's not so important with ints and pointers, but with more complex classes you'll write in the future, using intializer lists will avoid some unnecessary work.

Now that Nodes have a constructor, you can simplify your push() method.

push(int value)
{
    top = new Node(value, top);
}


The constructor of the Node class centralizes the logic of creating a new Node so you don't have to rewrite it everywhere.

Use methods and other named code to make your intent clear

Many places in your code check if top == nullptr. You should use the isEmpty() to tell readers of your code (including you in the future) what you actually care about.

void pop()
{
    if ( ! isEmpty())
    {
        Node *n = top;
        top = top-&gt;next;
        delete n;
    }
}


This goes for getTop() as well.

Decide how to handle copying and assignment

As it is now, when an instance of Stack is copied, both instances will refer to the same data and modifications to one instance may affect the other.

Stack s1;
s1.push(7);
Stack s2 = s1;
s2.push(21);
s1.display();
std::cout &lt;&lt; "\n\n\n";
s2.display();


This will result in the following output before crashing:

                        TOP ELEMENT
---------------------------------------------------------
Data: 7         Address: 007AF1C0       Next: 00000000
---------------------------------------------------------



                        TOP ELEMENT
---------------------------------------------------------
Data: 21                Address: 007AF230       Next: 007AF1C0
---------------------------------------------------------
                             |
                            \|/
---------------------------------------------------------
Data: 7         Address: 007AF1C0       Next: 00000000
---------------------------------------------------------


Notice that both nodes with the value 7 have the same pointer address. Now that the Stack class has a destructor, copying and assignment need to be handled to avoid deleting the same pointer address more than once. The simplest way is to disallow both.

Stack(const Stack&amp; other) = delete; // delete the copy constructor
Stack&amp; operator=(const Stack&amp; other) = delete; // delete the assignment operator


With these lines in your class declaration, the following code will not compile:

Stack s1;
Stack s2 = s1; // error: Stack cannot be copied
Stack s3(s1); // error Stack cannot be copied

Stack s4;
s4 = s1; // error: Stack cannot be assigned to.


If you want your Stack class to be copyable, then you need to explicitly copy all of the data from one to the other.

Stack(const Stack&amp; other) : top(nullptr)
{
    if( ! other.isEmpty())
    {
        top = new Node(other.top-&gt;data, nullptr);
    }
    else
    {
        return;
    }

    Node* bottom = top;
    for(Node* n = other.top-&gt;next; n != nullptr; n = n-&gt;next)
    {
        bottom-&gt;next = new Node(n-&gt;data, nullptr);
        bottom = bottom-&gt;next;
    }
}


After adding the copy constructor, you can rerun the code with s1 and s2 and see that the nodes containing the value 7 are at different addresses.

I'll leave writing the assignment operator as an exercise for you.

Tell the user when a request cannot be completed

Right now, when a user calls getTop() on an empty Stack, the user still gets an answer. Unless the user of this class sees the comment that INT32_MIN is never a valid value, there's no indication that anything went wrong. You may not have need of data values outside of [-100, 100], but what about future users (including your future self)? Why should your class not be allowed to store INT32_MIN as a valid piece of data?

It is better to tell the user that there is no data to be had. One way, if you are using C++17 (which may require a compiler flag or changing an IDE option), is to return a std::optional&lt;int&gt; (reference).

#include &lt;optional&gt;

// ...

std::optional&lt;int&gt; getTop()
{
    if(isEmpty())
    {
        return std::nullopt;
    }
    else
    {
        return top-&gt;data;
    }
}


The other option is to throw an exception in the empty case.

This way the caller of getTop() has to check if there is any data to be had.

Other notes

In the display() method, you can use a for-loop instead of a while loop to centralize all of the looping logic.

void display()
{
    for(Node* n = top; n != nullptr; n = n-&gt;next)
    {
        // output
    }
}


One more nitpick, you don't need the keyword struct in the Node::next declaration.

struct Node {
    Node(int inputData, Node* inputNext) : data(inputData), next(inputNext) {}
    int data;
    Node* next;
};


Using modern C++ features

A great way of improving this code is to use smart pointers. The std::unique_ptr (reference) tells readers, users, and the compiler that the variable or class containing the unique_ptr is responsible for cleaning up the variable when it is no longer needed.

So, you can change your Node class to

#include &lt;memory&gt;

struct Node {
    Node(int inputData, Node* inputNext) : data(inputData), next(inputNext) {}
    int data;
    std::unique_ptr&lt;Node&gt; next;
};


And the Stack class turns into

class Stack {
private:
    std::unique_ptr&lt;Node&gt; top;
// ...


Now, when a stack instance leaves the scope where it was created, the top node is automatically deleted. When top is deleted, top-&gt;next is automatically deleted, and then top-&gt;next-&gt;next, and then top-&gt;next-&gt;next-&gt;next, etc. So, you no longer need an explicit destructor (~Stack). The compiler will generate the correct destructor for you.

Now, you no longer have to call delete on any pointers. For example, the pop() method is now reduced to

void pop()
{
    if( ! isEmpty())
    {
        top = std::move(top-&gt;next);
    }
}


The std::move() call means that the contents of top-&gt;next--both the data and the pointer to the top-&gt;next-&gt;next node--will be stolen and moved into top. The previous contents of top will be deleted as this happens.

The other places where pointer manipulation changes is anywhere new is called. The push() method is now written with smart pointers as

void push(int value)
{
    top = std::make_unique&lt;Node&gt;(value, top.release());
};


The release() method returns the raw (non-smart) pointer held by the smart pointer and removes all responsibility for deleting it from that smart pointer. This happens so that the new smart pointer being added takes over responsibility for deleting the data when it is no longer needed. Assigning the new std::unique_ptr to top means that the Stack instance is now responsible for deleting the newly created Node.

Finally, if you don't want your Stack class to be copyable or assignable, you no longer need to delete those functions, since they will no longer be automatically generated. Instances of std::unique_ptr are not copyable or assignable, so any class they are a part of will lose those qualities as well. If you want to make your class copyable, you'll need the get() method of the std::unique_ptr. This returns the raw pointer like the release() method, but the std::unique_ptr retains responsibility for deleting the Node. This will allow your program to follow the linked list chain without copying std::unique_ptrs.