Case Studies for Common Pitfalls That JavaScript Developers Should Know

Recently, I saw an awful comic about the coding pitfall.

from reddit

After some laugh, I regretted not having shared our experiences and practices about coding pitfall. The scientists would have been alive if they knew this rule, RIP.

Okay, let’s get down to business.

As can be seen from the GitHub statistics recently announced, JavaScript is very popular. However, because of its dynamic nature, JavaScript is more challenging to manage its code quality as codebases become larger.

With this concern, I have developed a JavaScript static analyzer (DeepScan) that finds potentially error-prone code without executing the code. Instead of detecting mere code convention violations, it tries to find ECMAScript exceptions and common mistakes developers make.

For benchmarking the analyzer, we have applied it to in-house projects and also to various open source projects in GitHub. Although the analyzed sources are committed ones after local development, there are still some errors not caught by human developers and testing.

In this article, I’ll share common pitfalls found in the benchmark. I hope this article will give you some insight on what you consider in JavaScript development.

1. Misuse of typeof operator

Possible return values of typeof operator are the following strings:

• “undefined”
• “object”
• “boolean”
• “number”
• “string”
• “symbol”
• “function”

Unlike the right operand of instanceof in Java, it’s not arbitrary. When a developer compares with the value not listed above, it will always result in false undesirably.

I found some developers seem to use typeof operator to check if an object is an array type.

var param = typeof pChangeMessageList == "Array" ? pChangeMessageList.join('|') : pChangeMessageList;

Isn’t it natural?

But, “Array” (or “array”) is not a valid return value of typeof and pChangeMessageList will not be joined even when it is really an array. Developers should use Array.isArray() or utility functions in jQuery, lodash, ….

Also, because the return value is a string, typo is common.

In Apache Ambari, let’s see a typo udefined: app/controllers/main/host/details.js

In other projects, I also could see typo like undeifned.

if (typeof Object.prototype[k] != "undeifned" && Object.prototype[k] === this._table[k]) continue;

Pleasantly, I found above problems were fixed by the refactoring commit like this.

Note that typeof operator returns a predefined string only.

2. Use of undeclared variables

Let’s look at the example first. (Our in-house code)

var rows = [];
for (var i = 0; i < args.length; i++) {
    var obj = {
        patternId: patternId
    };
    rows.push(obj);
}
var patternId = $("#pattern").attr("patternId");
$.post('/delete.do', { data: JSON.stringify(rows) });

Since patternId variable is used before its declaration and assignment, obj has a wrong value.

In Java, the compiler will grumble about this “Unresolved variable” case. But the lifecycle of JavaScript variables allows above prior-usage pattern because patternId variable is hoisted and initialized as undefined immediately when the surrounding function is executed.

Just use let. It will throw ReferenceError(“patternId is not defined”) and you cannot send erroneous data to the server at least.

I recommend you read this article about var hoisting. Frankly, I did not know exactly the difference between var and let.

For your information, this is why some JavaScript coding conventions say a developer should declare all the variables in the beginning of function body.

3. Unused and Uninitialized variables

Unused variables can cause a problem. Below in-house code was appending userName parameter and trimming special characters.

if ($.browser.msie == true) {
    var target = url + "userName" + userName;
    target = url.replace(/\.|\?|\&|\/|\=|\:|\-|\s/gi,"");
}

But target variable appended with userName is not used afterwards, the final value of target variable does not include the parameter.

I’ll bet the developer intended to write target = target.replace but he made a mistake. To avoid this kind of mistake, double check when you are not using a variable that is assigned a value.

Unused variable case like the above is not specific to JavaScript, but the following uninitialized case is so.

In Modernizr, you can see an incorrect use of uninitialized variable html5: /src/html5shiv.js

You can see html5 variable declaration at line 7. It’s here because a developer wanted “Take the html5 variable out of the html5shiv scope so we can return it”.

But, 2 years later, another developer made a commit adding var at line 23. As a result, html5 variable returned at line 38 has undefined value.

If you are Java developer, the compiler will complain this “Uninitialized variable” but JavaScript developer can’t get such help. That’s why I developed this rule for checking the use of uninitialized variable.

4. Use of constant condition

Check your code again if you are using constant or redundant checks in conditional statements.

null/undefined

In Apache Ambari, let’s see an argument check: app/utils/number_utils.js

Condition str==undefined is always false because of the previous condition str==null. Note that undefined equals to null by non-strict equality operator ==. The condition is completely unnecessary.

Also when I review code, I often see argument check code like:

if (arg == null || arg == undefined || arg == '')

You can just do dead simple because == operator in JavaScript treats equally null, undefined and empty string ''.

if (!arg)

Nested if

In Apache Ambari, let’s see an argument check: app/controllers/main/host/bulk_operations_controller.js

turn_off at line 12 is always false because it’s in false branch of the condition at line 5. Therefore line 13 is never executed and included_hosts parameter is never set.

Similar problem can be found in Adobe Brackets: src/extensions/default/UrlCodeHints/main.js

self.cachedHints at line 11 is null because it’s checked by the condition if (this.cachedHints) at line 3. So line 12 is never executed and it implies you need to check the logic for rejection. Whether the rejection was unnecessary or processed at the wrong branch.

As a rule of thumb, check the logic again when you see constant condition and remove it if it turns out to be unnecessary.

5. Inconsistent null checks

Check for null arguments should be performed consistently.

Let’s see an inconsistent example in reveal.js: plugin/markdown/markdown.js

• At line 3, argument element is checked whether it is null.
• At line 11, it is accessed without any check.
• It can cause TypeError when null argument is passed. The check is needed also here if the argument can be null.

Another example in Apache Ambari: app/mixins/common/serverValidator.js

• At first, argument data is checked and a warning is printed.
• But flow does not stop!
• Falsy value is saved as recommended at line 5.
• Finally, TypeError is thrown when accessing resources array at line 6.

For your information, check for data argument was removed in the current code. Like this, consider the following when you check an argument:

• Remove the check itself if the argument cannot be null.
• Add checks consistently for each use.

6. Null pointer

I’ve found some cases trying to access the property of null object. It is often found in an else branch or inline initialization block. Seems that developers used up all their energy in coding the main execution flow.

Let’s see Adobe Brackets: src/language/HTMLInstrumentation.js

At line 20 (else branch), match variable is set to null. But match.mark is accessed at the end of the function, causing TypeError.

I think the fix should be return match && match.mark; and I happily found the problem is gone by this commit saying “This fixes `TypeError: Cannot read property ‘mark’ of null`”. Good job!

Another case in jQuery Mobile: demos/popup-alignment/popup.alignment.js

See align.x at line 11 (inline initialization block). Because align is uninitialized at that point, accessing align.x throws TypeError.

Seems that the desire to save one line of code caused this error. I guess the developer’s intent as follows:

    var arX = parseFloat( ar[ 0 ] );
    align = {
        x: arX,
        y: ar.length > 1 ? parseFloat( ar[ 1 ] ) : arX
    };

Null pointer is one of the major runtime errors. Therefore, if you set a variable to null or access an object in initialization block, pay attention to the way the variable is used afterwards or the object is properly initialized.

7. Misuse of return values

There are two erroneous cases about the return value of a function:

• You use a return value from the function which returns nothing
• You do not use a return value from the function which has no side effect

Let’s see the first case in CodeMirror: mode/vb/vb.js

Function dedent does not return anything. So if (dedent(stream, state)) is always false and ERRORCLASS is not returned.

When should ERRORCLASS be returned? I guess that might be when dedent returns zero or negative index. In other words, it implies that a return statement is missing in the function.

And the second case is often found when using built-in API of JavaScript string.

Below is our in-house code.

var retString = i18n.RESTOREDLG_SUCCESS + ' : ( ' + returnValue.unitServiceName + ' )';
if(returnValue.isRenamed) {
    retString.concat(' , '+i18n.RESTOREDLG_RENAME);
}
Notify.success(retString);

String.prototype.concat() always returns a new string without side effect. But the developer missed the return value of it. Therefore, users see the same notification message regardless of his action.

For the API of array, there is similar confusion. Array.prototype.concat() returns a new array without side effect while Array.prototype.push() appends an item to the input array itself.

It will be better if a tool can check these kinds of confusion rather than the problem is found after deployment. This is why I developed this rule for checking an ignored result of built-in APIs.

8. Bad operators

Next let’s see bad uses of assignment, bitwise, and negation operators.

See below negation operator in our in-house code.

if (!data instanceof Object) {

The expression is evaluated as (!data) instanceof Object, comparing boolean with Object. I bet the developer wanted !(data instanceof Object). If not, using parentheses will be helpful to indicate the rare situation.

The second is about bitwise operator. See uptime: lib/pollers/webpagetest/webPageTestPoller.js

config.webPageTest.testOptions | {} is definitely typo of config.webPageTest.testOptions || {} because bitwise operator in the former converts an object to NaN, resulting zero.

Therefore, you should check it out when the bitwise operator is used in the initialization pattern like bar | {}.

Although JSHint can prohibit the use of bitwise operator, it might be annoying because it detects all uses literally. For example, below code seems correct to me while JSHint complains.

function foo(option) {
    const OPTION_1 = 1 << 0;
    const OPTION_2 = 1 << 1;
    let isOption1 = option | OPTION_1;

For the assignment operator, let’s see MagicMirror: modules/default/alert/alert.js

At line 6, you can see an assignment operator in conditional statement. As a result, show_notification is called even when the payload type is not “notification”.

Pay attention to the assignment operator if it’s in conditional statement.

Thanks for reading!

JavaScript is easy to learn but error-prone unless a developer is extra careful.

So I wanted to share some patterns I’ve found while developing our JavaScript static analysis tool, DeepScan. I hope being aware of these patterns will be helpful to developers.

Tell me if you have an idea for the pattern to be checked in JavaScript. We will realize and spread that idea!

You can find me here:

• Official Website: https://deepscan.io
• Email: support@deepscan.io