Wednesday, August 26, 2009

What's wrong with this code?

Here's some student code for JavaBat caughtSpeeding that shows a tricky coding bug, can you spot it? This student ran the code again and again, and I'm sure they were very frustrated that the code would not do what it should, but I'm hoping we can all learn a little from it.
public int caughtSpeeding(int speed, boolean isBirthday) {
if (isBirthday=false) {
...snip...
}
else if (isBirthday=true) {
...snip...
}
else return 0;
}

The problem is the single "=" inside "if (isBirthday=false)" -- they meant to write a "==" to compare isBirthday to false. Instead the "=" assigns false to isBirthday and then uses false in the comparison. So that first if-statement is never true, regardless of what value was in isBirthday before. For many errors in Java code, you get a compiler error so you know right where you have something to fix. Compile errors are annoying at the time, but they are actually doing you a big favor, making it easy to find and fix simple problems. The tragic and very difficult to debug aspect of this bug is that it gets no compile error and it looks so similar to what the author intended ... it's hard for the eye to notice the little discrepancy. (Java, C, and C++ all treat = vs. == this way, so it's worth learning.)

How to fix 1: it's better to avoid the forms "if (isBirthday==true)" and "if (isBirthday==false)", instead writing them as "if (isBirthday)" or "if (!isBirthday)". This is shorter and more direct, and it happens to avoid this =/== trap which is so hard to debug.

How to fix 2: With the above fix, the code looks like...
public int caughtSpeeding(int speed, boolean isBirthday) {
if (!isBirthday) {
...snip...
}
else if (isBirthday) {
...snip...
}
else return 0;
}

That can work, but it could be improved a little. Notice that the 2 if-statements are exact opposites -- exactly one will run and the other will not. That's a common case, and for it you should just use a single if with a single else which looks like...
public int caughtSpeeding(int speed, boolean isBirthday) {
if (!isBirthday) {
...snip...
}
else {
...snip...
}
}


Incidentally, the technique here of having 2 big cases depending on isBirthday is a fine approach. Another approach which is shorter but trickier is to modify the "speed" variable if isBirthday is true, and then have a series of if/else to figure out the answer -- avoiding having one copy of the code for isBirthday and a second copy for !isBirthday.

2 comments: