What do you think when a Boolean "if" has three resulting code paths?

Question

(Background: from a previous job, a co-worker and I would end up discussing the bug pile during lunch. We began to develop a topic called "bug of the week". I doubt I have material for 52 posts a year, but here's the first one...)

Reported by:

QA tester was reading HTML/JS code to write a functional test of a web form, and saw:

if (form_field == empty)
{
...do stuff for empty field
}
else if (form_field != empty)
{
...do stuff for non-empty field
}
else
{
...do stuff that will never be done
}

After a couple embarassing attempts, tester realized that they couldn't trigger the alert strings hidden in the third block.

Things I'm wondering are:

• Is this problem more or less language specific (can non-JS people learn lessons here?)
• Are there legitimate reasons code ended up this way?
• What approaches should be used to find/address the problem (code coverage, code review, blackbox testing, etc.)

A couple other points:

1. I'm hoping we can keep it positive. Imagine this is a person you work with (in a company that does not encourage flaming). Whatever mean things you might think of the guilty party, I probably already thought them too.

2. I'm not a coder by profession. I checked the wiki box, in case someone wants to provide a more literal example, or add to the base of the thread.

[EDIT] Similar to http://stackoverflow.com/questions/114342/what-are-code-smells-what-is-the-best-way-to-correct-them

Answer 1

I don't believe that this problem is at all language specific. You could construct similar (flawed) conditional statements in a wide variety of other languages.

Also, I don't think that there is a legitimate reason for the conditional statement to be structured this way. As you state in the comment, the statements in the third block will just never be done.

You could probably find mistakes like this most effectively with code review. However, since that requires quite a bit of time from at least one developer, you may be better served by developing quality unit tests and inspecting code coverage. In this case, you probably would have noticed that the third portion of the conditional statement was never used.

Answer 2

I've seen cases where the else if used to have something else (AND/OR) there and the person who fixed it just 'fixed it' and didn't probe very deep.

Answer 3

In the general form

if (a)
  //1
else if (!a)
  //2
else
  //3

can always be reduced to

if (a)
  //1
else
  //2

with no side-effects.

Answer 4

I know I remember writing a few like that way back in my first year or two of college, before I knew any better. But I can't see a reason to do it now.

The only way it's even close to legitimate is if either "empty" or "form_field" were a volatile value, similar to VB's Now() function. But in that case, I wouldn't write it this way. Instead, you trap the value once above the if block and test on the trapped value.

Answer 5

static analysis would have told you that the third case is "dead code"

code like this comes from a misunderstanding of Boolean logic, or multiple edits over time by different people - or both ;-)

this is not specific to Javascript, this kind of mistake can be made in any language that has the if-else structure (or a reasonable facsimile thereof)

Answer 6

Although the third block can't trigger in Javascript, that is not true in all languages. In T-SQL:

declare @test as int

set @test = null

if @test = 1 
  print 1
else if not @test = 1
  print 2
else 
  print 3

This will print 3, because NULL is neither equal to, nor not equal to any other value.

Answer 7

• Is this problem more or less language specific (can non-JS people learn lessons here?)

No. You could run into this kind of code in any language with branches and comparisons.

• Are there legitimate reasons code ended up this way?
• What approaches should be used to find/address the problem (code coverage, code review, blackbox testing, etc.)

No, not really. I'd guess that code started out with something other than a Boolean comparison; maybe it was empty, numeric, or non-numeric, or empty, 1-5 chars, or more than 5 chars, something like that. When the logic was changed to empty or non-empty, the third block should have been removed - this would normally be caught in a peer review or something like that.

Depending on the language, some compilers may even catch this. A review would catch it; black-box testing would not, because you wouldn't be checking the code, but white-box testing would (although you should also notice it as soon as you see the code, before any testing is actually done).

Answer 8

Is this problem more or less language specific (can non-JS people learn lessons here?)

This is a language-agnostic problem. It's quite easy to write the following in Java, for example:

if(x)
{
  //do something
}
else if(!x)
{
  //do something else
}
else
{
  //never, ever, do anything
}

The key thing to remember is that the "if(!x)" is not required. Making that a simple "else" would create simpler code.

Are there legitimate reasons code ended up this way?

Sort of. It's standard practice for an else condition to always exist when a fall-through is needed. The problem was the programmer wasn't thinking very clearly single his "(form_field != empty)" was exactly the same as a simple "else". Point it out to him and he should kick himself. If he doesn't, question his role on the team.

What approaches should be used to find/address the problem (code coverage, code review, blackbox testing, etc.)

Static code analysis tools can catch this sort of issue. However, I'm not aware of any for Javascript. JSLint can catch a lot of bad stuff, but not logic flow issues.

Answer 9

This class of problem is easy to spot if you are testing a single condition A, because you know that you can have A and not-A.

It's also not too hard to do if you had conditions A and B. You look for A, not-A and B, not-A and not-B (for example) - and you can fairly easily tell that you've covered all cases.

Sometimes, for whatever reason, the conditions might get refactored out, and then (taking the above contrived example) while leaving the three blocks behind, and the resulting WTF.

But what if you had conditions A, B, C, and D? Stacked if-else's is really a gnarly way of doing it, but sometimes that's just the way it gets done.

Spotting "holes" in the logic coverage is easy to do by creating a complete truth table. A nice way to do this is with Karnaugh maps. The wikipedia entry on Karnaugh maps is a great starting point (with pictures, even!). For software coding, you want full coverage of the map without an overlap (well, at least, usually).

Answer 10

This can happen if you use poorly designed booleans (common in early C when there wasn't a true boolean type). You can still find this stuff in Windows.

BOOL result = SomeWindowsAPI();
if (result == TRUE)
{
    // success
}
else if (result == FALSE)
{
    // failure
}
else
{
    // wtf?
}

The key here is the explicit testing for "TRUE" which assumes that there is one and only one way the boolean can have a true value. When using integers as booleans, this isn't true.

In some languages on some computers (Fortran on VMS), there can be many different integer values that resolve to true and many different values resolve to false. On Windows, the HRESULT when being interpreted as SUCCESS or FAILURE is the same way.

Answer 11

Perhaps not in JavaScript, but any laguage that supports multithreaded programming, and declares form_field in such a way that it is shared between more than one thread could actually see that happen.

For example, Peterson's algorithm contains a similarly useless-looking double check:

     turn = 1;
     while( flag[1] && turn == 1 );

That's protecting against a race condition though. It'd still be damn tough to generate a test to cause it.

If there is no possible race condition then this is the kind of check we used to jokingly call a "cosmic ray check" back when I contracted for NASA. :-)

Answer 12

Different smells show up with different measurements.

I have found Cyclomatic Complexity tools useful. Anything with a complexity over about 5 deserves a closer look.

Answer 13

This happened at our company and I thought it was the funniest thing that the "dead code" in the second else was being executed. In our case it was an untyped pointer (don't ask why we have those) that was incorrectly set and cast to a class that had a Boolean Field.

The "if (A = TRUE) .. else if (A == FALSE) .. else .." was applied to the Boolean field. The resulting asm code was as follows:

cmp al,$01
jnz +$0c
...
test al,al
jnz +$0c
...

clear the value in al was > 1 and it fell through to the else ..

the fact the coder wrote this expecting it to happen is another matter...