Read and write variable in an IF statement

Question

I'm hoping to perform the following steps in a single IF statement to save on code writing:

If ret is TRUE, set ret to the result of function lookup(). If ret is now FALSE, print error message.

The code I've written to do this is as follows:

BOOLEAN ret = TRUE;

// ... functions assigning to `ret`

if ( ret && !(ret = lookup()) )
{
    fprintf(stderr, "Error in lookup()\n");
}

I've got a feeling that this isn't as simple as it looks. Reading from, assigning to and reading again from the same variable in an IF statement. As far as I'm aware, the compiler will always split statements like this up into their constituent operations according to precedence and evaluates conjuncts one at a time, failing immediately when evaluating an operand to false rather than evaluating them all. If so, then I expect the code to follow the steps I wrote above.

I've used assignments in IF statements a lot and I know they work, but not with another read beforehand.

Is there any reason why this isn't good code? Personally, I think it's easy to read and the meaning is clear, I'm just concerned about the compiler maybe not producing the equivalent logic for whatever reason. Perhaps compiler vendor disparities, optimisations or platform dependencies could be an issue, though I doubt this.

Your net savings when trying to save time while writing code are negative. Saving time writing code costs time reading code. Ten-fold. — IInspectable, Jun 01 '15 at 11:48
It will work as you described, though it is a bit terse and therefore lacks readability. — Cory Kramer, Jun 01 '15 at 11:48
@IInspectable in this case there is no time saving because he had to put additional effort to write this code, compared to a dumb, more verbose one — UmNyobe, Jun 01 '15 at 11:50
If you are using C++ why are you using BOOLEAN and TRUE instead of bool and true? — kfsone, Jun 01 '15 at 11:52
So the meaning of this code is clear to you, you're just not sure what exactly it does? At least you're not already aware it is considered bad practice. Oh, wait, you are... — kfsone, Jun 01 '15 at 12:26

score 5 · Accepted Answer · edited May 23 '17 at 12:30

...to save on code writing This is almost never a valid argument. Don't do this. Particularly, don't obfuscate your code into a buggy, unreadable mess to "save typing". That is very bad programming.

I've got a feeling that this isn't as simple as it looks. Reading from, assigning to and reading again from the same variable in an IF statement.

Correct. It has little to do with the if statement in itself though, and everything to do with the operators involved.

As far as I'm aware, the compiler will always split statements like this up into their constituent operations according to precedence and evaluates conjuncts one at a time

Well, yes... but there is operator precedence and there is order of evaluation of subexpressions, they are different things. To make things even more complicated, there are sequence points.

If you don't know the difference between operator precedence and order of evaluation, or if you don't know what sequence points are, you need to instantly stop stuffing as many operators as you can into a single line, because in that case, you are going to write horrible bugs all over the place.

In your specific case, you get away with the bad programming just because as a special case, there happens to be a sequence point between the left and right evaluation of the && operator. Had you written some similar mess with a different operator, for example ret + !(ret = lookup(), your code would have undefined behavior. A bug which will take hours, days or weeks to find. Well, at least you saved 10 seconds of typing!

Also, in both C and C++ use the standard bool type and not some home-brewed version.

You need to correct your code into something more readable and safe:

bool ret = true;

if(ret)
{
  ret = lookup();
}

if(!ret)
{
  fprintf(stderr, "Error in lookup()\n");
}

yes, I agree, not to write unreadable code "just to save typing". In above case, I would prefer if{...}else{...} — Learner, Jun 01 '15 at 12:18
@Learner When you mangle out the previously obfuscated code into this, you might realize that the algorithm itself might not make sense. I don't know what this code is supposed to do, but it would seem that 2 different bool variables would be preferred here. If you write if-else with the present code, you would have to write the same error handling code in two different places. — Lundin, Jun 01 '15 at 12:24
@Lundin-why cant you improve the answer to else{....} instead of if(!ret) , since ret is not compared against anything? — AlphaGoku, Nov 09 '16 at 05:34

Klaus · Answer 2 · 2015-06-01T12:01:35.667

3

Is there any reason why this isn't good code?

Yes, there are a lot issues whith such dirty code fragments!

1) Nobody can read it and it is not maintainable. A lot of coding guidlines contain a rule which tells you: "One statement per line".

2) If you combine multiple expressions in one if statement, only the first statements will be executed until the expression is defined! This means: if you have multiple expressions which combined with AND the first expression which generates false will be the last one which will be executed. Same with OR combinations: The first one which evaluates to true is the last one which is executed.You already wrote this and you! know this, but this is a bit of tricky programming. If all your colleges write code that way, it is maybe ok, but as I know, my colleagues will not understand what you are doing in the first step!

3) You should never compare and assign in one statement. It is simply ugly!

4) if YOU! already think about " I'm just concerned about the compiler maybe not producing the equivalent logic" you should think again why you are not sure what you are doing! I believe that everybody who must work with such a dirty code will think again on such combinations.

Hint: Don't do that! Never!

edited Jun 01 '15 at 12:01

answered Jun 01 '15 at 11:53

Klaus

24,205
7
58
113

Although I agree with you in general, there is much emotion and only few facts here. – TobiMcNamobi Jun 01 '15 at 12:13
@TobiMcNamobi These are actually not subjective things. When a lot of coding standards and authority figures all label something as bad practice, then it is. Assignment in expressions is a perfect example. Already back in 1990, Turbo C had a static check warning you against "possible incorrect assignment" when you attempted it. Before that, in the 80s, there were various weed-smokers recommending to use [yoda conditions](http://en.wikipedia.org/wiki/Yoda_conditions). And here we are some 30 years later and people still write assignments inside conditions. – Lundin Jun 01 '15 at 12:30
@Lundin I agree with you and I agree with Klaus. Don't get me wrong but coding standards and authority figures may be quoted. A quote is a fact. Exclamation marks and UPPERCASE words are emotions. – TobiMcNamobi Jun 01 '15 at 13:24
@TobiMcNamobi Indeed. So Klaus feel free to quote [CERT C++ EXP45-C](https://www.securecoding.cert.org/confluence/display/c/EXP45-C.+Do+not+perform+assignments+in+selection+statements) or MISRA-C:2012 13.4, or "C Traps and Pitfalls"/Andrew Koenig 1.1, and so on. – Lundin Jun 01 '15 at 13:35
1

I agree that my answer is also a personal statement of what I personally like or dislike. But I believe that there is a majority of people who agree with most of the coding guidelines. In fact is the question "is this good or bad code" always a matter of taste and this makes the question maybe off topic because the answer is "opinion based" :-) But on the other hand I have never seen a guideline which states "side effect programming" as helpful. At last it is important to know for which audience the code is written. And always think on the next years where you need to find the bugs :-) – Klaus Jun 01 '15 at 19:10

Read and write variable in an IF statement

2 Answers2