short linked list

[bejiz]

Hello,
I have written a short program for practising linked lists. But there
is surely something wrong for when I compile there is a unhandled
exception and it does not print if I try to add more than one element
to the list. Is this a problem with the function of the internal class
Node ?
Thanks for any help.
Here is the code:

#include<iostream>
using namespace std;

class UseNodes
{
private:
class Node
{
public:
Node()
{
Data = 1;
Next = 0;
}
Node(int a)ata(a){}
~Node(){};
Node* GetNext()const{ return Next;}
void SetNext( Node* A){ Next = A;}
int GetData()const{ return Data;}
void setData(int a) { Data = a;}
private:
Node* Next;
int Data;
};
Node* Head;
Node* Tail;

public:
UseNodes(){Head = 0; Tail = 0; }
~UseNodes(){};
void AddNode(int a)
{
Node* Temp = new Node(a);
if(Head==0)
{
Head = Temp;
Tail = Temp;
}
else
{
Node* Current = new Node();
Current = Head;
while(Current)
{
Current = Current->GetNext();
}
Current->SetNext(Temp);
Tail = Temp;
}
}
void Print()const
{
int a = 2;
Node* Temp = new Node(a);
if(Head==0)
cout << " Empty list.\n " ;
else
{
Temp = Head;
cout << Head->GetData() << endl;
while(Temp!=Tail->GetNext())
{
cout << Temp->GetData() << endl;
Temp = Temp->GetNext() ;
}
}
}

};

int main()
{
UseNodes A;
A.AddNode(29);
A.AddNode(7777);
A.Print();

getchar();
return 0;
}

 

[Ivan Vecerina]

: Hello,
: I have written a short program for practising linked lists. But there
: is surely something wrong for when I compile there is a unhandled
: exception and it does not print if I try to add more than one element
: to the list. Is this a problem with the function of the internal class
: Node ?
Just quickly skimming through the code...

: Thanks for any help.
: Here is the code:
:
: #include<iostream>
: using namespace std;
:
: class UseNodes
: {
: private:
: class Node
: {
: public:
: Node()
: {
: Data = 1;
: Next = 0;
: }
NB: initialization lists usually should be preferred.
Node() : Data(1), Next(0) {}
Note that it is probably best to omit this default
constructor.

: Node(int a)ata(a){}
This constructor leaves Next uninitialized.
This is probably the cause of the crash.

Also, for technical reasons (read into it), you would
want to label this constructor as explicit:
explicit Node(int a)
: Data(a), Next(00)
{ }

: ~Node(){};
: Node* GetNext()const{ return Next;}
: void SetNext( Node* A){ Next = A;}
: int GetData()const{ return Data;}
: void setData(int a) { Data = a;}
You're mixing upper and lowercase names... ('s'etData).
I would recommend using:
- leading uppercase letter for Class and Type names only
- a trailing underscore for data members

: private:
: Node* Next;
: int Data;
: };
: Node* Head;
: Node* Tail;
:
: public:
: UseNodes(){Head = 0; Tail = 0; }
: ~UseNodes(){};
: void AddNode(int a)
: {
: Node* Temp = new Node(a);
: if(Head==0)
: {
: Head = Temp;
: Tail = Temp;
: }
: else
: {
: Node* Current = new Node();
Should be: new Node(a);

: Current = Head;
: while(Current)
: {
: Current = Current->GetNext();
: }
: Current->SetNext(Temp);
Couldn't you use Tail instead of going through
the whole list to find the last element ?

: Tail = Temp;
: }
: }
: void Print()const
: {
: int a = 2;
: Node* Temp = new Node(a);
What is this for ?
Just remove the two previous lines...

: if(Head==0)
: cout << " Empty list.\n " ;
: else
: {
: Temp = Head;
Best is to declare variables upon first use.
That is, here, replace the previous line with:
Note* temp = head;

: cout << Head->GetData() << endl;
: while(Temp!=Tail->GetNext())
: {
: cout << Temp->GetData() << endl;
: Temp = Temp->GetNext() ;
: }
Very confused, and incorrect again.
You could write:
do {
cout << Temp->GetData() << endl;
Temp = Temp->GetNext();
} while( Temp != 00 );

: }
: }
:
: };
:
: int main()
: {
: UseNodes A;
: A.AddNode(29);
: A.AddNode(7777);
: A.Print();
:
: getchar();
: return 0;
: }


hth -Ivan

 

[bejiz]

Thanks for your detailed advices , my linked list is now working . I
didn't know how to define "Next" , and the functions "AddNode" and
"Print" were quite entangled .
Bejiz

 

[Salt_Peter]

Hello,
I have written a short program for practising linked lists. But there
is surely something wrong for when I compile there is a unhandled
exception and it does not print if I try to add more than one element
to the list. Is this a problem with the function of the internal class
Node ?
Thanks for any help.
Here is the code:

#include<iostream>
using namespace std;

class UseNodes
{
private:
class Node
{
public:
Node()
{
Data = 1;
Next = 0;
}
Node(int a)ata(a){}
~Node(){};
Node* GetNext()const{ return Next;}
void SetNext( Node* A){ Next = A;}
int GetData()const{ return Data;}
void setData(int a) { Data = a;}
private:
Node* Next;
int Data;
};

Your Node class is already a private embedded member of the List class.
There is no need to provide accessors (set/get functions). A ctor
alone will do with init list. Always, always intialize all your
members. Specially pointers like Next above.

Node* Head;
Node* Tail;

public:
UseNodes(){Head = 0; Tail = 0; }
~UseNodes(){};
void AddNode(int a)
{
Node* Temp = new Node(a);
if(Head==0)
{
Head = Temp;
Tail = Temp;
}
else
{
Node* Current = new Node();
Current = Head;
while(Current)
{
Current = Current->GetNext();
}
Current->SetNext(Temp);
Tail = Temp;
}
}
void Print()const
{
int a = 2;
Node* Temp = new Node(a);
if(Head==0)
cout << " Empty list.\n " ;
else
{
Temp = Head;
cout << Head->GetData() << endl;
while(Temp!=Tail->GetNext())
{
cout << Temp->GetData() << endl;
Temp = Temp->GetNext() ;
}
}
}

};

int main()
{
UseNodes A;
A.AddNode(29);
A.AddNode(7777);
A.Print();

getchar();
return 0;
}

A proper naming convention will help your code. Your class represents a
single linked list - then call it that: SingleLinkedList or something
relevant. That way, code like this becomes reusable. One day you'll
scan your projects and something like "UseNodes" won't describe a
single linked list.
Also, never use new without a corresponding delete.

here is one that will store anything, templates are much more usefull
than they are complex.


#include <iostream>
#include <ostream>
#include <string>

template< typename T >
class SingleLinkedList
{
struct Node
{
T t;
Node* p_next;
Node(T val, Node* p = 0) : t(val), p_next(p) { }
} *p_head, *p_tail;
public:
SingleLinkedList() : p_head(0), p_tail(0) { }
SingleLinkedList(const SingleLinkedList& r_l); // disabled copy ctor
~SingleLinkedList() { clear(); }
/* member functions */
void push_back(const T& t)
{
if (p_head == 0)
{
p_head = p_tail = new Node(t);
} else {
p_tail->p_next = new Node(t);
p_tail = p_tail->p_next;
}
}
void clear()
{
while(p_head != 0)
{
Node* p_zap = p_head;
p_head = p_head->p_next;
delete p_zap;
}
p_tail = 0;
}
/* friends */
friend
std:stream& operator<<(std:stream& os, const SingleLinkedList&
r_l)
{
Node* piterator(r_l.p_head);
while(piterator != 0)
{
os << piterator->t << "\n";
piterator = piterator->p_next;
}
return os;
}
};

int main()
{
SingleLinkedList< int > nlist;
for(int n = 0; n < 5; ++n)
{
nlist.push_back(n);
}
std::cout << nlist;

SingleLinkedList< double > dlist;
for(int i = 0; i < 5; ++i)
{
dlist.push_back(static_cast<double>(i) / 4);
}
std::cout << dlist;

SingleLinkedList< std::string > slist;
slist.push_back("string_0");
slist.push_back("string_1");
slist.push_back("string_2");
std::cout << slist;

return 0;
}

/*
0 <- nlist
1
2
3
4
0 <- dlist
0.25
0.5
0.75
1
string_0 <- slist
string_1
string_2
*/

 

[Thomas J. Gritzan]

: void AddNode(int a)
: {
: Node* Temp = new Node(a);
: if(Head==0)
: {
: Head = Temp;
: Tail = Temp;
: }
: else
: {
: Node* Current = new Node();
Should be: new Node(a);

: Current = Head;

No, should be:

Node* Current = Head;

: while(Current)
: {
: Current = Current->GetNext();
: }
: Current->SetNext(Temp);

At this point, Current is NULL. You can't use it. Better:

while (Current->GetNext())
{
Current = Current->GetNext();
}

Or better use Tail as Ivan suggested.