Truthy assignments

Question

I often argue with the voices about the correctness of having assignations within an if statement (or equivalent), meaning having this:

if($thing = someFunction()){
   //do stuff
}

Instead of this:

$thing = someFunction();
if($thing){
   //Do stuff
}

A part of me says its a bad practice because:

• If read quickly (by someone who is not that familiar with the practice) it might be hard to notice that its not a ==, and look like a mistake.
• It is not possible in all languages (IIRC, Java explodes with that).

But the voices say its good because:

• Less lines, looks cleaner.
• The practice is not that uncommon (C/C++, PHP).
• The variable acquires more semantics (it generally won't be used outside the if scope)
• It is actually easy to get used to reading it and not miss it the after seeing it a couple of times.

So, could the voices be right?, or, is it an aberrant practice?.

Answer 1

I agree with what @Fge says.

Another way that I've seen:

if (($thing = some_function()) == true) {
   do_something();
}

I'd still argue for the second way you present (assignment before the if statement). I believe it's more readable, since the assignment is separated from the comparison. There are idiomatic exceptions, like in C:

if ((ptr = malloc(sizeof(int) * size)) == null) {
   run_away_screaming();
}

I've seen this a lot, so much so that it's very easy for me to tell if the allocation is wrong. This is a common operation though, and I wouldn't use this method anywhere else.

(
This all assumes that the return value isn't boolean && the return value is used elsewhere. If not, it should be:

if (some_function()) {
   do_something();
}

Just covering the bases :)
)

Answer 2

I prefer the second way of doing it (not having it in the if-clause):

The practice is not that uncommon (C/C++, PHP).

There are a lot of bad practices out there. Just being common does not mean it is good.

The variable acquires more semantics (it generally won't be used outside the if scope)

Scope really should not be a problem. If it is, either your function is way too long or your variables are not named properly.

Less lines, looks cleaner.

Remove all comments from your code - it is even less lines then. Does this mean it is better?

It is actually easy to get used to reading it and not miss it the after seeing it a couple of times.

You already gave an argument against this by yourself:

If read quickly (by someone who is not that familiar with the practice) it might be hard to notice that its not a "==", and look like a mistake.

In my opinion this is the only argument against this practice. But it beats all other arguments by far. For example if someone else reads your code (fast fast-reading over it), he probably will not notice this - even if he is familiar with this. If you code has any chance of being misread, you should avoid it.

But this is my opinion only, there are probably supporters with good arguments for it out there. I guess this is something of personal style/preference.

Answer 3

Here are some serious concerns why assignment inside conditions is bad:

1. The classic == versus = newbie bug. Instead of adopting an obfuscated coding style to prevent this bug, ie if(NULL == var), simply avoid assignment inside conditions.

2. Assignment inside conditions makes it harder for a compiler or static analyser to find actual bugs in your code.

3. Order of evaluation issues. For most operators in C/C++, the order of evaluation is implementation-defined behavior. This means that you cannot know whether the left side or the right side of the = is evaluated first, which may or may not lead to fatal bugs. A typical example of a possibly fatal bug is x=y=0;, which the compiler is free to interpret as "put uninitialized garbage in x, then put 0 in y". Bugs like this are more likely to occur inside conditions.

4. Undefined behavior issues. This is related to the order of evaluation issue above, but even more severe. It occurs when you write code like while(arr[i] < i++). This is undefined behavior and your program is free to crash & burn if your code contains it. http://stackoverflow.com/search?q=i%2B%2B+undefined+behavior

Reason 1 and 2 above are taken from the widely-recogniced industry standard MISRA-C:2004, which bans assignment inside conditions in rule 13.1. Assignment inside conditions is also banned by CERT C, EXP18-C.

Answer 4

The answer really depends on who will be viewing the code. While both methods are correct and acceptable, if you are working with inexperienced/beginner level programmers then it will make everyone's life easier to use:

$thing = someFunction();
if($thing){
   //Do stuff
}

However, if your code will be looked at by medium to experienced programmers then they should have no problem in deciphering:

if($thing = someFunction()){
   //do stuff
}

At the end of the day, what is more critical is the run time of your code. Both techniques give O(1) run times thus either can be used depending on which level programmer will be viewing the code etc.

EDIT: As Lundin points out, it is more appropriate to use the first method. It seems that there are more pros for the first method, thus that should be used.