The definition of insanity is “doing the same thing over and over again and expecting different results”, so said Albert Einstein, or Mark Twain, or Benjamin Franklin (or someone. Who knows?).

I recently found the definition of insanity myself, in code form, in the codebase of one the mature applications that I work on.

(Some parts have been changed for the purposes of this example. I don’t actually work on an application that feeds pets)

Private Sub FeedPetX(ByVal thePet As Pet)
Try
If thePet = Pet.Cat Then
FeedTheCat()
Else
If thePet = Pet.Cat Then
FeedTheCat()
End If
End If
Catch ex As Exception
LogException(ex)
End Try
End Sub

Luckily, for the uninitiated among us, the programming language that this was written in (VB.NET) is fairly verbose, to the point that I’d expect most non-programmers to at least have a rough idea what is going on. In any case, I’ll try to explain what this code represents in English:

If we are dealing with the cat, then feed the cat. If we are not dealing with the cat, then if we are dealing with the cat, feed the cat.

Insanity, right? Asking the same question a second time and expecting a different answer.

The second “If thePet = Pet.Cat” can never be true. If it was, we wouldn’t have reached that point in the code in the first place. What the code should have been, is this:

Private Sub FeedPetX(ByVal thePet As Pet)
Try
If thePet = Pet.Cat Then
FeedTheCat()
Else
If thePet = Pet.Dog Then
FeedTheDog()
End If
End If
Catch ex As Exception
LogException(ex)
End Try
End Sub

view raw
FeedPetX-correct.vb
hosted with ❤ by GitHub

Or, in English:

If we are dealing with the cat, then feed the cat. If we are not dealing with the cat, but we are dealing with the dog, then feed the dog.

(see this codepen.io pen in order to view and play with a JavaScript equivalent)

How this code, which would have been written years ago by a colleague whom is a better and more experienced developer than I, has managed to lurk unnoticed for so long, was initially quite frightening.

My initial hypothesis was that the routine where I found this problem only gets called in an extremely rare circumstance or group of circumstances. Further inspection of the codebase shows that this was quite correct.

This real routine is only called in a very specific disaster recovery scenario. In actual fact, it may never have actually been executed “in the wild” at all. On top of all that, I would say that it is more likely to have been run in the cat case as opposed to the dog case, and, in any case, a disaster would already have happened, which would have got the blame for any ill behaviour.

Perhaps we failed by not having an approriate unit test (i.e. a kind of automated test that calls the routine independently of the broader codebase, passes different inputs into it, and verifies the output for each). Luckily for us, this doesn’t seem to have caused us a problem.

It also begs another question: are there any more, similar cases left in the codebase? I suppose we don’t know. If there are any, then they do not seem to be affecting our users, but we should learn from this, and at the very least, be more careful in the future.