operator overloading and destructor problem

[ganesh.tambat]

Hi,

Please see below a piece of code. Here I am trying to create a linked
list by attaching two linked list together. I have overloaded operator
+ for this. Now the output always says that the list is empty. I added
copy constructor and overloaded assignment operator but still the
behavior remain same. Can anyone guide me on this ?

Thanks
Ganesh

class LinkedList
{
private:
class Node
{
public:
int data;
Node * Next;
Node()
{
data = 0;
Next = NULL;
}
};
struct Node * Head;
public :
int AddNode(int data);
int RemoveNode(int data);
void Display(void);
void Reverse(void);

LinkedList(LinkedList &oldlist)
{
if(oldlist.Head != NULL)
{
Head = new Node;
Head->data = oldlist.Head->data;

if(oldlist.Head->Next != NULL)
{
Node * tmpoldlist = oldlist.Head->Next;
Node * tmpprevnewlist = Head;
Node * tmpnextnewlist = tmpprevnewlist;

while(tmpoldlist != NULL)
{
tmpnextnewlist = new Node;
tmpnextnewlist->data = tmpoldlist->data;
tmpnextnewlist->Next = NULL;
tmpprevnewlist->Next = tmpnextnewlist;
tmpprevnewlist = tmpnextnewlist;
tmpoldlist = tmpoldlist->Next;
}
}
}
}

LinkedList operator=(LinkedList oldlist)
{
LinkedList mylist;

if(oldlist.Head != NULL)
{
mylist.Head = new Node;
mylist.Head->data = oldlist.Head->data;

if(oldlist.Head->Next != NULL)
{
Node * tmpoldlist = oldlist.Head->Next;
Node * tmpprevnewlist = mylist.Head;
Node * tmpnextnewlist = tmpprevnewlist;

while(tmpoldlist != NULL)
{
tmpnextnewlist = new Node;
tmpnextnewlist->data = tmpoldlist->data;
tmpnextnewlist->Next = NULL;
tmpprevnewlist->Next = tmpnextnewlist;
tmpprevnewlist = tmpnextnewlist;
tmpoldlist = tmpoldlist->Next;
}
}
}
return mylist;
}

LinkedList operator+(LinkedList tlist4)
{
LinkedList tlist3;
Node * tmpNode = NULL, *tmpNode1 = NULL;

if(Head != NULL)
{
tmpNode = Head;
do
{
tlist3.AddNode(tmpNode->data);
tmpNode = tmpNode->Next;
}
while(tmpNode != NULL);
}

tmpNode = NULL;

if(tlist4.Head != NULL)
{
tmpNode1 = tlist4.Head;
do
{
tlist3.AddNode(tmpNode1->data);
tmpNode1 = tmpNode1->Next;
}
while(tmpNode1 != NULL);
}

return tlist3;
}

LinkedList()
{
Head = NULL;
}

~LinkedList()
{
Node * old;
while(Head != NULL)
{
old = Head;
if(Head->Next != NULL)
{
Head = Head->Next;
}
else
{
break;
}
delete(old);
}
delete Head;
}
};

[Donovan Rebbechi]

On 2005-06-04, ga***********@gmail.com <ga***********@gmail.com> wrote:

Hi,

Please see below a piece of code. Here I am trying to create a linked
list by attaching two linked list together. I have overloaded operator
+ for this.

You don't have the AddNode method, so your code will not compile.

You have a lot of special casing. Why do this ? The code for operator+
could look like this:

LinkedList operator+ (const LinkedList& L){
LinkedList result(*this);
for (const Node* n = L.Head; n; n = n->Next)
result.AddNode(n->data);
return result;
}

This version is simpler than yours, right ? 4 lines of code against however
many yours uses. That's the number one reason your code has bugs. You make
it unnecessarily complex, which increases the chance you'll mess something up.

BTW, don't pass objects by value if const reference will do.

Cheers,
--
Donovan Rebbechi
http://pegasus.rutgers.edu/~elflord/

[Bob Hairgrove]

On 4 Jun 2005 11:10:05 -0700, ga***********@gmail.com wrote:

Hi,

Please see below a piece of code. Here I am trying to create a linked
list by attaching two linked list together. I have overloaded operator
+ for this. Now the output always says that the list is empty. I added
copy constructor and overloaded assignment operator but still the
behavior remain same. Can anyone guide me on this ?
Yes ... use std::list instead!

I didn't even try to debug your code, there are so many beginner's
mistakes in it. You really need to read Scott Meyers books "Effective
C++" and "More Effective C++", and especially Stroustrup's "The C++
Programming Language".

Here are some comments interspersed within your code:
class LinkedList
{
private:
// class Node
// (everything is public, so struct is clearer)
struct Node
{
int data;
Node * Next;
Node()
// prefer an initialization list:
// data(0), Next(0)
{
data = 0;
Next = NULL;
}
};
Node * Head;
public :
int AddNode(int data);
int RemoveNode(int data);
void Display(void); // void argument is superfluous
// that probably should be:
// void Display() const;
void Reverse(void);
// void Reverse();

LinkedList(LinkedList &oldlist) // copy ctor should take LinkedList const &
{
if(oldlist.Head != NULL)
// if (oldlist.Head) is better style
{
Head = new Node;
Head->data = oldlist.Head->data;

if(oldlist.Head->Next != NULL)
{
Node * tmpoldlist = oldlist.Head->Next;
Node * tmpprevnewlist = Head;
Node * tmpnextnewlist = tmpprevnewlist; // = Head ???

while(tmpoldlist != NULL)
// while(tmpoldlist)
{
tmpnextnewlist = new Node; // what about the previous assignment??
tmpnextnewlist->data = tmpoldlist->data;
tmpnextnewlist->Next = NULL; // prefer 0 to the NULL macro
tmpprevnewlist->Next = tmpnextnewlist;
tmpprevnewlist = tmpnextnewlist; // huh ??
tmpoldlist = tmpoldlist->Next;
}
}
}
}
// copy ctor's canonical signature is:
// LinkedList & operator=(LinkedList const & oldlist)
LinkedList operator=(LinkedList oldlist) // should take LinkedList const &;
// you are creating two unnecessary
// temporary objects here!!
{
// guard against self-assignment here!!
// if (this != &oldlist)
// {
LinkedList mylist;

if(oldlist.Head != NULL)
{
mylist.Head = new Node;
mylist.Head->data = oldlist.Head->data;

if(oldlist.Head->Next != NULL)
{
Node * tmpoldlist = oldlist.Head->Next;
Node * tmpprevnewlist = mylist.Head;
Node * tmpnextnewlist = tmpprevnewlist;

while(tmpoldlist != NULL)
{
tmpnextnewlist = new Node;
tmpnextnewlist->data = tmpoldlist->data;
tmpnextnewlist->Next = NULL;
tmpprevnewlist->Next = tmpnextnewlist;
tmpprevnewlist = tmpnextnewlist;
tmpoldlist = tmpoldlist->Next;
}
}
}
// }
return mylist;
}

LinkedList operator+(LinkedList tlist4)
// this should not be a member function ...
// ... but if it is, it should be const ...
// ... and its argument should be const & in either case.
{
LinkedList tlist3;
Node * tmpNode = NULL, *tmpNode1 = NULL;

if(Head != NULL)
{
tmpNode = Head;
do
{
tlist3.AddNode(tmpNode->data);
tmpNode = tmpNode->Next;
}
while(tmpNode != NULL);
}

tmpNode = NULL;

if(tlist4.Head != NULL)
{
tmpNode1 = tlist4.Head;
do
{
tlist3.AddNode(tmpNode1->data);
tmpNode1 = tmpNode1->Next;
}
while(tmpNode1 != NULL);
}

return tlist3;
}

LinkedList() // : Head(0)
{
Head = NULL;
}

~LinkedList()
{
Node * old;
while(Head != NULL)
{
old = Head;
if(Head->Next != NULL)
{
Head = Head->Next;
}
else // unnecessary else
{
break;
}
delete(old); // delete old;
}
delete Head;
}
};

--
Bob Hairgrove
No**********@Home.com

[Bob Hairgrove]

On Sat, 04 Jun 2005 21:46:36 +0200, Bob Hairgrove
<in*****@bigfoot.com> wrote:

// copy ctor's canonical signature is:
// LinkedList & operator=(LinkedList const & oldlist)

Of course, this should read: "assignment operator", not "copy ctor"

--
Bob Hairgrove
No**********@Home.com