5
votes
\$\begingroup\$

I often argue with the voices about the correctness of having assignments 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?

\$\endgroup\$

locked by rolfl Jul 1 '15 at 2:41

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • \$\begingroup\$ Belongs on Programmers.SE \$\endgroup\$ – doppelgreener Jul 17 '11 at 0:43
3
votes
\$\begingroup\$

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 :)
)

\$\endgroup\$
  • \$\begingroup\$ Yep, that was just the alternative I was looking for. It is kind of easy to say "Oh, I'll simplify that; who uses x == true anyways?", so, putting in coding standards =). \$\endgroup\$ – dukeofgaming Jun 27 '11 at 16:52
2
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This is most definitely not just a simple style issue, see the answer I posted. \$\endgroup\$ – Lundin Jul 8 '11 at 13:24
2
votes
\$\begingroup\$

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. https://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.

\$\endgroup\$
  • \$\begingroup\$ About 1) In my opinion the NULL == var statement has another purpose then I think you mean: I use it to force compiler errors when i accidentally forgot the second = (this happens more often then you might think) - not to make clear this should be an assignment. NULL = var gives you an error at write-time (*by your IDE) while var = NULL will even compile. Thus this is not a obfuscated coding style in my opinion ;) \$\endgroup\$ – Fge Jul 8 '11 at 15:00
  • \$\begingroup\$ @Fge That was true in the 80s when that style was popular for that reason. Since around 1991 almost every known compiler warns for assignment inside conditions. And in modern programming, static analysers are used, making that style even more obsolete. \$\endgroup\$ – Lundin Jul 8 '11 at 17:45
  • \$\begingroup\$ Not every language has highly optimized compilers, analyzers or IDEs (e.g. many script-languages, but then again, the null = var assignment does even work sometimes). However, I guess that's pretty much a personal taste :) \$\endgroup\$ – Fge Jul 8 '11 at 19:12
0
votes
\$\begingroup\$

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.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.