#Lessons From A JavaScript Code Review
I was recently asked to review some code 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: (function() {
return window.XMLHttpRequest ?
new XMLHttpRequest:
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: 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)