RSS
TwitterClean Code: Nested conditionals
May 12
Ugly code is everywhere. Inside that copy of Microsoft Office 2007, behind that latest Linux kernel, and in the dozens and dozens of web 2.0 MicroISV tools you use day-in and day out. Lurking.
But there are those of us who fight! Who rage against the dying of the light! Those who are more than happy to share our solutions to cleaning up ugly code, in the hopes that up-and-coming programmers will notice and apply said solutions. This… is one such solution.
Take, for example, nested conditions. They look like:
if (thing) {
[CHUNK A]
if (thing) {
[CHUNK B]
if (thing) {
[CHUNK C]
}
}
}
It’s not THAT ugly, but try extending that out into five or more chunks, or if you’ve already indented a couple of times, you will notice just how ugly this looks. Since each chunk is only executed based on the result of a boolean test (the if statements), and each chunk is directly dependant on the previous chunk (and oughn’t be run if the previous chunk fails), we can take advantage of short-circuit operators and rewrite this section of code to be much cleaner.
Instead, rewrite each chunk into its own function, have that function return a boolean, and you can rewrite the original code like:
$success = do_chunk_a();
$success = $success && do_chunk_b();
$success = $success && do_chunk_c();
This is a huge win in terms of readability, and therefore in terms of maintainability. Especially so if you name your new functions descriptively, like “send_invoice()” or “update_client_list()”. It’s far easier to look at the descriptive name of the functions instead of having to read through the entirety of each chunk, just to find out what the code is supposed to do.
May 12, 2009 @ 14:36:14
Uhm… what? That is a huge loss in terms of readability.
Instead of the natural / obvious meaning of “if (test) then (task)”, you’ve got an unusual and unusual-looking abuse of a language feature.
If the code chunks are rather large, then splitting them off into their own functions is a good idea, and they can certainly return a bool to indicate success or failure. This can be used in a compact series of nested ifs that’s quite easy to follow as it all fits on one screen, and appropriate variable names are quite good at self-documentation:
bool success = do_chunk_a();
if (success) {
success = do_chunk_b();
if (success) {
success = do_chunk_c();
}
}
… which is the whole code block, and easy enough to follow since it’s all visible at once.
Or, if there’s no subsequent code in the current function, and you really just dislike the nesting, abort the function upon partial failure:
bool success = do_chunk_a();
if (!success)
return false;
success = do_chunk_b();
if (!success)
return false;
return do_chunk_c();
Or, if you can’t return upon partial failure, but want to do away with the nesting, use a simpler and more explicit way to chain the successes from each call:
bool success = do_chunk_a();
if (success)
success = do_chunk_b();
if (success)
success = do_chunk_c();
Or, if you really want to reduce your code, why not:
bool success = do_chunk_a() && do_chunk_b() && do_chunk_c();
This even avoids the user having to figure out the assignment of success to itself business.
Any of these alternatives are *much* easier to read than your suggestion, IMO.
May 12, 2009 @ 14:53:59
Addendum: I’d generally avoid using lazy evaluation in a chained series of && to do a series of steps with significant side effects. If do_chunk_X() just returns a value and doesn’t modify anything, then chaining is probably acceptable.
if (object->Initialized() && parameter->Initialized())
object->DoSomethingWith(parameter);
In C++ the Initialized functions would often be declared const, and not modify the objects they’re called on. Thus, they are conceptually replaced with a true or false, and nothing is hidden.
If each of the chunks has significant side effects, it’s misleading, to me, to write the code as if it’s just checking if something is true/false, which using the return value directly in a chained && appears to do. Better to put the significant actions on their own line, even if it requires some nested ifs or similar, so it’s obvious that several different significant tasks are being done. You can also then comment to explain each task separately.
May 13, 2009 @ 06:17:49
Your first example implies that B and C will only execute if A succeeds, and C will only execute if A and B succeed, whereas your “solution” will execute all three regardless of the outcome of the others. I would have done this:
$success=true;
if ($success) { $success=A(); }
if ($success) { $success=B(); }
if ($success) { $success=C(); }
Or, because they’re being split up, I’d want to report to the user on these failures. So:
$success=true;
if ($success) { $success=A(); }
if ($success) { $success=B(); } else { errorA(); }
if ($success) { $success=C(); } else { errorB(); }
if (!$success) { errorC(); }
But I think a little nesting would be easier to read then that.
May 13, 2009 @ 07:27:40
Geoff – I use this frequently when I need to have multiple steps in a transaction and if any of them fail, just roll the whole damn thing back. There’s no point in executing any further steps if any of the previous steps fail. I suppose in the future, using actual code examples instead of highly theoretical ones is a good idea.
And I rather like your last suggestion in the first post, chaining them all together on one line – much cleaner.
I hate having these giant pyramids in my code, and my solution is – at least to my eyes – much cleaner. That said, yours is cleaner still.
Avid – check out “short-circuiting”. You’ll find (at least in PHP, where I do the majority of my coding (and I should have made that explicit in the article)) that if do_chunk_a() returns with false, none of the other functions will be evaluated as the expression is already guaranteed to return false.