What are the advantages of squashing assignment and error checking in one line?

Question

This question is inspired by this question, which features the following code snippet.

int s;
if((s = foo()) == ERROR)
    print_error();

I find this style hard to read and prone to error (as the original question demonstrates -- it was prompted by missing parentheses around the assignment). I would instead write the following, which is actually shorter in terms of characters.

int s = foo();
if(s == ERROR)
    print_error();

This is not the first time I've seen this idiom though, and I'm guessing there are reasons (perhaps historical) for it being so often used. What are those reasons?

Answer 1

I think it's for hysterical reasons, that early compilers were not so smart at optimizing. By putting it on one line as a single expression, it gives the compiler a hint that the same value fetched from foo() can be tested rather than specifically loading the value from s.

I prefer the clarity of your second example, with the assignment and test done later. A modern compiler will have no trouble optimizing this into registers, avoiding unnecessary loads from memory store.

Answer 2

I make a conscious attempt at combining the two whenever possible. The "penalty" in size isn't enough to overcome the advantage in clarity, IMO.

The advantage in clarity comes from one fact: for a function like this, you should always think of calling the function and testing the return value as a single action that cannot be broken into two parts ("atomic", if you will). You should never call such a function without immediately testing its return value.

Separating the two (at all) leads to a much greater likelihood that you'll sometimes skip checking the return value completely. Other times, you'll accidentally insert some code between the call and the test of the return value that actually depends on that function having succeeded. If you always combine it all into a single statement, it (nearly) eliminates any possibility of falling into these traps.

Answer 3

I often prefer the first form. I couldn't say exactly why, but it has something to do with the semantic involved.

The second style feels to me more like 2 separate operations. Call the function and then do something with the result, 2 different things. In the first style it's one logical unit. Call the function, save the temprary result and eventually handle the error case.

I know it's pretty vague and far from being completely rational, so I will use one or the other depending on the importance of the saved variable or the test case.

Answer 4

I would always go for the second. It's easier to read, there's no danger of omitting the parentheses around the assignment and it is easier to step through with a debugger.

Answer 5

I often find the separation of the assignment out into a different line makes debugger watch or "locals" windows behave better vis-a-vis the presence and correct value of "s", at least in non-optimized builds.

It also allows the use of step-over separately on the assignment and test lines (again, in non-optimized builds), which can be helpful if you don't want to go mucking around in disassembly or mixed view.

YMMV per compiler and debugger and for optimized builds, of course.

Answer 6

I personally prefer for assignments and tests to be on different lines. It is less syntactically complicated, less error prone, and more easily understood. It also allows the compiler to give you more precise error/warning locations and makes often makes debugging easier.

It also allows me to more easily do things like:

int rc = function();

DEBUG_PRINT(rc);

if (rc == ERROR) {
    recover_from_error();
} else {
    keep_on_going(rc);
}

I prefer this style so much that in the case of loops I would rather:

while (1) {
    int rc = function();
    if (rc == ERROR) {
        break;
    }
    keep_on_going(rc);
}

than do the assignment in the while conditional. I really don't like for my tests to have side-effects.