Is this C++ assignment operator code incorrect?

Question

0.00/5 (No votes)

See more:

I am reading a book and it includes code for an assignment operator and I'm pretty sure it is incorrect but I also think maybe I am just wrong because it is unlikely the book will make a mistake. Here is the code:

C++

LinkedList& LinkedList::operator= (const LinkedList& other)
{
// make sure we aren't assigning to ourself--we can just ignore
// that if it happens. Notice that we're using 'this' here to ensure
// that the other value isn't the same address our as object
if ( this == & other )
{
// return this object to keep the chain of assignments alive
return *this;
}
// before copying over the new values, we need to free the old memory
// since it's no longer used
delete _p_head;
_p_head = NULL;
LinkedListNode *p_itr = other._p_head;
while ( p_itr != NULL )
{
insert( p_itr->val );
}
}

The problem is, he never increments p_itr so p_itr is just always pointing to the head of the list. Is the code incorrect?

Also a separate question I have is, as this assignment operator is working with references to the variables, why does it need to return anything and where are the return values going?

And last question unrelated is here is some code that defines the operator= but there is not type given before the function:

operator= (const Player& other);

this is code inside of the private part of the structure. Why does it not have a type?

What I have tried:

Nothing............................................................

Posted 30-Jun-18 2:08am

BerthaDusStuf

Updated 4-Jul-18 11:13am

v6

Add a Solution

Comments

[no name] 1-Jul-18 11:22am

This is not a copy constructor, this is the assignment operator. The copy constructor has the signature class_name ( const class_name & )

BerthaDusStuf 4-Jul-18 17:09pm

Oh ok sorry I didnt realise, I mean assignment operator definition

2 solutions

Solution 1

In a copy contructor you must create a FULL COPY for the new object. So you must copy all member element in a unique matter for the constructed object.

When copying a linked list, you must create a copy each element and put it a new list.

you need:

C++

Player& operator= (const Player& other);

See this operator tutorial.

Posted 30-Jun-18 23:01pm

KarstenK

Comments

BerthaDusStuf 4-Jul-18 17:10pm

Ok thanks

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

«_Superman_» · Accepted Answer · 2018-07-01T18:43:00

Solution 2

This is not a copy constructor.
This is an assignment operator.

Copy constructor signature would be something like LinkedList(const LinkedList& rhs);
Remember constructors don't return anything.

Quote:
is working with references to the variables, why does it need to return anything and where are the return values going?

So that you could do something like list1 = list2 = list3 = list4 = ...

Quote:
he never increments p_itr

You probably need to look inside the insert method to figure out if anything happens there.

Quote:
operator= (const Player& other);

Not correct. In fact this is the signature for a copy constructor like Player(const Player& other);

Posted 1-Jul-18 18:43pm

«_Superman_»

Comments

BerthaDusStuf 4-Jul-18 17:39pm

"So that you could do something like list1 = list2 = list3 = list4 = ..."

Is this because it is like writing:
list1.operator=(list2.operator=(list3.operator=(list4)))

"You probably need to look inside the insert method to figure out if anything happens there"

But the insert method doesnt take p_itr as a parameter so it can not modify it, it takes a value pointed to by p_itr. This could just be a mistake in the book but I just didn't want to assume so before checking with people who know better.

"Not correct. In fact this is the signature for a copy constructor like Player(const Player& other);"

Sorry I meant assignment operator not copy constructor. Is operator=(const Player& other); a valid definition for a copy constructor because there is no type given.

«_Superman_» 4-Jul-18 22:36pm

>> Is this because it is like writing:
>> list1.operator=(list2.operator=(list3.operator=(list4)))
That is correct.

>>But the insert method doesnt take p_itr as a parameter so it can not modify it, it takes a value pointed to by p_itr.
I was referring to what could be done with pre-processor macros like in this link - The OFFSETOF() macro - GeeksforGeeks[^]
But it probably does not apply here. You could try and debug the code to check what is actually happening.

>>Is operator=(const Player& other); a valid definition
In C++ it is not. In C, int is assumed as the return type if nothing is specified.
However this defeats the purpose since you're trying to make a copy of the object, unless you have a single integer argument conversion constructor.

BerthaDusStuf 5-Jul-18 4:40am

Ok thanks, very easy to understand and well structured answer