using namespace std; // TESTING

Judging by your comment, I'm guessing you're aware about the problems of exposing namespace members in the global scope. But did you know that you canusing namespace inside scopes other than the global? It is fine if in your unit tests you want to expose thestd stuff to make the code less verbose, but do that in the shortest possible scope then, in this case, insidemain():

int main(){    // Make an exception since this is a unit test and expose    // the whole Standard Library inside main(), so we don't have     // to std:: qualify everything.    using namespace std;    // same as before ...}

 cout << "Destroyed" << endl;  myStack.~Stack();

Very unusual for you to be calling the destructor formyStack. A destructor is called automatically when an object goes out of scope or isdeleteed. There are very few cases where a programmer would manually call a destructor, those are not present here. If you just wan't to ensure the object is destroyed before the end ofmain(), to log some stuff, wrap its declaration inside a new scope. E.g.:

int main(){    {        Stack<char> myStack(3);        ...    } // <== ~myStack() called here}

This way you won't risk ending with a half destroyed object in your hands.

template <typename T>T Stack<T>::top() const{    if (head != nullptr)    {        return head->data;    }    else return NULL;    //          ^^^^ problem here!}

That will not work for aT type that is not a pointer or integer.NULL can be assigned to integers because it is usually implemented as#define for0. That's one of the reasons why you should usenullptr whenever possible (C++11). If you had usednullptr, your test withT=char would have failed and you would have noticed this problem.

The usual convention for a generic stack is to throw an exception if you try to access the top for an empty stack. Returning a "default" value is less generic and also makes it harder for the caller to detect errors. Returning a default also requires the typeT to be default constructible, so don't do it.

Same goes for popping on an empty stack. Right now you are not generating any errors. You should consider throwing and exception. Deriving aStackUnderflow exception type fromstd::runtime_error might be a good idea. Then you can extend the concept to aStackOverflow error when trying to push to the bounded stack. Granted that you can do with a simplestd::runtime_error, but defining custom exception classes is a nice exercise if that's your point for writing this implementation.

cout << "push: "; myStack.push('H'); cout << myStack.top() << endl;

Don't pack that many statements in a single line. People read shorter columns of text much faster. But not just that, mixing several statements together makes it a lot easier for a quick read to miss some important part of it. Put each statement in its own line.

cout << "push: "; myStack.push('H'); cout << myStack.top() << endl;

Not a huge thing, but usually this kind of data structures usesstd::size_t for things likesize andcapacity. That's an unsigned integer, which makes sense since the stack size is not meant to be negative, however, there is some discussion aboutavoiding unsigned integers, so that's up to you to decide if it is worth it.

Last thing is a bit of a big subject, which was introduced with C++11, calledmove semantics. Your code does some unnecessary copying of data onpush andStackNode's constructor, which might affect performance whenT is something other that a native type or pointer. I'll leave you with a couple references you can read to further learn about this:

• SO: What are move semantics?

• cppreference.com: Move constructors.

• \$\begingroup\$That's great thanks! One question - I used the destructor just go see if it did clear the stack, if I wanted a function to clear the stack should I just wrap the destructor in a method called clear() ?\$\endgroup\$

  user3392999

  – user3392999

  2015-07-16 23:26:58 +00:00

  CommentedJul 16, 2015 at 23:26
• \$\begingroup\$@user3392999, Sure! Having a publicclear() is a good idea. That allows you to implement the destructor by simply calling the clear function.\$\endgroup\$

  glampert

  – glampert

  2015-07-17 00:44:58 +00:00

  CommentedJul 17, 2015 at 0:44

You've quite correctly marked theStack full printout with adelete comment, however youmust give an indication thatpush failed. An easiest way is to make it returnbool.

It is unclear why do you want to restrict stack capacity. The only reason of implementing stack via a linked list (as opposed to fixed-size array) is to have a virtually unlimited capacity.

StackNode default constructor is never used (and never shall be used), so I recommend to make sure it is never called. In any case it should initializedata asdata(T(0)).

• \$\begingroup\$Won't the default constructor be called if I don't specify a value? Such as Stack<int> myStack ?\$\endgroup\$

  user3392999

  – user3392999

  2015-07-16 23:22:37 +00:00

  CommentedJul 16, 2015 at 23:22
• \$\begingroup\$@user3392999, vnp meant the default constructor you've written forStackNode. Which is not useful because you only crate stack nodes withnew StackNode<T>(val, head);. To prevent misuse ofStackNode, you could either make the default constructorprivate or delete it altogether withStackNode() = delete; in the declaration.\$\endgroup\$

  glampert

  – glampert

  2015-07-17 00:50:00 +00:00

  CommentedJul 17, 2015 at 0:50

• c++
• c++11
• linked-list
• stack