#Lessons From A JavaScript Code Review
I was recently asked to review some code for a 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.
####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{
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 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: An uncached array.length is being used in all for loops.
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).
/*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--) {
}See here for a jsPerf comparing the performance benefits of caching the array length inside and outside the loop, using prefix increments, counting down and more. http://jsperf.com/caching-array-length/7
####Problem: Variables are declared all over the place.
var someData = /**/
someMethod.apply(someData);
var otherData = /**/, moreData = /**/
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. 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)
nice