Is it bad to do Inner assignments?

Question

We were having this discussion wiht my colleagues about Inner assignments such as:

return result = myObject.doSomething();

or

if ( null == (point = field.getPoint()) )

Are these acceptable or should they be replaced by the following and why?

int result = myObject.doSomething();
return result;

or

Point point = field.getPoint();
if ( null == point)

Solution 1

The inner assignment is harder to read and easier to miss. In a complex condition it can even be missed, and can cause error.

Eg. this will be a hard to find error, if the condition evaluation prevent to assign a value to the variable:

if (i == 2 && null == (point = field.getPoint())) ...

If i == 2 is false, the point variable will not have value later on.

Solution 2

if ( null == (point = field.getPoint()) )

Pros:

• One less line of code

Cons:

• Less readable.
• Doesn't restrict point's scope to the statement and its code block.
• Doesn't offer any performance improvements as far as I am aware
• Might not always be executed (when there is a condition preceding it that evaluates to false.

Cons outweigh pros 4 / 1 so I would avoid it.

Solution 3

This is mainly concerned with readablity of the code. Avoid inner assignments to make your code readable as you will not get any improvements with inner assignments

Solution 4

Functionally Not Necessarily. For Readability Definitely Yes

Solution 5

They should be avoided. Reducing the number of identifiers/operations per line will increase readability and improve internal code quality. Here's an interesting study on the topic: http://dl.acm.org/citation.cfm?id=1390647

So bottom line, splitting up

return result = myObject.doSomething();

into

result = myObject.doSomething();
return result;

will make it easier for others to understand and work with your code. At the same time, it wouldn't be the end of the world if there were a couple inner assignments sprinkled throughout your code base, so long as they're easily understandable within their context.

View more solutions

Comments

[Matzi]

Example attached, the point variable may not contain value if the condition is failed prematurely.

[Adrian Shum]

&& and || is short-circuit evaluation, that means if i != 2, point=field.getPoint() will not be evaluated

[fd8s0]

I don't think this is a valid argument against inner assignments... if you write things badly that's not its fault. When considering if statements I agree it's not good, but in while statements, not using an inner assignment can lead to writing the same assignment twice, which is also not great.

[Matzi]

The same stands for goto. It can be used right and can be used wrong, but generally considered a bad practise. It's not that extreme in this case, but better to be careful when using. I just highlighted the dangers.

[Line]

But why not use just return myObject.doSomething(); in that case?

[fedenusy]

Yep, that's usually better. I would only do intermediate assignment when dealing with long / complex method chains. In those cases, the variable name can serve as documentation.

[Line]

I like that way once, but Sonar complains about it - that's where I have this idea that is bad from.

[Haphil]

Not saying that inner assignments are good, but regarding your answer I have to state the following: 1. if the inner assignment is the first operand, it also improves readability if ( (point = field.getPoint()) == null ) I want to say with that: your example decreases the readability even more. 2. regarding "doesn't restrict point's scope" --> that's what an inner assignment is supposed to do, so you could also write that as a pro and remove it from cons :)