Friday 13 August 2004 — This is 20 years old. Be careful.
I saw this in a chunk of code recently (the names have been changed to protect the guilty):
if (state == kActive ||
// always allow invitation responses.
isInviteResponse ||
// ..(three-line comment that I couldn't understand)..
( (state == kUninvited || state == kInvited) &&
( opcode == kUninvite ||
( isOutgoing && opcode == kUpdate ) ) ) )
{
// execute the command.
}
Um, right, whatever. Is that condition correct? Who can tell? I can’t make sense of it. I would rewrite it with intermediate booleans like this:
bool bExecuteIt = false;
// Active users can always execute.
if (state == kActive) {
bExecuteIt = true;
}
// Always execute invitation responses.
if (isInviteResponse) {
bExecuteIt = true;
}
bool bWaitingForResponse = (state == kUninvited || state == kInvited);
bool bSendingUpdate = (isOutgoing && opcode == kUpdate);
if ((bWaitingForResponse && (opcode == kUninvite || bSendingUpdate))) {
bExecuteIt = true;
}
// Finally, execute the command! (if we should).
if (bExecuteIt) {
// execute the command.
}
This is still complicated, but at least we’ve managed to pull apart that horrendous boolean expression into some named variables. I’d like to simplify it further so that no boolean expression has both ‘and’ and ‘or’, but that last condition might be as good as it’s going to get.
Comments
I often find myself using the idiom:
doItFlag = False
# complicated loop or set of decisions sets doItFlag True
if doItFlag:
specifically to avoid Huge Nested Ifs. And if I find myself using elif: I immediately re-examine my code to find out what's wrong with it.
b
Ned: One thing you don't mention is that not only is your rewrite much easier to understand but it's a heck of lot easier to debug. One one the uglier aspects of the "if statement from hell" that I mentioned was that debugging it was horrible. It wasn't just testing variables, it was making lots of method calls. Where does the predicate turn false? Let's see: step, step, step, step.... dang I missed it! When the code is broken up it becomes much more transparent both in terms of reading code and stepping through it with a debugger. And it's likely that there's no runtime difference with the generated code. In fact, the new code may actually malke the compiler's life easier.
bool bExecuteIt = false;
// Always execute invitation responses.
if(isInviteResponse) bExecuteIt = true;
else switch(state) //check state
{
// Active users can always execute.
case kActive: bExecuteIt = true; break;
// Invited and Uninvited users depends on the opcode
case kInvited:
case kUninvited:
swtich(opcode)
{
//execute an uninvite
case kUninvite: bExecuteIt = true;
//execute an update if it's outgoing
case kUpdate: bExecuteIt = isOutgoing;
}
break;
default: bExecuteIt = false;
}
Would be nicer if there was no isInviteResponse, or if switch was a simple statement that evaluted to a result.
Sometimes, in the end game, the goal is not beautification, but to fix the problem while making your diffs look minimal.
The time to fix this is in the beginning of a release cycle, where nobody cares about diffs and QA can have a full testing cycles to make sure that it still works with all of the matrixes that it touches. The problem then is that nobody wants to "fix" code that works.
Hence, the Ghetto (pattern) is born...
Add a comment: