permalink

31

Avoiding The Quirks: Lessons From A JavaScript Code Review

I was recently asked to review some code for a new JavaScript application and thought I might share some of the feedback I provided as it includes a mention of JavaScript fundamentals that are always useful to bear in mind.

Code reviews are possibly the single biggest thing you can do to improve the overall quality of your solutions and if you're not actively taking advantage of them, you're possibly missing out on bugs you haven't noticed being found or suggestions for improvements that could make your code better.

Challenges & Solutions

Code reviews go hand-in-hand with maintaining strong coding standards. That said, standards don't usually prevent logical errors or misunderstandings about the quirks of a programming language. Even the most experienced developers can make these kinds of mistakes and code reviews can greatly assist with catching them.

Often the most challenging part of code reviews is actually finding an experienced developer you trust to complete the review and of course, learning to take on-board criticism of your code.

The first reaction most of us have to criticism is to defend ourselves (or in this case, our code) and possibly lash back. While criticism can be slightly demoralizing, think of it as a learning experience that can spur us to do better and improve ourselves because in many cases, once we've calmed down, it actually does.

It can also be useful to remember that no one is compelled to provide feedback on your work and that if the comments are in fact constructive, you should be grateful for the time spent on the input (regardless of whether you choose to use it or not).

Review Sources For JavaScript Developers

JSMentors is a mailing list that discusses everything to do with JavaScript (including Harmony) but also has a number of experienced developers on their review panel (including JD Dalton, Angus Croll and Nicholas Zakas). These mentors might not always be readily available, but they do their best to provide useful, constructive feedback on code that's been submitted for review.

Code Review Beta – you would be forgiven for confusing Code Review with StackOverflow, but it's actually a very useful, broad-spectrum subjective feedback tool for requesting peer reviews of code you've written. Whilst on StackOverflow you might ask the question 'why isn't my code working?', CR is more suited to questions like 'why is my code so ugly?'. If there's still any doubt at all about what it offers, I strongly recommend checking out their FAQs.

Twitter – it may seem like an odd suggestion, but at least half of the code review requests that I submit are through social networks. This of course works best when your code is open-source, however it never hurts to try. The only thing I would suggest here is to ensure you're following or interacting with experienced developers – a code review completed by a developer with insufficient experience can sometimes be worse than no code review at all, so be careful!.

 

The Code Review

On to the review – a reader asked me to go through their code and provide them with some suggestions on how they might improve it. Whilst I'm certainly not an expert on code reviews, I believe that reviews should both point out the shortfalls of an implementation but also offer possible solutions and suggestions for further reading material that might be of assistance.

Here are the problems and solutions I proposed for the reader's code:

 

Problem: Functions and objects are passed as parameters to other functions without any type validation

Feedback: For functions, at minimum 1) test to ensure the parameter passed actually exists and 2) do a typeof check to avoid issues with the app attempting to execute input which may not in fact be a valid function at all.

if (callback && typeof callback === "function"){
    /*rest of your logic*/
}else{
    /*not a valid function*/
}

As Angus Croll however points out in more detail on his blog, there are a number of issues with typeof checking that you may need to be aware of if using them for anything other than functions.

For example: typeof null returns "object" which is incorrect. In fact, when typeof is applied to any object type which isn't a Function it returns "object" not distinguishing between Array, Date, RegExp or otherwise.

The solution to this is using Object.prototype.toString to call the underlying internal property of JavaScript objects known as [[Class]], the class property of the object. Unfortunately, specialized built-in objects generally overwrite Object.prototype.toString but as demonstrated in my objectClone entry for @140bytes you can force the generic toString function on them:

Object.prototype.toString.call([1,2,3]); //"[object Array]"

You may also find Angus's toType function useful:

var toType = function(obj) {
  return ({}).toString.call(obj).match(/\s([a-zA-Z]+)/)[1].toLowerCase()
}

 

Problem: There are a number of cases where checks for app specific conditions are repeatedly being made throughout the codebase (eg. feature detection, checks for supported ES5 features).

Feedback: You might benefit from applying the load-time configuration pattern here (also called load-time or init-time branching). The basic idea behind it is that you test a condition only once (when the application loads) and then access the result of that initial test for all future checks.This pattern is commonly found in JavaScript libraries which configure themselves at load time to be optimized for a particular browser.

This pattern could be implemented as follows:

var tools = {
    addMethod: null,
    removeMethod: null    
};

if(/* condition for native support*/){
    tools.addMethod = function(/*params*/){
        /*method logic*/
    }
}else{
    /*fallback*/
    tools.addMethod = function(/**/){
        /*method logic*/
    }
}

The example below demonstrates how this can be used to normalize getting an XMLHttpRequest object.

var utils = {
    getXHR: null
};

if(window.XMLHttpRequest){
    utils.getXHR = function(){
        return new XMLHttpRequest;
    }
}else if(window.ActiveXObject){
    utils.getXHR = function(){
        /*this has been simplified for example sakes*/
        return new ActiveXObject('Microsoft.XMLHTTP');
    }
}

Stoyan Stefanov also has a great example of applying this to attaching and removing event listeners cross-browser in his book 'JavaScript Patterns':

var utils = {
    addListener: null,
    removeListener: null
};
// the implementation
if (typeof window.addEventListener === 'function') {
    utils.addListener = function (el, type, fn) {
        el.addEventListener(type, fn, false);
    };
    utils.removeListener = function (el, type, fn) {
        el.removeEventListener(type, fn, false);
    };
} else if (typeof document.attachEvent === 'function') { // IE
    utils.addListener = function (el, type, fn) {
        el.attachEvent('on' + type, fn);
    };
    utils.removeListener = function (el, type, fn) {
        el.detachEvent('on' + type, fn);
    };
} else { // older browsers
    utils.addListener = function (el, type, fn) {
        el['on' + type] = fn;
    };
    utils.removeListener = function (el, type, fn) {
        el['on' + type] = null;
    };
}

 

 

Problem: Object.prototype is being extended in many cases.

Feedback: Extending native objects is a bad idea – there are few if any majorly used codebases which extend Object.prototype and there's unlikely a situation where you absolutely need to extend it in this way. In addition to breaking the object-as-hash tables in JS and increasing the chance of naming collisions, it's generally considered bad practice and should be modified as a very last resort (this is quite different to extending host objects).

If for some reason you *do* end up extending the object prototype, ensure that the method doesn't already exist and document it so that the rest of the team are aware why it's necessary. The following code sample may be used as a guide:

if(typeof Object.prototype.myMethod != 'function'){
    Object.prototype.myMethod = function(){
        //implem  
    };
}

@kangax has a great post that discusses native & host object extension which may be of interest.

 

Problem: Some of what you're doing is heavily blocking the page as you're either waiting on processes to complete or data to load before executing anything further.

Feedback: You may benefit from using Deferred execution (via promises and futures) to avoid this problem. The basic idea with promises are that rather than issuing blocking calls for resources, you immediately return a promise for a future value that will be eventually be fulfilled. This rather easily allows you to write non-blocking logic which can be run asynchronously. It's common to introduce callbacks into this equation so that the callbacks may be executed once the request completes.

I've written a relatively comprehensive post on this previously with Julian Aubourg if interested in doing this through jQuery, but it can of course be implemented with vanilla JavaScript as well.

Micro-framework Q offers a CommonJS compatible implementation of promises/futures that is relatively comprehensive and can be used as follows:

/*define a promise-only delay function that resolves when a timeout completes*/
function delay(ms) {
    var deferred = Q.defer();
    setTimeout(deferred.resolve, ms);
    return deferred.promise;
}

/*usage of Q with the 'when' pattern to execute a callback once delay fulfils the promise*/
Q.when(delay(500), function () {
        /*do stuff in the callback*/
});

If you're looking for something more basic that can be read through, Douglas Crockford's implementation of promises can be found below:

function make_promise() {
  var status = 'unresolved',
      outcome,
      waiting = [],
      dreading = []; 

  function vouch(deed, func) {
    switch (status) {
    case 'unresolved':
      (deed === 'fulfilled' ? waiting : dreading).push(func);
      break; 
    case deed:
      func(outcome);
      break;
    }
  };

  function resolve(deed, value) {
    if (status !== 'unresolved') {
      throw new Error('The promise has already been resolved:' + status);
    }
    status = deed;
    outcome = value;
    (deed == 'fulfilled' ? waiting : dreading).forEach(function (func) {
      try {
        func(outcome);
      } catch (ignore) {}
    });
    waiting = null;
    dreading = null;
  };

  return {
    when: function (func) {
      vouch('fulfilled', func);
    }, 
    fail: function (func) {
      vouch('smashed', func);
    },
    fulfill: function (value) {
      resolve('fulfilled', value);
    },
    smash: function (string) {
      resolve('smashed', string);
    }, 
    status: function () {
      return status;
    }
  };
};

 

 

Problem:You're testing for explicit numeric equality of a property using the == operator, but may wish to use === in this case instead

Feedback: As you may or may not know, the identity (==) operator compares for equality after performing any necessary type conversions. The === operator will however not do this conversion so if the two values being compared are not the same type === will just return false.

The reason I recommend considering === for more specific type comparison (in this case) is that == is known to have a number of gotchas. Take for example the following set of boolean checks for equality with it:

3 == "3" // true 
3 == "03" // true
3 == "0003" // true
3 == "+3" //true
3 == [3] //true
3 == (true+2) //true
' \t\r\n ' == 0 //true

If you're 100% certain that the values being compared cannot be interfered with by the user, feel free to opt for ==, however === would cover your bases in the event of unexpected input being used.

 

 

Problem: An uncached array.length is being used in all for loops. This is particularly bad as you're using it when iterating through 'HTMLCollection's

for(var i=0; i< myArray.length;i++){
/*do stuff*/
}

Feedback: This isn't a great idea as the the array length is unnecessarily re-accessed on each loop iteration. This can be slow, especially if working with HTMLCollections. With the latter, caching the length can be anywhere up to 190 times faster than repeatedly accessing it (as per Zakas' High-performance JavaScript). See below for some options you have for caching the array length.

var len = myArray.length;
for (var i = 0; i < len; i++) {
}
/*cached inside loop*/
for (var i = 0, len = myArray.length; i < len; i++) {
}

/*cached outside loop using while*/
var len = myArray.length;
while (len--) {
}

A jsPerf test comparing the performance benefits of caching the array length inside and outside the loop, using prefix increments, counting down and more is also available.

 

 

Problem: Variables are declared all over the place

/*example*/
var someData = "testing";
someMethod.apply(someData);
var otherData = "data1", moreData = "data2";
moreMethods.call(otherMethods());

Feedback: Within functions, it's significantly more clean to use the single-var pattern to declare variables at the top of a function's scope because of variable hoisting.

var someData = "testing",
    otherData = "data1",
    moreData  = "data2";

someMethod.apply(someData);
moreMethods.call(otherMethods());

This provides a single place to look for all variables, prevents logical errors with a variable being used prior to being defined and is generally less code. I also noticed variables being declared without an initial value. Whilst this is fine, note that initializing variables with a value can help prevent logical errors because uninitialized vars are initialised with a value of undefined.

 

Problem: I noticed that jQuery's $.each() is being used to iterate over objects and arrays in some cases whilst for is used in others.

Feedback: Whilst the lower level $.each performs better than $.fn.each(), both standard JavaScript for and while loops perform much better as proven by this jsPerf test. Some examples of loop alternatives that perform better can be found below:

/*jQuery $.each*/
$.each(a, function() {
 e = $(this);
});

/*classic for loop*/
var len = a.length;
for (var i = 0; i < len; i++) {
  e = $(a[i]);
};

/*classic while loop*/
var i = a.length;
while (i--) {
  e = $(a[i]);
}

Given that this is a data-centric application with a potentially large quantity of data in each object or array, it is recommended considering a refactor to use one of these.

 

Problem: There are a number of aspects to the application which you may wish to make more easily configurable.

Feedback: At the moment such settings as stored in disparate var declarations, but using the configuration object pattern may provide a cleaner way changing configurations later on. This is easier to read, maintain and extend, however you do need to remember there is a chance property names won't be minified.

var config = {
   dataSource: 'myDataSource.json',
   debugMode: false,
   langs: ['en','fr','de']
};

 

Problem: JSON strings are being built in-memory using string concatenation.

Feedback: Rather than opting to do this, why not use JSON.stringify() – a method that accepts a JavaScript object and returns its JSON equivalent. Objects can generally be as complex or as deeply-nested as you wish and this will almost certainly result in a simpler, shorter solution.

var myData = {};
myData.dataA = ['a', 'b', 'c', 'd'];
myData.dataB = {
    'animal': 'cat',
    'color': 'brown'
};
myData.dataC = {
    'vehicles': [{
        'type': 'ford',
        'tint': 'silver',
        'year': '2015'
    }, {
        'type': 'honda',
        'tint': 'black',
        'year': '2012'
    }]
};
myData.dataD = {
    'buildings': [{
        'houses': [{
            'streetName': 'sycamore close',
            'number': '252'
        }, {
            'streetName': 'slimdon close',
            'number': '101'
        }]
    }]
};
console.log(myData); //object
var jsonData = JSON.stringify(myData);

console.log(jsonData);
/*
{"dataA":["a","b","c","d"],"dataB":{"animal":"cat","color":"brown"},"dataC":{"vehicles":[{"type":"ford","tint":"silver","year":"2015"},{"type":"honda","tint":"black","year":"2012"}]},"dataD":{"buildings":[{"houses":[{"streetName":"sycamore close","number":"252"},{"streetName":"slimdon close","number":"101"}]}]}}
*/

 

Problem: The namespacing pattern used is technically invalid

Feedback: although namespacing across the rest of the application is implemented correctly, the initial check for namespace existence used is invalid. Here's what you have currently have:

if (!MyNamespace) {
  MyNamespace = { };
}

 

The issue here is that !MyNamespace will throw a ReferenceError because the MyNamespace variable was never declared. A better pattern might take advantage of boolean conversion with an inner variable declaration as follows:

if (!MyNamespace) {
  var MyNamespace = { };
}

//or

if (typeof MyNamespace == 'undefined') {
  var MyNamespace = { };
}

 

@kangax also has a very comprehensive post on namespacing patterns related to this (recommended reading).

And that's it!. If enough readers find this useful I might post more code reviews in future. Until next time, good luck with your own projects and reviews.

31 Comments

  1. Nice post. Glad I already do most of these things. The first solution to the last problem wasn't obvious to me at first, but then I remembered how function scope and var declarations work and it makes perfect sense. I will start to use this technique more often. thanks

  2. useful. Might be good to split it into a code-review series; you covered 6 or 7 issues in this one. Why not let each of them stand alone.

    • Thanks for the feedback. I'll definitely consider splitting up the reviews into multiple posts as you're right, more detailed lessons on some of these gotchas may be more appropriate. Cheers!

  3. Great post! Regarding the uncached array.length, I often see reverse for loops:

    for (var i = myArray.length – 1; i > 0; i–) { … }

  4. Great post! Regarding uncached array.length, I often see reverse for loops as well:

    for (var i = myArray.length – 1; i > 0; i–) { … }

    • Thanks Mike!. I certainly plan on covering memoization if I come across these issues another review, but I'll see what I can do to expand on this post to include it.

      Appreciate the gist (which I recommend anyone wishing to compare both approaches check out).

  5. Pingback: Avoiding The Quirks: Lessons From A JavaScript Code Review | Efekat ♥

  6. Nice review. Just a note; your comparison of `for` vs `jQuery.fn.each` isn't accurate. In the JSperf comparison code you retrieve the DOM objects to be iterated on and cache them in the preparation code. You then iterate over the cached objects. This is fine but when you iterate using jQuery.fn.each, you are not iterating over the cached objects, you are retrieving the objects again… $('*').each().

    The updated JSPerf shows that there is not really any significant performance difference between any of the different loop implementations.

    • I read your post, but I'm a little confused about your findings. You suggest that using the single-variable pattern can cause you to run into issues with scope because JavaScript will effectively force any variables after the first line in that pattern to be global. Here's a jsFiddle where attempting to access variables defined after the first comma cannot be accessed within the global scope, only within the local function scope, which is how I expect it to work. http://jsfiddle.net/zVC4Z/2/ Is there something I'm missing?

  7. Pingback: Faster JavaScript Memoization For Improved Application Performance

  8. Pingback: Interesting Stuff on the Web – 2

  9. Pingback: Avoiding The Quirks, BoltJS, Mobile Sites vs. Mobile Apps, BOSSMAN, FlightGear, Webify Me | Serene Global

  10. As usual, excellent post Addy. Being a developer at a small web development firm its difficult for me to find people to review my JavaScript code. I've tried posting some of my JavaScript on codereview.stackexchange.com but haven't received the kind of feedback that I'd like, mostly because you've set the bar quite high here with this post!

    Do you have any suggestions were a person like myself could go to get a good review?

    • Sure. If you're finding that stackexchange and stackoverflow aren't providing the level of feedback you're after, I would consider trying to ask other JavaScript developers in the community (those who you feel might be experienced/qualified enough to answer) to try their hand at a review. Look at developers who actively contribute to open-source projects for example.

      Now, a lot of them might be busy with other projects, but post some code in a gist, ask them on twitter for their opinion on whether anything could be done better and see if you get a response (you may find developers who contribute to smaller projects more able to assist here).

      The important thing is to make sure not to spam anyone. I can't personally speak for others but it regularly takes me a few days to get back to all the messages I get, but I try my best and I'm sure its the same with others. Hope that helps!

  11. I’ve been looking for an implementation of Crockford’s promise code but haven’t been able to find one. jQuery’s fine but it would be nice for the purpose of better understanding JavaScript to see how to create and use promises without resorting to a library or framework.

Leave a Reply

Required fields are marked *.