Why you should lint your code

For this blog post, I thought I’d share some thoughts on a tool I worked with during this term while contributing to Review Board. I’ll understand if you prefer to skip to the last section for the essentials : my impressions on working on Review Board with UCOSP this term 🙂

Review Bot and static analysis

My focus on Review Board was on Review Bot, an extension to Review Board that automates various tasks on uploaded code in a Review Board review request, then publishes its output as a review. One main focus of Review Bot at the moment is running static code analysis on code submitted in a review request, i.e. running a style checker such as PEP8 (for Python code) and outputting style errors found. My contributions were thus in the same vein, and my task was two fold : 1) Add another static analysis tool : in particular, the JSLint JavaScript code quality tool; 2) Add functionality to allow manual triggering of Review Bot’s static analysis (rather than only allow its automatic trigger on review request publication).

For the first, I essentially had to add a JavaScript engine as a dependency to Review Bot so that we could run JSLint (written in JavaScript) on our code. I then ported JSLint itself into our project and plugged it into our existing tool framework so that it could be triggered along with PEP8. The second task is in progress, but it involves adding some new elements to the UI and tweaking the Review Bot extension such that it could be triggered manually from the Review Board GUI, taking into account user options and permissions that should be set to arbiter the running of scripts on code uploaded to Review Board.

But why static analysis?

In this context – Review Board is a code review tool after all – the answer appears fairly obvious : production-quality code should be tidy and conform to certain standards before it gets merged into the codebase, and a quality verification process should be part of the review. Static analysis of code – “linting” in the case of JSLint and other similar tools – is an excellent tool in ensuring quality. Human errors are not uncommon, and even if the compiler doesn’t complain, there are style errors that can actually lead to defects. Another point to ponder: a static analysis tool will complain if certain variables are undeclared, if variable names are misspelled, if the code isn’t formatted consistently, among other things. If a static analysis tool is having trouble deciphering your code, consider that it might be difficult for a fellow human to understand it as well. This is where static analysis helps us write quality code that not only performs well, but less painful to read in pair or group programming situations.

Case in point : JavaScript. It is a forgiving language; some might call it sloppy. The standard JavaScript interpreter won’t enforce variable declaration, won’t ensure that a method call respects the actual method signature, won’t complain if variable nomenclature is inconsistent, won’t require that constructors begin with an upper case letter, allows overloading of the “+” operator, allows type coercion (ie automatically converts one type to another) … we get the idea. So how do we ensure that our JavaScript is still valid and polished at the end of the day? Enter JSLint.

JSLint by Doug Crockford

Doug Crockford is a JavaScript enthusiast, as it appears from his writings on the matter. He is a believer that JavaScript, despite it’s more easy-going and Web-related nature, is not in fact a tool for amateurs, but a capable and professional-grade language that is suitable for all sorts of purposes other than dynamic Web page functionality (see his article on JS being a misunderstood language). He is also weary of the mistakes that programmers can easily make in JavaScript, mistakes that would be permitted based on the ECMAScript Programming Language Standard, but that just won’t fly in a professional context. Hence his “professional subset” of JavaScript defined by his code quality tool JSLint.

JSLint is strict. It won’t tolerate missing semi-colons, neither does it like variables defined in blocks. If you are writing JavaScript, according to JSLint, you must ensure that each line (one instruction per line only) is terminated by a semi-colon so as to prevent ambiguity, and you must declare all variables at the top of a function because there are just too many scope issues if you don’t. Also, variables must be declared using the var keyword, lest an undeclared variable become an implied global variable. If statements must be written using { } brackets, rather than take advantage of of the following allowed syntax :

if (condition)

statement

as that can easily be misinterpreted by another coder. Constructors used with the new prefix must be capitalized, or else if the new keyword is omitted, a lack of capital letter convention could lead to a call to the wrong function. The ++ and — operators must be avoided because a syntax error such as print(“Hello” + + name) could be mistaken for a legitimate operation, and bitwise operators must be avoided because they are not as efficient as they are meant to be in the first place (because of conversion from floating point to integer and back), and secondly their resemblance to boolean operators (see &, and I) make them prone to error. So Crockford says no. JSLint also says no to the “eval()” function, which is often a security hole if not used carefully as it can execute a (potentially malicious)user-provided string to the application’s (or user’s) peril.

These are just the points that stood out : a complete list is available and explained in the JSLint documentation. It seems so far that however restrictive these rules seem to be, they are backed up with valid arguments that identify potential (and dangerous) programming errors that could easily go uncaught or worse, be propagated (think very large code base and/or very many developers). What’s more, ALL of the above requirements can be turned off via JSLint’s options. However, some still argue that JSLint might go too far.

When the code quality tool is … a tyrant ?

Anton Kovalyov and Paul Irish wrote JSHint : a popular fork of JSLint. Kovalyov himself, a past content JSLint user, describes that eventually Crockford’s tool became “increasingly opinionated and hostile towards your code”. His view is that JSLint is looking less and less like a defect-prevention quality tool and more and more like a way to coerce developers into writing JavaScript the way Crockford would like it. He provides a hard-to-refute example in which the following construct would be forbidden :

for ( var name in obj ) {

}

This would be prohibited by JSLint because the tool requires variables to be declared at the beginning of functions only, with an extended preference that only one “var” declaration be made (forcing all variables to be declared in one place). While the latter requirement can be switched off using JSLint’s options, the former (and main) rule cannot. This turns off some (a lot of?) developers who would rather allow some flexibility for constructs like in the above example.

To that point, one particular other restriction in the JSLint documentation stood out : underscores are forbidden as the first letter in variable names. While it is not uncommon to use the underscore prefix in nomenclature to distinguish “private” variables (not an official notion in JavaScript), Crockford says this does not actually ensure privacy (he is right) then adds “Avoid conventions that demonstrate a lack of competence.” Ouch. On the bright side though, even this restriction can be turned off using options.

The response, JSHint, is a tool that allows the developer more flexibility to determine their own coding conventions before going into static analysis. One of the main problems developers who used JSLint seemed to have was that JSLint would stop short of scanning the entire file after having met what appears to be a variable quota of errors. JSHint takes away that problem (and it’s a pretty big one). I have never used JSHint so I cannot comment, but I’d be curious to see the difference for myself.

Conclusion?

It’s worth mentioning that JSLint’s source contains the following comment:

“//WARNING: JSLint will hurt your feelings”. I suppose it’s meant to be aggressive and intolerant, as that is precisely what Crockford appears to be after: ensuring poor programming habits don’t create unintended errors, and that coders can focus on writing code and not on scrutinizing their own styling mistakes, since the tool rigorously does it for them. That error quota is quite bothersome, but I can appreciate the intention here: style and syntax matter when it comes to preventing avoidable problems down the line.

Enough rambling about JavaScript: why everyone should do UCOSP

Back at university, where I study Computer Science, “the craft” is generally reduced to a theoretical subject. A lot of the coding assignments from day one were to be done in Java, and the extent to which we study Web frameworks and other Web technologies is minimal. Sad as this is, there is hope. Once in a while, the opportunity to work on an exciting project like Review Board comes along and rocks the worlds of those who participate, by allowing them to explore and experiment with new technologies and environments, learn to overcome real new challenges that help build new skill sets, and contribute to code that matters to real users. As much as I might sound like every other Computer Science who has complained, I cannot emphasize enough how important it has been for me to break out of the comfort of the classroom during my first degree and experience coding on a larger project such as this one. Given the opportunity to participate in the Undergraduate Capstone Open Source Projects (UCOSP) for my final honours project, I jumped at the possibility to be part of a new team and collaborate with creative minds. Needless to say, I have no doubt it was the right thing to do. The many new things I’ve learned during this term so far – from becoming comfortable in a Linux environment to learning a new programming language (Python) new Web framework (Django) – have made this an invaluable experience.

As the semester comes to an end, hard as it is to believe, these are in fact closing remarks on my Review Board experience. This was an immensely rewarding and educational experience for me, and I am a better developer now because of it.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s