Implementing a Stack with Templates and Smart pointers I'm trying to understand the concepts of smart pointers and templates. To get a better understanding of these, I tried to implement these two concepts on Stack. I have created a Stack which can hold objects of any type. For dynamic memory allocation, I have used smart pointers.

Please suggest how I could improve the code. Am I missing any cases while using smart pointers and templates? Since I'm using smart pointers, can I rest assure that free store (heap) memory will be deleted once the Stack variable goes out of scope?

#include "stdafx.h"
#include&lt;iostream&gt;
#include&lt;string&gt;
#include&lt;memory&gt;

using namespace std;

// Template Node
template &lt;typename T&gt;
struct Node
{
    T data;
    shared_ptr&lt;Node&gt; next;
    Node(T d, shared_ptr&lt;Node&gt; n) : data(d), next(n) {}
};

// Template Stack
// Note: T type is used to set the Node type
template &lt;typename T&gt;
class Stack {
private:
    shared_ptr&lt;Node&lt;T&gt;&gt; head;
public:
    Stack() : head(NULL) {}
    Stack(const Stack&lt;T&gt; &amp;rhs) = delete;
    Stack&amp; operator=(const Stack&lt;T&gt; &amp;rhs) = delete;
    void push(const T&amp;);
    void pop();
    T top() const;
    void print() const;
};

// Notice &lt;T&gt; after the class name, it is the syntax requirement
template &lt;typename T&gt;
void Stack&lt;T&gt;::push(const T &amp;data) {
    shared_ptr&lt;Node&lt;T&gt;&gt; temp = make_shared&lt;Node&lt;T&gt;&gt;(data,nullptr);
    if (head == nullptr) {
        head = temp;
    }
    else {
        temp-&gt;next = head;
        head = temp;
    }
}

template &lt;typename T&gt;
void Stack&lt;T&gt;::pop() {
    //shared_ptr&lt;Node&lt;T&gt;&gt; temp = head;
    head = head-&gt;next;
}

template &lt;typename T&gt;
T Stack&lt;T&gt;::top() const {
    return head-&gt;data;
}

template &lt;typename T&gt;
void Stack&lt;T&gt;::print() const {
    // shared_ptr&lt;Node&lt;T&gt;&gt; temp;
    for (auto temp = head; temp != nullptr; temp = temp-&gt;next)
        cout &lt;&lt; temp-&gt;data &lt;&lt; '\n';
}

int main()
{
    cout &lt;&lt; "String data" &lt;&lt; '\n';
    {
        Stack&lt;string&gt; strStack;
        strStack.push("Test1");
        strStack.push("Test2");
        strStack.push("Test3");
        strStack.print();
        strStack.pop();
        strStack.print();
    }
    cout &lt;&lt; "Integer data" &lt;&lt; '\n';
    {
        Stack&lt;int&gt; intStack;
        intStack.push(10);
        intStack.push(20);
        intStack.push(30);
        intStack.print();
        intStack.pop();
        intStack.print();
    }
    return 0;
}
Readability

Please put a space after the include and before the &lt;

#include&lt;iostream&gt;
#include&lt;string&gt;
#include&lt;memory&gt;


Don't do this

You can read any other C++ revue on this site.

using namespace std;


See: Why is “using namespace std” considered bad practice?

Don't add useless comments

// Template Node
template &lt;typename T&gt;
struct Node


Thanks but I can read the code I can see it is a template and a class called Node. Useless comments are worse than no comment.

// Template Stack
// Note: T type is used to set the Node type
template &lt;typename T&gt;


Again you are describing what the code does. If you write nice well formatted code it should explain itself (with good method and variable names).

// Notice &lt;T&gt; after the class name, it is the syntax requirement
template &lt;typename T&gt;
void Stack&lt;T&gt;::push(const T &amp;data) {


Don't explain the compiler rules. The compiler does not care nor do I.

Pass parameters by reference

Here you pass the parameters by value.

    Node(T d, shared_ptr&lt;Node&gt; n) : data(d), next(n) {}


This means you are copying the data to the parameters than you are making a second copy of the data into the member variables.

Try this:

    Node(T const&amp; d, shared_ptr&lt;Node&gt;&amp; n) : data(d), next(n) {}


You can even use move semantics. I'll cover that more in the push() below.

    Node(T&amp;&amp; d, shared_ptr&lt;Node&gt;&amp; n) : data(std::move(d)), next(n) {}


Push

You don't need that if block.

void Stack&lt;T&gt;::push(const T &amp;data) {
    shared_ptr&lt;Node&lt;T&gt;&gt; temp = make_shared&lt;Node&lt;T&gt;&gt;(data,nullptr);
    if (head == nullptr) {
        head = temp;
    }
    else {
        temp-&gt;next = head;
        head = temp;
    }
}


You always assign temp to head. The value assigned to temp.next is nullptr if the head is nullptr or it is head. In both cases the value of head is assigned to next.

void Stack&lt;T&gt;::push(const T&amp; data) {
    head = make_shared&lt;Node&lt;T&gt;&gt;(data, std::move(head));
}


You pass the data by reference, which is good. Make sure that node also takes the parameter by reference.

In some situations we also want to add move semantics to the class. So it can also be useful to add a push() that takes an r-value reference. This allows for a much more efficient push when T is large (like an vector).

void Stack&lt;T&gt;::push(T&amp;&amp; data) {
    head = make_shared&lt;Node&lt;T&gt;&gt;(std::move(data), std::move(head));
}


Also in the vain of push. Sometimes T is expensive to build and copy but its arguments can easily be passed. In this case it is worth allowing the user to pass the parameters to the constructor of T and then building the T object in place (rather than passing it around).

template&lt;typename... Args&gt;
void Stack&lt;T&gt;::push&lt;Args&gt;(Args...&amp;&amp; args) {
    head = make_shared&lt;Node&lt;T&gt;&gt;(std::move(args));
}

template&lt;typename... Args&gt;
Node&lt;T&gt;::Node&lt;Args...&gt;(Args...&amp;&amp; args)
    : data(args...) // Use the constructor of T
{}


Top

Here you return the result by value. This means a copy is probably generated. This could be expensive so it I would return a reference rather than a value. The user can then decide if they want to copy the reference explicitly into a value.

T Stack&lt;T&gt;::top() const {
    return head-&gt;data;
}


Change to

  T const&amp; Stack&lt;T&gt;::top() const {
//  ^^^^^^
      return head-&gt;data;
  }


Print

Looks fine. 

void Stack&lt;T&gt;::print() const {
    // shared_ptr&lt;Node&lt;T&gt;&gt; temp;
    for (auto temp = head; temp != nullptr; temp = temp-&gt;next)
        cout &lt;&lt; temp-&gt;data &lt;&lt; '\n';
}


The thing I would change is to pass a stream as a parameter (std::cout is not the only stream you want to print to). It can default to std::cout for backward compatibility.

Also the standard way of printing in C++ is using operator&lt;&lt; so it would be nice to define that in addition.

void Stack&lt;T&gt;::print(std::ostream&amp; str = std::cout) const
{
    for (auto temp = head; temp != nullptr; temp = temp-&gt;next)
        str &lt;&lt; temp-&gt;data &lt;&lt; '\n';
}


std::ostream&amp; operator&lt;&lt;(std::ostream&amp; str, Stack&lt;T&gt; const&amp; value)
{
    value.print(str);
    return str;
}