Why You Should Always Use === and Other Bad Practices

In my day job I work on a fairly large sized JavaScript coded base, about 40,000 lines of legacy code. Some days, when I'm particularly bored, I fire up jsHint and try to reduce the scary number of warnings the static analysis tool reports. One such day, I stumbled on a bit of code that looked roughly like the following:

if(attributeArray instanceof Array) {  
    for(var index in attributeArray) {
        if(index == 0) {
            // do something with attributeArray[index]
            console.log(attributeArray[index])
        } else {
            // do something else with attributeArray[index]
            console.log(attributeArray[index]);
        }
    }
}

Do you see the issue?

Well, I didn't and quickly changed the == to === and then made about 100 other minor cleanup changes to a bunch of different files.

A few hours later, while I was using a new feature our application, everything went wrong. Luckily, there was a nice stack trace and I quickly went through the list of recent commits to see if any of the files in the stack trace were affected. None were.

Oops.

At this point I started rolling back commits until I got to the one where I made the small change above. It turned out the above code was being used as a mixin, and that's why it wasn't listed in the stack trace.

If you don't see an issue with the above code, you may be alarmed to find there is not just one issue, but three bad practices all rolled into this small snippet. The first issue, the one issue I blindly fixed, is that, we as JavaScript developers, should strive to use strict equality when ever possible, and especially when comparing against things like 0,1, null, undefined, true, and false. Simply using ==, as you would in most other languages, indicates to JavaScript interpreters that you are okay with type conversion occurring before the comparison is made. Most of the time this is not what you want, With type conversion, all of the following expressions are true:

0 == ''  
'\n' == 0  
null == undefined  
false == '0'  
'0' == 0  

If, instead of using ==, you used the strict equality operator, all the above expressions would evaluate to false. This is, more than likely, what you expect. Unfortunately for me, changing the last expression to evaluate to false broke my application.

Why? Well, the for-in loop iterates over object properties. Since array is an object, its properties can also be iterated over, and that is exactly what the for-in loop is doing. Since object properties in JavaScript are strings, the for-in loop is simply assigning the property names to the index variable as a string.

So index takes the values “0”, “1”, “2”, “3”, etc but the original developer was comparing it simply to 0, type conversion was taking place and the expression '0' == 0 was true due to type conversion. Simple.

But is this a good idea to rely on type conversion as we do in the block of code shared?

Of course not. It's far too trusting. In a large application, with many external libraries, it's a bad idea to trust all the code not do modify the Array prototype. You might be clever and suggest using hasOwnProperty() but simply doing this is also far too trusting. Any developer can easily modify the array itself and add properties:

// The following could cause issue with the above snippet
Array.prototype.seven = 7;  
// or
attributeArray.f = function() {};  

But there is another reason not to use for-in, and it is probably the final nail in the coffin that the above production code snippet needs to be entirely rewritten.

for-in loops are simply slow. In Chrome version 32, using a for-in is roughly 20 times slower than a traditional for loop that iterates. But don't just take my word for it. We can use jsPerf to measure the performance differences. See this jsPerf for a test that compares for-in to for loops.

Chrom for-in vs for loop results

95% slower is a tremendous slowdown!

Given these speed differences, the issue iterating over array properties, and the idea that we should avoid type conversion if possible, there is absolutely no reason our production code snippet should stay as is. Instead we can easily solve all these issues by rewriting it using a traditional for loop:

if(attributeArray instanceof Array) {  
    for(var index=0, len=attributeArray.length; index < len; index++) {
        if(index === 0) {
            // do something with attributeArray[index]
            console.log(attributeArray[index]);
        } else {
            // do something else with attributeArray[index]
            console.log(attributeArray[index]);
        }
    }
}

In JavaScript, it often pays to keep it simple.