patterncppModerate
Stack implementation using linked list
Viewed 0 times
stackusingimplementationlistlinked
Problem
This is a working stack implementation using a linked list. I'm just curious to know if this is a good way of doing it. Any suggestions are welcome. Can we keep track of the number of elements in the stack using a
count variable as used in the code?#include
#include
using namespace std;
class node
{
public:
int data;
node* next;
};
class StackusingList
{
public:
StackusingList(int max)
{
top = NULL;
maxnum = max;
count=0;
}
void push(int element)
{
if(count == maxnum)
coutdata = element;
newTop->next = NULL;
top = newTop;
count++;
}
else
{
newTop->data = element;
newTop->next = top;
top = newTop;
count++;
}
}
}
void pop()
{
if(top == NULL)
coutnext;
count--;
delete(old);
}
}
void print()
{
node *temp;
temp = top;
while(temp!=NULL)
{
coutdatanext;
}
}
private:
node *top;
int count; //head
int maxnum;
int stackData;
};
int main(int argc, char** argv) {
StackusingList *sl = new StackusingList(5);
sl->push(1);
sl->push(2);
sl->push(3);
sl->push(4);
sl->push(5);
sl->push(6);
sl->pop();
coutprint();
return 0;
}Solution
-
At some point, I'd recommend using
-
It'd be better to define
-
Prefer
-
-
-
In both
-
-
Watch out:
You do not call
At the end of
Beyond that, you really don't need to use
-
-
You're missing a few useful
-
There is no destructor! You're allocating
With the destructor defined, you'll need to satisfy The Rule of Three. This is also important because the default copy constructor and assignment operator will perform a shallow copy of
At some point, I'd recommend using
template here. This will allow you to store any date type, not just ints, in your structure.-
It'd be better to define
node as a struct inside StackusingList as private. In your current implementation, node's fields are not hidden because they are all made public.class LinkedList
{
private:
struct Node
{
};
public:
};-
Prefer
nullptr to NULL if you're using C++11.-
StackusingList() should be an initializer list:StackusingList(int max) : top(NULL), maxnum(max), count(0) {}-
count should be of type std::size_t.-
In both
push() and pop(), you'll need to return if the stack is full or empty respectively. Otherwise, the function will continue to execute, defeating the purpose of the check.-
print():// no data members are being modified,
// so make this const
void print() const
{
// could just be initialized
// the asterisk is commonly
// put next to the type in C++
node* temp = top;
while (temp)
{
cout data next;
}
}-
Watch out:
StackusingList *sl = new StackusingList(5);You do not call
delete with this new, thereby causing a memory leak! Always call delete properly with every new.At the end of
main() before the return:delete s1;Beyond that, you really don't need to use
new here. It's only necessary with the nodes, which you're already doing. It's best to just avoid manual allocation/deallocation as much as possible.-
stackData isn't used anywhere (and doesn't have a clear meaning), so just remove it.-
You're missing a few useful
public member functions:std::size_t size() const { return count; } // put into headerbool empty() const { return count == 0; } // put into headertemplate
T StackusingList::top() const { return top->data; }-
There is no destructor! You're allocating
new nodes, so you have to define the destructor to properly delete them:StackusingList::~StackusingList()
{
node* current = top;
while (current)
{
node* next = current->next;
delete current;
current = next;
}
top = NULL;
}With the destructor defined, you'll need to satisfy The Rule of Three. This is also important because the default copy constructor and assignment operator will perform a shallow copy of
top. This means that the pointer itself will be copied instead of just the data at that pointer. This will cause problems if you try to copy list objects or initialize new ones with existing ones.Code Snippets
class LinkedList
{
private:
struct Node
{
};
public:
};StackusingList(int max) : top(NULL), maxnum(max), count(0) {}// no data members are being modified,
// so make this const
void print() const
{
// could just be initialized
// the asterisk is commonly
// put next to the type in C++
node* temp = top;
while (temp)
{
cout << temp-> data << ",";
temp = temp->next;
}
}StackusingList *sl = new StackusingList(5);std::size_t size() const { return count; } // put into headerContext
StackExchange Code Review Q#33195, answer score: 10
Revisions (0)
No revisions yet.