Automating JavaScript Code Quality Checks

At Wealthfront, we’re big advocates for automation. In general, automation saves time and ensures consistency.

One of the things we want to ensure is the quality of our JavaScript code. This is particularly important for JavaScript, given its weirdness. While this task can’t be fully automated, there’s some low hanging fruit available by automating linting and style checking.

Some tools for this purpose are the Google Closure Compiler, Google Closure Linter, JSLint, JSHint, JSCS, and ESLint. We decided to use JSHint for detecting potential bugs, and JSCS for automated style checking. While we’re actually using Closure for compiling our JS, it wasn’t flexible enough for us to use with our current codebase. ESLint looks like it’ll be really good, perhaps better than JSHint plus JSCS, but since it’s still new, we decided against it.

What do we do with JSHint and JSCS?

– JSHint checks for potential errors, such as using an undefined variable or forgetting a break statement in a switch block.

– JSCS enforces a common style, such as enforcing camelCase variables, always using semicolons, or requiring lines no more than 120 characters in length.

– Both checks run whenever someone checks code into a side branch, and before deploying. Engineers also are able to run the checks locally.

What won’t we do with this setup?

– An automated system won’t check that code is well designed (DRY, MVC, etc, although ESLint is does have DRY rule tests on their roadmap). For that, you should hire well, emphasize learning, share best practices, and hold code reviews.

– It won’t check as thoroughly as the Google Closure Compiler. Since the Closure tools compile your code, they’re able to analyze more thoroughly. Closure can detect more potential bugs, like functions called with incorrect parameters, or unused or unreachable code that probably indicates a bug. It can also apply JSDoc comments for additional benefits, like type checking and deprecated code. (Our JSCS validates JSDoc too, but it isn’t nearly as powerful as Closure, and we rarely use JSDoc.) One disadvantage of the Google Closure Compiler is that it is less configurable than other tools. We actually compile our JavaScript with the Google Closure Compiler, and might use its code quality warnings in the future, but decided on JSHint plus JSCS for now.

If either JSHint or JSCS detects a problem, our build emails us with an explanation. For instance, while refactoring some inline JavaScript to place it in its own file, there was a variable defined in evaluated Ruby:

var shouldShowInstructionsParams = [“<%= @deposit_type %>”, “<%= @deposit_amount %>”];

In the process of refactoring, the variable was renamed to a property of our w.inlineVars object, so that it wouldn’t be a global variable.

w.inlineVars.shouldShowInstructionsParams = [“<%= @deposit_type %>”, “<%= @deposit_amount %>”];

Unfortunately, the variable didn’t get renamed everywhere it was used in the JavaScript files. This is the sort of thing that your unit tests would catch, assuming the unit tests cover it. Even without a test, though, the linter caught the bug:

    Checking style with JSHint...
    app/assets/javascripts/pages/transactions.js: line 14, col 7,     'shouldShowInstructionsParams' is not defined. (W117)
    1 error

Once alerted, it’s easy to recognize the error and change shouldShowInstructionsParams to w.inlineVars.shouldShowInstructionsParams, fixing the bug.

One of the reasons we decided on JSHint and JSCS over Closure is flexibility. We’re able to configure the tools to match the rules we want to enforce. For instance, here’s our .jshintrc file:

{
  "bitwise": true,
  "browser": true,
  "camelcase": true,
  "curly": true,
  "eqeqeq": true,
  "immed": true,
  "jquery": true,
  "latedef": true,
  "loopfunc": true,
  "maxdepth": 5,
  "maxlen": 120,
  "maxparams": 6,
  "multistr": true,
  "newcap": true,
  "nonstandard": true,
  "sub": true,
  "undef": true,
  "unused": true,
  "globals": {
    "_": false,
    "d3": false,
    "ActiveXObject": false
  }
}

This describes how we’ve configured our JSHint rules. For instance, eqeqeq means we require triple equal signs over double equal signs. You can find the full list of options here: http://www.jshint.com/docs/options/

If you’re looking to improve the quality of your company’s JavaScript, we recommend adding automated quality checks to your build process. It’s also useful for open source projects, because you’ll get lots of pull requests that don’t look like the rest of the project’s code. Being able to say “PRs need to pass JSHint/JSCS” is a simple way to enforce consistency as well as find potential bugs.