justjs: node.js tutorials

New here? You might want to start at the beginning.

3/11 '13

jslint: a tool for better code quality (and a certain amount of picky nonsense)

 

Today I made the uploadfs npm module's main .js file jslint compliant (or as close as a node module can be, anyway). jslint is an extremely powerful tool for catching "lint" in JavaScript code, not just excellent at finding likely bugs but also at encouraging code that will be easy for others to maintain and teaching you things you never knew about JavaScript along the way.
 
jslint is also a little annoying. But as author Douglas Crockford, one of JavaScript's greatest contributors, puts it right up front: "jslint will hurt your feelings." So I guess I shouldn't be too surprised.
 
Here is a revealing log of what I had to change, what felt valuable or even super-important, what felt unnecessary but ultimately kinda helpful, and what just seemed over the top.
 
But first I should clarify something that was confusing for me: jslint is frequently used to check (partially) for compliance with JavaScript's new-ish "strict mode," and indeed jslint insists you turn strict mode on. But strict mode and jslint are not the same thing. jslint enforces many things, like indentation conventions, that strict mode doesn't care about. And strict mode, because it is in effect at runtime, can flag things a static analyzer like jslint usually can't see, like attempts to delete properties that can't be deleted.
 
In general, you shouldn't judge strict mode's value by what jslint complains about.
 
Thanks to @slicknet for de-confusing me on this point. Let's jump in and see what jslint has to say.
 
Off the bat we get:
 
#1 Unexpected dangling '_' in '_'.

    var _ = require('underscore'); // Line 1, Pos 5
 
jslint considers leading and trailing underscores a bad practice. Why? Frankly, I'm not sure. Sometimes they are used to signify "private" properties, which could be harmful because JavaScript doesn't actually enforce that in any way. But that's just speculation.
 
At any rate, this is problematic for me in two ways: 
 
1. The underscore module, which itself helps avoid many worst practices (especially in browser-side code) by providing safe, correctly written tools for functional programming, is traditionally assigned to the _ variable, which makes it convenient to use and thus more likely to be used.
 
2. Node itself introduces variables like __dirname which also violate this rule.
 
We can override it by introducing this comment which is interpreted as a command (a "pragma") by jslint:
 
/*jslint nomen = true*/
 
Now we graduate to:
 
 #1 Expected exactly one space between 'function' and '('.

    module.exports = function() { // Line 10, Pos 26
 
This feels like sheer pickiness to me, but a case can be made that a language with one code formatting convention is a language that's easier for everyone to pick up and maintain existing code with. So let's suck it up and change every instance of function( to function ( and move on.
 
Next jslint wants us to make our desire for strictness explicit:
 
 #1 Missing 'use strict' statement.

    return new uploadfs(); // Line 11, Pos 3
 
OK, so at the top of the module:
 
"use strict";
 
We've moved along to:
 
 #1 Expected 'return' at column 5, not column 3.

    return new uploadfs(); // Line 13, Pos 3
 
jslint wants us to indent everything four spaces rather than two. I like two, but again: conventions make code easier to maintain. Also, it reminds us not to get too crazy with nested callbacks because the code starts dancing off the edge of the screen. (For those wondering how to avoid getting crazy with nested callbacks, see my article on the proper use of the async module.)
 
Fine, four spaces it is. Batter up!
 
 #1 'uploadfs' was used before it was defined.

    return new uploadfs(); // Line 13, Pos 16
 
OK, this is irritating. The complaint is that I haven't defined uploadfs yet, but I do define it with the function keyword later in the same file. Computers can sort. If it's more readable to do things in this order, that may produce more maintainable code, not less.
 
But... perhaps this just means I should move my uploadfs constructor to a separate file, and require that file. There's something to be said for that, even if the main module file is about three lines long at that point. For now we'll just move our declaration of module.exports after the constructor and go on with the show. Declarations first, immediate execution second.
 
Now we get a sea of complaints about separate var statements for each variable:
 
 #1 Combine this with the previous 'var' statement.

    var backend; // Line 14, Pos 9

 #2 Combine this with the previous 'var' statement.

    var imageSizes; // Line 15, Pos 9

 #3 Combine this with the previous 'var' statement.

    var orientOriginals = true; // Line 16, Pos 9

 #4 Combine this with the previous 'var' statement.

    var self = this; // Line 17, Pos 9
 
Now this I simply don't get. You can definitely make the case that separate statements to define each variable are more readable, not less, than one long statement. It's hard to find anybody finds extra var statements befuddling. I'm going with this one under protest.
 
 
Now we have:
 
 #1 Strict violation.

    var tempPath, backend, imageSizes, orientOriginals = true, self = this; // Line 13, Pos 71
 
Great, that's informative.
 
Fortunately, a few runs prior to this point I did see a better error message, warning that constructor functions (those used with the new keyword) should have uppercase names. I am not sure why this error message no longer appears by the time I reach this point, but it does turn out to be the issue. And I have no problem with the idea that a convention for constructor names is useful. So changing the name of the uploadfs function to Uploadfs, we can move on:
 
 #1 Expected exactly one space between 'typeof' and '('.

    if (typeof(options.backend) === 'string') { // Line 19, Pos 19
 
Apparently typeof gets a trailing space too. Grumble grumble, consistency is good, okay.
 
 #1 Unexpected '(space)'.

    options.backend = require(__dirname + '/' + options.backend + '.js');  // Line 20, Pos 82
 
Trailing space on the line is detected and griped about. Cleaning up.
 
 #1 Use the || operator.

    imageSizes = options.imageSizes ? options.imageSizes : []; // Line 24, Pos 41
 
OK, now this is awesome. I have written too much code in other languages in which the || operator returns 1 or 0, or "true" or "false." jslint is reminding me that in JavaScript, the || operator always returns the value of the first "truthy" argument it encounters, whether that is the left or right side. And that means you can use it as a more succinct alternative to the ternary operator in some cases:
 
imageSizes = options.imageSizes || [];
 
To complain about this, you'd have to resort to an argument like "I have to write code in other languages too and now I'm going to screw up over there," and I'm not crazy about that line of argument, because it leads to lowest common denominator programming. So I like this rule.
 
OK, now I have a bunch more functions that complain that I use them before I define them... and now things are getting annoying. I happen to think this is a very readable pattern:
 
async.series([getThings, permissions, frobThings], sendThings);


function getThings(callback) { ... }


function permissions(callback) { ... }


function updateThings(callback) { ... }


function sendThings(err, result) { ... }
 
This is good top-down programming (hey, remember your first programming class ever?) and makes the overall intent easy to understand. Our best alternative is to... I don't know what our best alternative is. We can nest the functions directly in the array in the async.series call, which decreases readability. We can declare the functions first, which decreases readability (I want to understand the overall intent before I see these details). Or we can pretend we're writing Java by moving them all into a "class file" and requiring that first. I am not crazy about any of these approaches to what should be a simple problem.
 
I am going to declare the functions inline. I can collapse them in my editor when I'd rather see an overview of what's going on, at least. That looks like:
 
async.series([ 

    // create temp folder if needed

    function (callback) { ... },

    // invoke backend init with options

    function (callback) { ... },

], callback);
 
That brings us to:
 
 #1 Unexpected 'else' after 'return'.

    return fs.mkdir(options.tempPath, callback); // Line 41, Pos 25
 
Which is complaining about:
 
if (!exists) {

    return fs.mkdir(options.tempPath, callback);

} else {

    return callback(null);

}
 
Wait, what? Why would it not be cool to use an else statement after a return statement inside a conditional?
 
Well, it's not wrong. But it is entirely unnecessary, because the return statement means there's no possibility that execution will continue past this point anyway unless the condition was false. So we can write:
 
if (!exists) {

    return fs.mkdir(options.tempPath, callback);

}

return callback(null);
 
This feels like jslint just telling me to stop typing so much, rather than actually protecting me from anything. But there's a larger point, which is that jslint is calling me out to understand my own code fully. If I don't "get" that the else clause can never execute, maybe I don't understand that I've returned...
 
OK, yeah, that seems like a stretch to me too. But let's move on.
 
Now we have:
 
 #1 Expected ';' and instead saw ','.

    }, // Line 52, Pos 6
 
This is at the end of a block that began like this:
 
self.init = function (options, callback) {
 
Hey, good catch, jslint. This is an assignment statement, so it really ought to end with a semicolon. Ending with a comma will work, but it implies I don't really get what's going on here and that's worth flagging. (Or that this code started out following the object literal notation before I switched to assignment statements. But anyway.)
 
After a few variations on what we've already seen, we come to:
 
 #1 Combine this with the previous 'var' statement.

    var t = context.info.width; // Line 130, Pos 29
 
This is interesting because the "if" statement seems to imply we shouldn't:
 
var somethingElse;

if (...) {

    var t = context.info.width;

}
 
Can we really marge them? Doesn't that "if" statement create a new scope? Repeat after me: the only safe way to create a new scope in JavaScript is to nest another function! Otherwise the result is very much browser-dependent. Chrome will create a new scope for a "while" statement, Firefox won't (unless you use a Firefox-specific keyword). Yes, we're in node and could make assumptions, but let's not get in the habit of writing code that will bite us if we use it in the browser. Besides, this is an if statement, and I'm not sure if even Chrome creates a new scope there. 
 
Better to write:
 
var somethingElse, t;


if (...) {

    t = context.info.width;

}
 
Which should mean the same thing in all environments. So that's another good catch from jslint.
 
The rest of the results for this file were similar, with jslint pointing out when I followed a "function name(args) { ... }" block with a semicolon (hey programmer, you know this wasn't an imperative statement, right?) and, similarly, when I didn't follow an imperative statement with a semicolon.
 
Overall jslint's input was helpful. It did not catch any bugs, though I suspect it would have told me if I'd had any variables without var statements hanging about. But to be fair to jslint, this module already has strong unit tests, so major bugs have likely been flushed out already. Had I used jslint throughout its development I might have passed those tests sooner.
 
My one major beef with jslint is the objection to using functions before they are declared, something all browsers in even remotely common use today do support (as does Node, of course). Avoiding that can lead to an awkward style in which it is difficult to understand the overall purpose before diving into the details. But again, to be fair, you can shut this off with the "undef" pragma.
 

jslint is good for you

jslint bugs me, but it's good for me. I intend to make it part of my regular practice and encourage its use company-wide at P'unk Avenue, though I may decide to shut off the "undef" rule to allow top-down development, and I will certainly keep the rule against leading underscores shut off. One of JavaScript's few serious lingering significant negatives is the availability of significantly dangerous things, like accidentally declaring a global variable in the middle of a function. jslint and strict mode work together to effectively address such risks.

(I've committed my changes to uploadfs in git. I'll release them as a new npm version when I've had time to jslint-ify the various storage backends as well.)

 

 
blog comments powered by Disqus