And then there is writing stuff in a non-standard way for no reason.
When a normal person reads code, they make huge jumps. Reading any non-trivial codebase line-by-line is a non-starter. If they eye-grep something deviating from the standard way of doing it, they try to understand what's the difference in behaviour, and when they can't spot one they have to look again only to remember the code is crap so unjustified deviations are to be expected.
This post is not about random casts to silence the compiler, missing -Wall and the like in compilation flags, missing headers and other junk of the sort.
Let's take a look at weird ass code samples. The list is by no means complete.
1 2 3 | do { ..... } while (1); |
When you want to have an infinite loop you either while (1) or for (;;). Having a do {} while loop in this scenario only serves to confuse the reader for a brief moment.
1 2 | while (foo()); bar(); |
Is the author trying to hide something? It is easy to miss the semicolon on while line and think someone without a real editor just did not indent bar properly. If you notice the semicolon, you start wondering if it ended up there by mistake.
There are legitimate reasons for having loops with empty bodies. If you need to use one, do the following instead:
1 2 3 4 | while (foo()) continue; bar(); |
Next goodie:
1 2 | if (foo || (!foo && bar)) ....... |
What? This is equivalent to mere (foo || bar). Inserting !foo only serves to make the reader read the condition several times while wondering if they had enough coffee this morning.
1 | char *p = '\0'; |
I have not encountered this animal in the wild myself, but got reports of its existence. Reasoning behind the abuse '\0' and 0 (and resulting NULL) equivalency eludes me. Just use NULL. Thank you.
1 2 | if (FOO == bar) ....... |
Famous yoda-style comparisons. Not only they look terrible, there is no legitimate justification that I could see.
There are people who claim it prevents mistakes of the form if (bar = FOO) where comparison was intended. Good news is that your compiler more than likely will tell you that such an expression is fishy and if you really mean it, use additional parenthesis. Which is a good thing since you may need to compare variables, in which case the "trick" would be useless. AVOID.
1 2 3 4 5 6 7 8 | if (foo) { bar(); return; } else { baz(); } quux(); |
Or even worse:
1 2 3 4 5 6 7 8 | if (!foo) { baz(); } else { bar(); return; } quux(); |
What's the point of else clause? Do this instead:
1 2 3 4 5 6 7 | if (foo) { bar(); return; } baz(); quux(); |
Annoyingly I can't use a HTML pre tag to markup code when commenting. So you get no indentation. Sorry about that.
ReplyDeleteFrom our code at work:
f(...) {
do {
x = trysomething();
if (!x) {
break;
}
y = trysomethingelse();
if (!y) {
break;
}
dosomething(x, y);
return success;
} while(0);
cleanup(x);
cleanup(y);
return failure;
}
The author here is using a do ... while (0) idiom so that he can make use of break to drop out to the cleanup code if things go wrong. Brownie points for Duffing with the conditionals there, but bad for everything else!
I don't know about you, but when reading code, if I see a do {, I expect it'll be a loop. This is a surprise when I get to the bottom and find } while(0);, which jolts against expectations. Here's the same thing after a cleanup:
f(...) {
x = trysomething();
if (!x) {
goto cleanup;
}
y = trysomethingelse();
if (!y) {
goto cleanup;
}
dosomething(x, y);
return success;
cleanup:
cleanup(x);
cleanup(y);
return failure;
}
Doesn't it make more sense? The label name means there's no surprise at what's going on; I don't need to read the end in order to figure out what `goto cleanup;` is going to do.