5
\$\begingroup\$

This is my first template implementation. I'm trying to learn C++ and Algorithms. Any constructive criticism on code quality, readability is welcome. Everyone has their own style, but there are certain rules which all good programmers should follow. Kindly remark as to if I have followed those and what rules are missing.

#include <iostream>
#include <new>
using std::cout; using std::cin; using std::endl;
using std::nothrow; using std::size_t;

template<typename T> class Stack {
 public:
  // constructor for class
  Stack() { count = 0;
            last = NULL; }
  // destructor for class
  ~Stack() {  if (count > 0) Clr(); }
  // push item at last of stack
  void Push(T item);
  // pop item from last of stack
  T Pop();
  // get size of stack
  size_t Size();
  void Clr();       // delete all the elements
  // print all the elements
  void Print();
 private:
  // the stack will store elements in the form of a linked list
  struct Node {
    T value;
    Node* next;
  };
  Node* last;   // last points to the last item of the stack
  size_t count;    // number of elements in stack
};
/*
Check for memory allocation first. If memory allocated, then Push element in
the stack. 
 */
template<typename T> void Stack<T>::Push(T item) {
  Node* node = new (nothrow) Node;
  if (node == NULL) {
    cout << "Memory allocation failed " << endl;
    return;
  } else {
    node->value = item;
    if (count == 0) {
      node->next = NULL;
      last = node;
      ++count;
    } else {
      node->next = last;
      last = node;
      ++count;
    }
  }
}
/*
Delete the elements
 */
template<typename T> void Stack<T>::Clr() {
  Node* node = last;
  while (count != 0) {
    Node* tempnode = node;
    node = node->next;
    delete tempnode;
    --count;
  }
}
/*
Pop the last element of the stack and return it
 */
template<typename T> T Stack<T>::Pop() {
  if (count == 0) {
    cout << "No element to delete" << endl;
    return (T)0;
  } else {
    Node* node = last;
    T tempval = node->value;
    last = node->next;
    delete(node);
    --count;
    return tempval;
  }
}
/*
Print all the elements
 */
template<typename T> void Stack<T>::Print() {
  if (count == 0) {
    cout << "No elements to print" << endl;
    return;
  } else {
    cout << "Printing from top of the Stack to Bottom" << endl;
    Node* node = last;
    while (node != NULL) {
      cout << node->value << "->";
      node = node->next;
    }
    cout << endl;
  }
}
/*
Get the size of the Stack
 */
template<typename T> size_t Stack<T>::Size() {
  return count;
}
/*
Main function
 */
int main() {
  Stack<int> s;
  s.Push(12);
  s.Push(10);
  s.Push(23);
  s.Push(34);
  cout << "The value Popped from the Stack: " << s.Pop() << endl;
  s.Print();
  return 0;
}
\$\endgroup\$
  • \$\begingroup\$ NEVER do this using std::cout; using std::cin; using std::endl; in a header file. You will change the meaning of my code just because I include your header file. That is really bad and will get your code banned from projects. \$\endgroup\$ – Martin York Sep 5 '15 at 14:28
4
\$\begingroup\$

NULL versus nullptr

You shouldn't be using NULL, as you did in various places, like here:

node->next = NULL;

NULL is deprecated, and you should be using nullptr, like this:

node->next = nullptr;

If this is legacy code, and you need to support users running old compilers that date to before C++11, you should keep using NULL.


std::endl versus "\n"

Rather than using std::endl, you should just suffix this onto the end of any std::cout statements:

<< "\n";

You should be doing this unless you're very sure that you want to flush the output buffer each time you run std::cout. If you do need to flush the output buffer, then you should keep using std::endl.


Nitpicks

There are a fair amount of things that I want to pick at here, so bear with me.

  • You should remove all the using statements at the top of your code. By removing the prefix of std:: on some of it's members, your code loses clarity. The only time it's really appropriate to use a namespace, or namespace members is when you have a really deep nested namespace, like foo::bar::baz::barbaz::blah.
  • Secondly, there are a lot of useless comments. For example, the comment by the function signature for Clr, is not really needed. If you give Clr a better name, like removeAll, the need for the comment vanishes. This applies to many other comments as well.
  • Finally, the Stack class should have a toString method, which should return a std::string-representation of the stack.
\$\endgroup\$
  • \$\begingroup\$ std::cout << '\n'; should do I.e. char should do. Also going with the standard function, it should be std::to_string. \$\endgroup\$ – legends2k Sep 5 '15 at 14:17
3
\$\begingroup\$

Use Virtual destructors

This for no other reason than polymorphism, so if you are not planning on having another class inherit from this one, ignore the next little bit. Example:

class base {};
class derived : public base {
    public:
    ~derived() {}
};

int main() {
    base* b = new derived();
    delete b; // this is undefined unless base defines a destructor that is virtual
}

Read more here

Constructors should be declared "explicit"

The reason for doing this is because it is actually possible in C++ to do something like this:

class myclass {
    int value;
    public:
    myclass(int v) : value(v) {}
};

int main() {
    myclass c = 5;
    return 0;
}

And the compiler will not even complain. However, if you had declared your constructor like this:

explicit myclass(int v) : value(v) {}

Then the above code would have resulted in a compiler error.

Printing error messages is for debugging

This one might be debatable but you should throw an exception rather than printing an error message when something unexpected has happend.

I call this the fail-fast pattern. In essence, you don't want the person reusing your class to keep using it the wrong way and only end up realising this when the code has developed into an application. No! You want to alert them immediately something goes wrong; And the way to do this is not by printing nice error messages to standard output, but rather to throw an exception and halt program execution until the error has been fixed.

Misc.

  • Your Print function should be able to print not just to standard output but also to some other output stream the user wants. This means that it should take an std::ostream object as parameter and use that instead of std::cout

  • Your Clr function should use Pop for deleting items. This reduces the amount of stress you face if you end up finding a fault with the way Clr works

  • Replace all if (count == 0) or if (count != 0) statements with a function called isEmpty() which returns the value of that expression

\$\endgroup\$
2
\$\begingroup\$

There are already two reviews with many good points, one by Ethan Bierlein and a second by Smac89.
I will explicitly mention all their recommendations, criticizing, emphasizing and putting them into context as I go.

  1. First, the load of distracting comment should be contained and severely pruned back.
    Ethan recommends reducing the need for comments by proper naming, which is sound advice, and deserves more emphasis.
    You already did much of that, but you restate even the blindingly obvious in comments.
    I recommend reading about DRY (Don't Repeat Yourself), or as Jeff calls this aspect of it, Coding Without Comments.

  2. If there are any comparable interfaces already, choose one and conform to it as best you can. Doing so enables code-reuse and minimizes confusion and neccessary documentation. In your case, look at the standard containers, specifically std::stack.
    For example, Size() should be size() and accompanied by empty(). Pop and Push should be lowercase.

  3. Ethan's praise for nullptr over NULL is sound advice, because even though NULL is conceptually a pointer of unknown type (and thus should be the defined as nullptr), it could also be defined as a number for backwards-compatibility.
    Still, NULL isn't deprecated, nullptr doesn't exist pre-C++11, and if your C++ code shall also be valid C++/CLI code, using nullptr will lead to much fun in the minecraft sense.

  4. Ethan is death on using using, but there's some nuance to it.

    While in an implementation-file, pulling in some symbols from other namespaces (or in certain cases even all symbols from select namespaces, read https://stackoverflow.com/q/1452721 for when it isn't) is useful to avoid being needlessly long-winded and repetitive, in header-files (https://stackoverflow.com/q/495021), introducing any symbol you don't have to (which is not part of your interface) courts the dangers of inadvertent dependencies and breakage.

  5. You should consider putting your code into your own namespace to avoid future clashes.

    You might even want to use an inline-namespace for versioning.

  6. Ethan's recommendation for std::endl versus \n, while basically sound, should actually be: Eschew explicit flushing unless you need it to ensure timely output.

  7. You could provide an iterator-interface to get all elements, though that's not customary for a stack. Mutable ForwaddIterator is the concept it should fulfill.
    The advantages are access to standard algorithms and general ability to reduce coupling.

  8. If you want to allow conversion to std::string as Ethan advocates, use a free to_string-function.
    You can use the iterator-interface for it if you implemented that, or it must be a friend-function. Not much use outside debugging.

  9. As Smac89 says, if you supply a Print-function it accept a user-supplied std::ostream (though you can (and maybe should) certainly provide a default-argument of std::cout).
    It can be implemented in terms of to_string, and all the other remarks I made for that apply too.

  10. Smac's point on virtual destructory isn't really germane to your current class, though should be kept in mind.

    Use a virtual destructor if the class has any other virtual members (functions, variables and bases, inherited or direct), or you want to destroy polymorphically, neither of which applies to you.
    In general, avoid virtual members because they add conceptual complexity (adding a contract about when it must be called, this does not strictly apply to the destructor) and runtime overhead.

  11. Never silently keep going at an error, like failure to allocate neccessary memory.
    Iff there's internal corruption or there's no chance the caller might be able to recover, abort the program (and try to log details to logfile/stderror, never to standard output).
    Otherwise, but only if the error is expected, your function should return an error-code.
    Exceptions are the preferred way though.

  12. Your code violates the rule of three, the implicit copy-/move- constructors and assignment-operators are wrong.

    Either disable them with =delete, or implement them yourself.

  13. You should put the pointer first in struct Node to enable more identical-code-folding.

  14. Your stack is not exception-safe, it needlessly assumes copying or assigning a T can never throw. Correct that.

    Using std::unique_ptr where possible makes that easy.


Now going into the details of some of your functions:

  1. Push: You have a useless if-else-statement in there:

    template<typename T> void Stack<T>::push(T item) {
        auto x = make_unique<Node>{last, std::move(T)};
        ++count;
        last = x.release();
    }
    
  2. Clr: You shouldn't constantly update the member-variables, it should be called clear, otherwise ok.

  3. Pop: Underflow should be a fatal error. Also, if you want a default-element, better use T().

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.