CODEWITHSUNDEEP
c++
Doddy
Doddy

Reputation: 1259

Read and write variable in an IF statement

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.

Upvotes: 1

Views: 183

Answers (2)

Klaus
Klaus

Reputation: 25653

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!

Upvotes: 3

Lundin
Lundin

Reputation: 214780

...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");
}

Upvotes: 5

Related Questions