Short-circuiting and readability

Question

In this line

if ((last_search == NULL) || (last_search != NULL && total_results != 0))

I know that C's short-circuit evaluation rules say that only if last_search is not null will it try and evaluate the right-hand side of ||, hence it's equivalent to writing

if ((last_search == NULL) || (total_results != 0))

and I was advised to use the later by someone, but still isn't the former more readable? Also won't the compiler optimize out the redundant last_search != NULL?

Side note : I'd rather move constant to left, `(NULL == last_search)...`, — a1ex07, Sep 17 '12 at 15:36
@a1ex07 I know it's just personal preference (and I know I've seen a trend toward this lately), but to me putting the constant on the left makes it harder to read. I read that as "if NULL is equal to ...", which is just wrong, because NULL is a constant and doesn't change. "if last_search is equal to ..." is a much more natural and understandable reading to me. — twalberg, Sep 17 '12 at 15:41
Things that are obvious should not be mentioned..and so the first case, in a way, just has some redundancy. — akaHuman, Sep 17 '12 at 15:44
@a1ex07 I think moving constants to the left is a bad advice. You are testing if `last_search` value is `NULL` and not if `NULL` is `last_search` value. — ouah, Sep 17 '12 at 15:44
@twalberg: having `val==NULL` in conditions is a way more error-prone (`val=NULL` is absolutely valid -opposite to `NULL=val`- but means different)... — a1ex07, Sep 17 '12 at 15:48
@a1ex07 Yeah, I get why it's being done. I just think it reduces readability. — twalberg, Sep 17 '12 at 16:26
Related: http://stackoverflow.com/questions/628526/is-short-circuiting-boolean-operators-mandated-in-c-c-and-evaluation-order — legends2k, Sep 17 '12 at 19:10

score 10 · Accepted Answer · answered Sep 17 '12 at 15:32

This is subjective, but no, the first variant is not more readable because there's more to read. The most readable code (and the least buggy!) is that which does not exist. Just think what would happen if you wanted to check 3 or 4 conditions at once.

And here's another counterexample: would you write code like this?

if (number < 0) {
}
else if(number >= 0) {
    // why not just "else"?
}

As for performance: the compiler would probably optimize the redundant call away, but undoing the performance degradation does not help with the readability degradation.

+1. If I want to emphasize the `number>=0` (e.g. after a very long if-block), I move the if-part of the elseif into a comment. — Bergi, Sep 17 '12 at 15:43

score 5 · Answer 2 · answered Sep 17 '12 at 15:32

5

No it's not. The second one is a lot more readable. Why would you keep the redundant check in there, regardless of whether the compiler will optimize it away?

The first version tells you that last_search is not NULL because it got there, but if you can't tell that from the first condition (last_search == NULL failed), you've probably got bigger issues than readability.

answered Sep 17 '12 at 15:32

Luchian Grigore

253,575
64
457
625

+1 for you because this answer is just as good as Jon's yet you got none of the upvotes because he has more rep... – Sep 17 '12 at 15:34
1

@H2CO3 to be fair, I too upvoted him for "The most readable code is that which does not exist.". :) – Luchian Grigore Sep 17 '12 at 15:35
@H2CO3: Wait till you see what happens when you answer a question and Jon Skeet does the same... btw, I also upvoted this because Luchian was first. – Jon Sep 17 '12 at 15:39
1

@Jon been there. Fortunately, it doesn't happen too often, I rarely follow Java (and C# is far from my thing)... – Luchian Grigore Sep 17 '12 at 15:42
@Jon yeah, it's just the same with Jon Skeet. Everybody upvotes him just because he's Jon Skeet. And I wouldn't mind it if it was because he had the best answer. Of course, in a big part of the cases, he has the best answer, but when not, then why? – Sep 17 '12 at 15:43
@LuchianGrigore +1 for "C# is far from my thing". I don't understand how a Microsoft (non-)technology can be the leading topic on SO. Oh wait, SO itself is written in ASP? Well played, tech world... – Sep 17 '12 at 15:44

score 1 · Answer 3 · answered Sep 17 '12 at 15:37

1

It is not only the short circuit evaluation that makes it unreadable but also the fact of using superfluous comparisons.

if ( !last_search || total_results)

is much easier to read than anything that you proposed.

answered Sep 17 '12 at 15:37

Jens Gustedt

76,821
6
102
177

The problem with this is what if the value of `NULL` isn't `false`? `NULL` is just a constant (i hope it is, anyway - if it isn't, there's more problems going on here), and as such, may actually correspond to something that won't evaluate as false. Use "if ((last_search == NULL) || total_results)" – AJMansfield Sep 17 '12 at 15:47
@AJMansfield `NULL` will always evaluate as false. Even if its underlying representation is not all zeros. – Dave Sep 17 '12 at 15:49
conversion of pointer values to obtain a logical condition is well defined by the standard. It does exactly what you would expect: it is `false` when the pointer is a null pointer. Just use the tools that the standard offers. – Jens Gustedt Sep 17 '12 at 15:51
@Dave Isn't `NULL` here a compiler constant? What if its being used as a placeholder (e.g. `-1`)? Although I agree that if it is, there are more problems with the code than just this. – AJMansfield Sep 17 '12 at 15:51
Ok thanks, I'm just not very familiar with C. Thanks for clarifying. – AJMansfield Sep 17 '12 at 15:53
@AJMansfield, ok, no problem. But could you please avoid editing an answer for something your are not very familiar with? – Jens Gustedt Sep 17 '12 at 15:55
@Jens Gustedt Sorry my browser was having some trouble, I thought it was writing a comment, and I couldn't figure out how to delete the edit. – AJMansfield Sep 17 '12 at 15:56
@AJMansfield, ah, ok, that explains it. – Jens Gustedt Sep 17 '12 at 15:58
IMHO, that is is horrible and does not explain what you are doing. == NULL and != 0 make it extremely clear what is going on – Tom Tanner Sep 17 '12 at 16:22
@TomTanner, I think this is just a matter of habits, nothing else. And occasionally it is exactly the way it is foreseen by the C standard since the beginning. Here it makes even one thing more apparent than in the other versions: in this case the condition is completely symmetric, there is no reason that one of the conditions has to be evaluated before the other. – Jens Gustedt Sep 17 '12 at 16:36
I'm not objecting to ||. It's the lack of equality testing. Whether or not short circuiting is good for parallel evaluation is a different question entirely – Tom Tanner Sep 18 '12 at 10:53

Short-circuiting and readability

3 Answers3