Code Renaissance is about building great teams and great software. By exploring best practices, team interactions, design, testing and related skills Code Renaissance strives to help you create the team and codebase that you've always wanted.

Cleaning up Bad Javascript with JSLint

The web app I'm working has a ton of Java Script. It was written by many people, over a short period of time, under incredible deadlines. To make matters worse the direction of the product changed several times during initial development, which required things to be shifted and duck taped together quickly with no time for cleanup. Needless to say, the code base is a mess.

So what's the answer to cleaning up a brittle Java Script code base?

JS Lint!

JSLint is life, JS Lint is sanity. Yes it can be a huge pain and even it's author Douglas Crockford notes "JS Lint will hurt your feelings", but JSLint is the first step in solidifying the most horrid code base.

When I run a file through JS Lint for the first time I inevitably find several problems:
  • Unreferenced code - it's very difficult to make sense of code that has unused functions and variables; how can you know what's relevant? JS Lint will flag these for you so you can remove them.
  • Bad practices - While I'm sure JS Lint's list of bad practices will include things you disagree with, it's worth it to fix all of the minor errors so that the truly horrific things will be revealed. Be very careful with your changes though. When JS Lint says you should use !== instead of != for comparisons doesn't mean that's the exact change you should make.
    if(a != 0) could need to be changed to any of the following:
          if(!a)//excludes any falsy values (NaN, '', false, 0, null, undefined)
          if(a !== 0)//excludes 0
          if(parseFloat(a,10) !== 0)//excludes 0 and '0', but not '' or 'abc'
          if(parseInt(a,10) !== 0)//excludes 0, '0', but also 0.13, '.13'( truncated .)
  • Undeclared global references - JS Lint flags undeclared global references. Some of these will be objects that are missing a declaration, some will be mispelled variable names, and some will be intentional global references to other js files. Once you comment your intentional globals JS Lint will no longer flag them. I prefer to comment my globals like this:
    Authorization, ApprovalTypes, //Authorization.js        
    jQuery, $, //jQuery 
    window //browser

Once you've gotten your file to pass JS Lint and tested your changes then you need to do a check-in. This gives you a stable starting point to begin refactoring the code. I'll admit this can be dangerous but rerunning the code through JS Lint every so often as you're making changes catches a lot of issues. Everytime you revisit a file to make a change refactor it a little more. If the function you're working on is too big and too confusing then extract out its different responsibilities into cohesive methods. Clean up the code until it makes sense and you can reliably make your change.

It is critical to resist the temptation to clean up code that will not be tested. Don't fool yourself into thinking you won't introduce bugs, you will. That's why it's equally critical to take advantage of the opportunity to clean up the code while you're making functional changes. This will allow your clean up efforts to be tested. Don't Pass up an opportunity to clean up code while you're in it... that's how the code became brittle in the first place.

2 - What do you think?:

Erik Giberti said...

I've dealt with similar issues with large bodies of JavaScript code. I finally took the time to automate JSLint testing and bundled it into the deploy script on a few projects.

DH said...

Thanks Erik. I read/liked your article. Unfortunately I work in a corporate/windows environment and that's not a tool-set that they'd approve of. Still it great to see the direction things are heading.

I installed JSLint.VS today which should allow some similar things but unfortunately I'm having a little trouble with it. We'll see.

Thanks again.