HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Stack Implementation in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationstackstackoverflow

Problem

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
#include
using std::cout; using std::cin; using std::endl;
using std::nothrow; using std::size_t;

template 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 void Stack::Push(T item) {
Node* node = new (nothrow) Node;
if (node == NULL) {
cout value = item;
if (count == 0) {
node->next = NULL;
last = node;
++count;
} else {
node->next = last;
last = node;
++count;
}
}
}
/*
Delete the elements
*/
template void Stack::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 T Stack::Pop() {
if (count == 0) {
cout value;
last = node->next;
delete(node);
--count;
return tempval;
}
}
/*
Print all the elements
*/
template void Stack::Print() {
if (count == 0) {
cout value ";
node = node->next;
}
cout size_t Stack::Size(

Solution

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.

Code Snippets

node->next = NULL;
node->next = nullptr;

Context

StackExchange Code Review Q#103830, answer score: 4

Revisions (0)

No revisions yet.