Dont use if to test and return bool
if(top==-1)
return true;
else
return false;
This is easier to write as:
return top == -1;
Overflow
if(top>=-1){
// STUFF
}
else
std::cout<<"Overflow"<<std::endl;
Is top being -1 really overflow? Would not overflow be when you ran out of memory for list items. Especially since there is not interface that allows you to get the size of the stack.
Also I don't expect a diagnostic message. If something fails to happen its an error (by that I mean exception). If you fail to push the code that called push needs to know that the operation failed otherwise it will keep on doing what it is doing. The best way to indicate to the calling code something went wrong is an exception.
Design
You push and pop to the back of the list. Why not push and pop from the front of the list then you will never need to traverse the list to find the item.
Bug
When you pop, you don't actually remove the item from the list (or delete it). So poping gives you the last value but does nto remove it thus it will continue to give you the last item.
Efficiency
Checking for underflow/overflow. Sure this is good practice. But what about code were I know it is not going to under/over-flow. Do I need to check then? In most C++ code effeciency is paramount and you usually provide unchecked versions for situations where the check is not needed.
Stack s;
// Fill stack
while(!s.empty())
{
std::cout << s.pop(); // Does this pop need to be checked.
// We have already done a manual check
// to make sure that a pop() is valid
// by calling empty() first.
}
Note: There is no issue with having both a checked and unchecked version of a call. Lookat std::vector<> and operator[] Vs at().
Minor Things
The call to empty should be const:
bool Stack<T>::stack_empty() const
^^^^^
Easier to declare the operator<< as a friend then you don't need to worry about the template declaration.
template<class U>
std::ostream & operator <<(std::ostream& os, const Stack<U>& s){
s.display(os);
return os;
}
Put it inside the class and mark as friend:
friend std::ostream& operator<<(std::ostream& os, Stack const& s) {
s.display(os);
return os;
};
Revised Version
#include <iostream>
template<class T>
class Stack{
struct Node{
T data;
Node *next;
// This is the standard constructor
// Where you copy the `val` into the node.
Node(Node* next, T const& val)
: data(val)
, next(next)
{}
// This is the move constructor
// It moves the content of `val` into the node. For
// types like vectors this is much more efficient as it
// simply means copying three (or so) pointers and thus
// transferring the internal containers without the cost
// of copying all the elements in the vector.
Node(Node* next, T&& val)
: data(std::move(val))
, next(next)
{}
// This is an emplace constructor.
// Rather than passing a `T` into the node you pass all the
// parameters need to construct a `T` then create the T
// in place as the node object is constructed.
template<typename... Args>
Node(Node* nect, Args&&... args)
: data(std::forward<Args>(args)...)
, next(next)
{}
};
Node* head;
public:
Stack()
: head(nullptr)
{}
// Disable the copy semantics as the default version does not
// work when you have an owned RAW pointer in your class (like head).
Stack(Stack const&) = delete;
Stack& operator=(Stack const&) = delete;
~Stack()
{
// Reclaiming all the nodes.
for(Node* next; head != nullptr; head = next)
{
next = head->next;
delete head;
}
}
bool empty() const
{
return head == nullptr;
}
// Pass a reference to a value and use it to create a node with a copy.
void push(T const& value)
{
head = new Node(head, value);
}
// Pass an r-value reference to a value and use it to create a node with a move.
void push(T&& value)
{
head = new Node(head, std::move(value));
}
// Pass a reference to set of parameters that can be used to
// construct a T object in place. This is know as perfect forwarding.
template<typename... Args>
void push(Args&&... args)
{
head = new Node(head, std::forward<Args>(args)...);
}
// pop a value from the head node
// delete and remove the head node.
T pop()
{
Node* tmp = head;
head = head->next;
T result = std::move(tmp->data);
delete tmp;
return result;
}
// Print a stack.
void display(std::ostream& out = std::cout) const
{
for(Node* loop = head; loop != nullptr; loop = loop->next)
{
out << loop->data << " ";
}
}
// Implement the stream operators for Stack
friend std::ostream& operator<<(std::ostream& os, Stack const& s)
{
s.display(os);
return os;
}
};
int main()
{
Stack<int> stack1;
stack1.push(3);
stack1.push(4);
stack1.push(11);
stack1.push(30);
std::cout<<stack1<<std::endl;
// No need to check pop()
// I know there are four items in the stack
// If a push had failed it would have thrown an exception.
std::cout<<stack1.pop()<<std::endl;
}
