patterncppMinor
Stack Implementation in C++
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(
```
#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 nullptrYou 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
usingstatements at the top of your code. By removing the prefix ofstd::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, likefoo::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 giveClra better name, likeremoveAll, the need for the comment vanishes. This applies to many other comments as well.
- Finally, the
Stackclass should have atoStringmethod, which should return astd::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.