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

Stack implementation using linked list

Submitted by: @import:stackexchange-codereview··
0
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 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 header


bool empty() const { return count == 0; } // put into header


template 
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 header

Context

StackExchange Code Review Q#33195, answer score: 10

Revisions (0)

No revisions yet.