current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

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;
}
share|improve this question

2 Answers 2

up vote 4 down vote accepted

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++09, 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 an std::string representation of the stack.
share|improve this answer
    
Nitpick: you mean "suffix" when talking about << "\n". –  fluffy 46 mins ago
    
@fluffy Edited. –  Ethan Bierlein 46 mins ago
up vote 3 down vote

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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