Sign in to follow this  
monchito

Error in my linklist template

Recommended Posts

Hello everyone.
I'm new with templates in C++ and started to learn something. I'm using
Code::Blocks 8.02 and mingw32-g++
With this code I can work with one list without crashing anything.
I got a problem when creating a second list. The first is created correctly
and the program crashes at the second list creation.
Using the cout's I got:
pushed first time to the list
node added to the list
12
35
list size.. 2

What I'm doing wrong? Actually after a few hours I'm tired and find no cure for this behavior. Could someone look at my code and show me the way?
Thanks in advance.


#include <iostream>
using namespace std;

template < typename T >
class CStack
{
struct node
{
T data;
struct node * next;
};

public:
CStack():root(NULL) { };
~CStack(){ };

private:
struct node * root;

public:
T top( )
{
struct node * tmp = root;
if( root == NULL )
return 0;
else
{
while( tmp->next != NULL )
tmp = tmp->next;
}
return tmp->data;
}

void push( T const &ndata )
{
if( root == NULL )
{
struct node * tmp = new struct node;
tmp->next = NULL;
tmp->data = ndata;
root = tmp;
cout << "\n pushed first time to the list" << endl;
}
else
{
struct node * tmp = root;
while( tmp->next != NULL )
tmp = tmp->next;

struct node * tmp2 = new struct node;
tmp2->next = NULL;
tmp2->data = ndata;
tmp->next = tmp2;
cout << "\n node added to the list" << endl;
}
}

void showData( )
{
if( root == NULL )
{
cout << "\n no data" << endl;
}
else
{
struct node * tmp = root;
do {
//cout << "\n traversing the list" << endl;
cout << tmp->data << endl;
tmp = tmp->next;
}while( tmp != NULL );
}
}

unsigned int size( )
{
unsigned int i = 0;

if( root != NULL )
{
struct node * tmp = root;
while ( tmp != NULL )
{
tmp = tmp->next;
i++;
//cout << "\n counting the list... " << i << endl;
}
}
return i;
}

void clear()
{
if ( root == NULL ) return;
else
{
struct node * tmp = root;
if ( tmp->next == NULL )
{
delete tmp;
root = NULL;
}
else
{
struct node * tmp2 = NULL;
while ( tmp->next != NULL )
{
tmp2 = tmp;
tmp = tmp->next;
delete tmp;
//cout << "\n deleting node" << endl;
}
delete tmp;
tmp2->next = NULL;
}
}
}

void pop()
{
if ( root == NULL ) return;
else
{
struct node * tmp = root;
if ( tmp->next == NULL )
{
delete tmp;
root = NULL;
}
else
{
struct node * tmp2 = NULL;
while ( tmp->next != NULL )
{
tmp2 = tmp;
tmp = tmp->next;
}
delete tmp;
tmp2->next = NULL;
//cout << "\n deleting node" << endl;
}
}
}

};

int main()
{
CStack < int >* lista;
lista->push(12);
lista->push(35);
lista->showData();
cout << "list size...:";
cout << lista->size() << endl;

CStack < int >* listb;
listb->push(3); // --->Causing Error
// listb->push(4);
// listb->showData();
// cout << "list size...:";
// cout << listb->size() << endl;

// cout << listb->top() << endl;
// listb->pop();
// lista->clear();
// listb->clear();
return 0;
}


Share this post


Link to post
Share on other sites
Quote:

CStack < int >* lista;
lista->push(12);
CStack < int >* listb;
listb->push(3); // --->Causing Error

I have no idea how that even worked the first time...
You need to initialize your pointers! Neither lista nor listb are pointing at anything. Why are they even pointers to being with? take out the '*'s and see where that gets you.

Share this post


Link to post
Share on other sites
You don't need dynamic allocation in main(). The easiest way to manage this is to use "automatic" allocation:

int main()
{
// Look ma, no pointers!
CStack < int > lista;
lista.push(12);
lista.push(35);
lista.showData();
cout << "list size...:";
cout << lista.size() << endl;

CStack < int > listb;
listb.push(3);
}


Now you don't need to remember to delete the pointers if you do this.

You need to implement your destructor properly to clean up the memory even if clear() isn't called. You should also implement the copy constructor and assignment operator, or make your type noncopyable.

In C++ you don't need to use the "struct" keyword when declaring objects of those types.

It would be more efficient to implement your stack as a backwards linked list. That is, the "top" element should be the first item. That way both top(), pop() and push() would be O(1), right now they are O(n).

Your implementation of clear() and pop() look a little complex. Clearing or popping from a one element list should not need a special case over clearing an N element list.

Finally, you are aware of std::list<> and std::stack<>, right?

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this