Conditions from hell

Friday 13 August 2004

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

[gravatar]
Ben Last 3:24 AM on 14 Aug 2004

Hmmm - looks like someone's trying to implement a state machine somehow. I'd assume that kInvited and kUninvited are mutually exclusive and somewhere deep inside I hear the cry of a Boolean trying to replace them :)

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

[gravatar]
Hans Martin Kern 4:47 AM on 14 Aug 2004

Why not transmogrifying these intermediate booleans into functions/methods, thus facilitating re-use of these "partial" conditions?

[gravatar]
Bob 10:33 AM on 14 Aug 2004

Reminds me of some code (much, much worse than this example) that was a veritible predicate minefield. It was testing to see whether something was cacheable or not. The code was a huge if (a || b || c || d || e .....)

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.

[gravatar]
Omer Trajman 1:51 PM on 14 Aug 2004

I'm a sucker for rewriting conditions. This may be more intelligable, and gives the compiler more hints at optimization.

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.

[gravatar]
owen 1:45 AM on 16 Aug 2004

nope; never had a condition like that before. I suspect all the problems started somewhere before the person wrote that line - bad implementation.

[gravatar]
andrew 11:16 AM on 16 Aug 2004

Yes, yes. It looks terrible and yes, there is a lot of purists that are tut-tutting. The reality is that this is what slowly morphing code looks like. It probably started life as a perfectly normal "if (state == kActive)" and each new condition was the result of a featurette or a bug and this was the least-touch way of fixing it.

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...

[gravatar]
Pete 7:40 PM on 16 Aug 2004

I can't agree with Andrew on his point. I' ve never understood this goal of minimizing diffs, but I've certainly seen it preached. To my mind, unless you are at the very, very end game I think it's worth making a limited change like this. One of the greatest sins I think exists in software is the belief you don't have time to make a change. I think the opposite is nearly always true - you don't have the time to not make it.

[gravatar]
Nate Finch 10:51 AM on 21 Apr 2005

I'm way, way late to the game, but since it's my code that's being taken apart, I'll comment regardless. Andrew had it right. It started off simple, got a little more complex, and then a little more. I commented the hell out of it to try to make it more understandable, and probably should have split it out into multiple local variables. However, several of the changes were "just before beta" changes when we were in code freeze, and I didn't want to go rewriting the whole thing on the chance that I'd break something which could take a week to figure out. I hope Ned figured it out and fixed it.

Add a comment:

Ignore this:
Leave this empty:
Name is required. Either email or web are required. Email won't be displayed and I won't spam you. Your web site won't be indexed by search engines.
Don't put anything here:
Leave this empty:
URLs auto-link and some tags are allowed: <a><b><i><p><br><pre>.