Quick question, what is wrong with the following code?
bool result = false;
if (test1) {
if (subtest1) {
DoSomething();
} else
DoSomethingElse();
result = FinalMethod();
} else if (test2) {
result = GetSecondResult();
}
return result;
Stumped? What if I told you that FinalMethod should only be called if “subtest1” is false?
I have changed the names and circumstances to protect the innocent, but the facts of the error remains the same as one i just encountered in a code review. Of course, even knowing that little tidbit of information, it still took me a couple of minutes to see the error (Fortunately my spidey sense was tingling, so I knew *something* was wrong…) The first time I read the code, this is what I saw (I reformatted to make the misperception clear, but the formatting was in reality the same as above):
bool result = false;
if (test1) {
if (subtest1) {
DoSomething();
} else {
DoSomethingElse();
result = FinalMethod();
} else if (test2) {
result = GetSecondResult();
}
}
return result;
Of course, that is a nonsensical statement, but that is how my brain interpreted the block. Since the code was missing the brackets around the else statement, my brain simply supplied the expected syntax, regardless of its validity. Given that the error was an introduced error, it is safe to assume the original developer’s brain did much the same thing, since there was even a comment to the effect that if subtest1 failed it would return the result of FinalMethod().
So how can we prevent these types of errors? If our brains are actively working against us, then we need to work to give our brain what it wants:
bool result = false;
if (test1) {
if (subtest1) {
DoSomething();
} else {
DoSomethingElse();
result = FinalMethod();
}
} else if (test2) {
result = GetSecondResult();
}
return result;
Now there is no need for our brain to supply the missing brackets, since they are explicit. In addition, if we had originally used those brackets, it would have been immediately clear to anyone reading the code that the call to FinalMethod was occurring outside the else block, making the error much easier to detect, and much more unlikely to enter the codebase.