Return to Answer

• 1

• 97.7k
• 5
• 126
• 341

• 97.7k
• 5
• 126
• 341

###Readability Please put a space after the include and before the <

#include<iostream>
#include<string>
#include<memory>

###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 <typename T>
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 <typename T>

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 <T> after the class name, it is the syntax requirement
template <typename T>
void Stack<T>::push(const T &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<Node> 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& d, shared_ptr<Node>& n) : data(d), next(n) {}

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

 Node(T&& d, shared_ptr<Node>& n) : data(std::move(d)), next(n) {}

###Push You don't need that if block.

void Stack<T>::push(const T &data) {
 shared_ptr<Node<T>> temp = make_shared<Node<T>>(data,nullptr);
 if (head == nullptr) {
 head = temp;
 }
 else {
 temp->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<T>::push(const T& data) {
 head = make_shared<Node<T>>(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<T>::push(T&& data) {
 head = make_shared<Node<T>>(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<typename... Args>
void Stack<T>::push<Args>(Args...&& args) {
 head = make_shared<Node<T>>(std::move(args));
}
template<typename... Args>
Node<T>::Node<Args...>(Args...&& 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<T>::top() const {
 return head->data;
}

###Print

Looks fine.

void Stack<T>::print() const {
 // shared_ptr<Node<T>> temp;
 for (auto temp = head; temp != nullptr; temp = temp->next)
 cout << temp->data << '\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<< so it would be nice to define that in addition.

void Stack<T>::print(std::ostream& str = std::cout) const
{
 for (auto temp = head; temp != nullptr; temp = temp->next)
 str << temp->data << '\n';
}
std::ostream& operator<<(std::ostream& str, Stack<T> const& value)
{
 value.print(str);
 return str;
}