468,768 Members | 1,708 Online
Bytes | Developer Community
New Post

Home Posts Topics Members FAQ

Post your question to a community of 468,768 developers. It's quick & easy.

operator overloading and destructor problem

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;
}
};

Jul 23 '05 #1
3 2855
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/
Jul 23 '05 #2
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
Jul 23 '05 #3
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
Jul 23 '05 #4

This discussion thread is closed

Replies have been disabled for this discussion.

Similar topics

6 posts views Thread by - Steve - | last post: by
20 posts views Thread by Ioannis Vranos | last post: by
7 posts views Thread by Eckhard Lehmann | last post: by
3 posts views Thread by md | last post: by
16 posts views Thread by EM.Bateman | last post: by
4 posts views Thread by fabian.lim | last post: by
1 post views Thread by fabian.lim | last post: by
1 post views Thread by CARIGAR | last post: by
reply views Thread by zhoujie | last post: by
By using this site, you agree to our Privacy Policy and Terms of Use.