Javascript code quality, as part of your build

01 Aug 2011

Static code analysis

So you're developing a web application full of rich client-side interactions. Maybe it started off as a few functions, but by now you have quite a collection of Javascript files and modules. Chances are you're already treating this code like any other code: it's modular and unit tested, and that's great! So what's the next step?

Javascript

Well in my current project we thought: we run checkstyle on Java code, what about doing the same in Javascript? Sure, static code analysis has some drawbacks, but all in all if you pick the right rules it helps a lot with catching mistakes early and defining general guidelines. That's even more true in a language like Javascript where there is no compiler to warn you about an inadvertent access to the global scope or even a plain pitfall of the language.

We first trialled JsLint - but as much respect as I have for Douglas Crockford it didn't work for us. It had a tendency to report errors in completely valid code and isn't very customisable. Which is why I'm glad we found JsHint, a fork of the initial project that addresses these exact issues.

As part of the build

Now what we wanted was to integrate it into our build:

JsHint is now integrated in wro4j, but we had trouble processing all our files at once, and it also doesn't support a mercy / threshold. I think this is quite critical when running code analysis on an existing codebase, since it's very unlikely you will fix all the issues at once. Mercy values allow you to ensure the number of errors always goes down until it finally reaches 0.

So we decided to go with the simplest approach: JsHint actually ships with a Rhino adapter to run the analysis from the command line. We needed to make a few changes, but no 50 lines of Javascript were going to stop us Smiley. It's all pretty straightforward here, we're simply calling JsHint on a list of files, counting the errors, and failing the build if need be.

Show me the code

/* rhino_multi.js */ 

load("jshint.js");
load("options.js");

(function validateMultipleFiles(args) {

    function getErrors(file) {

        var input = readFile(file);
        if (!input) {
            print('jshint: Couldn\'t open file ' + file);
            return 1;
        }

        if (!JSHINT(input, JSLINT_OPTIONS)) {
            var shortName = (file.length <= 40) ? file : '...' + file.substring(file.length - 40);
            print('\nErrors in ' + shortName + '\n');
            JSHINT.errors.forEach(function (err) {
                print('    ' + err.reason + ' (line: ' + err.line + ', character: ' + err.character + ')');
                print('    > ' + (err.evidence || '').replace(/^\s*(\S*(\s+\S+)*)\s*$/, "$1") + '\n');
            });
            return JSHINT.errors.length;
        }

        return 0;
    };

    var totalErrors = 0;
    var files = Array.prototype.slice.call(args);
    var errors = files.forEach(function (file) {
        totalErrors += getErrors(file);
    });

    print('JsHint completed with ' + totalErrors + ' errors (mercy is ' + JSHINT_MERCY + ')');
    quit (totalErrors <= JSHINT_MERCY ? 0 : 1);

}(arguments));

You will notice a reference to options.js and the global variables JSHINT_MERCY and JSLINT_OPTIONS. We didn't come up with anything better at the time, but are open to suggestions! Anyway, this is what it looks like, and where all the build configuration is done - simply refer to the JsHint website for the list of available options.

/* options.js */

var JSHINT_MERCY = 60;
var JSLINT_OPTIONS = {

    // Environments

    browser     : true,     // assume browser global variables (document, window...)
    jquery      : true,     // assume jQuery global variables
    es5         : true,     // assume EcmaScript 5 compatibility (we use a shim for old browsers)

    // Options

    curly       : true,     // if curly braces around blocks should be required (even in if/for/while)
    debug       : false,    // if debugger statements should be allowed
    eqeqeq      : true,     // if === should be required
    evil        : false,    // if eval should be allowed
    forin       : false,    // if for in statements must filter
    immed       : true,     // if immediate invocations must be wrapped in parens
    noarg       : true,     // if arguments.caller and arguments.callee should be disallowed
    noempty     : true,     // if empty blocks should be disallowed
    onevar      : false,    // if only one var statement per function should be allowed
    undef       : true,     // if variables should be declared before used

};

As this point you should have a folder structure that looks just like this:

JsHint

Maven example

So now it's just a matter of calling our Rhino wrapper as part of the build. This can be easily done regardless of the build platform you use… in the case of Maven, a simple antrun plugin does the trick just fine.

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-antrun-plugin</artifactId>
    <version>1.6</version>
    <executions>
        <execution>
            <phase>compile</phase>
            <goals>
                <goal>run</goal>
            </goals>
            <configuration>
                <target>
                    <apply dir="${basedir}/jshint" executable="java" parallel="true" failonerror="true">
                        <fileset dir="${basedir}/src/main/webapp/javascript" excludes="3rdparty/**" />
                        <arg line="-jar ${settings.localRepository}/rhino/js/1.7R2/js-1.7R2.jar"/>
                        <arg line="${basedir}/jshint/rhino-multi.js" />
                    </apply>
                </target>
            </configuration>
        </execution>
    </executions>
    <dependencies>
        <dependency>
            <groupId>rhino</groupId>
            <artifactId>js</artifactId>
            <version>1.7R2</version>
        </dependency>
    </dependencies>
</plugin>

The build output!

So that's it, with this setup you should have JsHint running on all your Javascript code - and should see something like this as part of your build.

[apply] Errors in ...webapp\javascript\details.js
[apply]
[apply]     Missing radix parameter (line: 45, character: 31)
[apply]     > total = parseInt(sum) + parseInt(allowance);
[apply]
[apply]     Expected === and instead saw == (line: 48, character: 21)
[apply]     > if (value == 0) {
[apply]
[apply] Errors in ...\webapp\javascript\email.js
[apply]    
[apply]     Unnecessary semicolon (line: 259, character: 5)
[apply]     > ;
[apply]    
[apply] JsHint completed with 15 errors (mercy is 16)

Next step: bringing the number of errors to 0 - and feeling warm and fuzzy about our cleaner Javascript code!

Comments