Posts Tagged ‘the bad parts’

indexOf in if statements is dangerous

Saturday, April 23rd, 2011

I was recently cleaning up some legacy code, looking for an intermittent bug and I encountered the following code:

    var data = "someData";
    if(data.indexOf("e")) {
        //do something here

Now, in 99.9% of cases the string was the same so the “e” was present and the code entered into the condition and did the right thing.

However, in the remaining 0.1% the code looked like this:

    var data = "enterSomeData";
    if(data.indexOf("e")) {
        //do something here

This scenario would fail, but it’s not at first glance obvious why. Until you remember what indexOf does and what in javascript is a falsey value. Consider:

if ("foo".indexOf("z")) // returns -1 which evals to true
if ("foo".indexOf("f")) // returns 0 which evals to false
if ("foo".indexOf("o")) // returns 1 which evals to true

The bug was primarily hiding because there were never any cases where the “e” was not present at all, and so arguably, you could remove the whole if cause as it’s unnecessary. However, the person who wrote the code must have assumed that indexOf returns a falsey value if the “e” is not present and a truthy value if it is, a not so unimaginable leap and indeed, the same mistake that I first made when I looked at the code.

So for my book, that’s what Douglas Crockford would label as a bad part of the language, a part with the potential to trip you up if your not paying attention. Ideally something like JSLint would check to make sure you are comparing the return of indexOf with either > 0 or !== -1.