#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).
####JavaScript developers looking for a way to get code reviews have a few different options here:
JSMentors (http://jsmentors.com/) - this 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 (http://codereview.stackexchange.com) - 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 (http://codereview.stackexchange.com/faq).
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!.
##My 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 to be used as callbacks (as well as objects) are passed as parameters to other functions without any type validation.
Feedback: For functions, at minimum 1) test to ensure the callback 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*/
} As Angus Croll however points out in more detail here: http://javascriptweblog.wordpress.com/2011/08/08/fixing-the-javascript-typeof-operator/, 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 (https://gist.github.com/1136817) 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 utils = {
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 here: http://perfectionkills.com/extending-built-in-native-objects-evil-or-not/
####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 (http://msdn.microsoft.com/en-us/scriptjunkie/gg723713), but it can of course be implemented with vanilla JavaScript as well.
Micro-framework Q (https://github.com/kriskowal/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
As you may or may not know, the identity (==) operator compares for equality after 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 //trueIf 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 invent 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.
eg.
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.
/*cached outside loop*/
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: http://jsperf.com/caching-array-length/7
####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
/*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.
eg.
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
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 has a very comprehensive post on namespacing patterns related to this here: http://perfectionkills.com/unnecessarily-comprehensive-look-into-a-rather-insignificant-issue-of-global-objects-creation/ (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.