Lessons From A JavaScript Code Review
On to the review.
A developer recently wrote in, asking me to review their code and
provide some useful suggestions on how they might improve it. While I’m
certainly not an expert on reviewing code (don’t let the above fool
you), here are the problems and solutions that I proposed.
Problem 1
Problem: Functions and objects are passed as arguments to other functions without any type validation.
Feedback: Type validation is an essential step in
ensuring that you’re working only with input of a desired type. Without
sanitization checks in place, you run the risk of users passing in just
about anything (a string, a date, an array, etc.), which could easily
break your application if you haven’t developed it defensively. For
functions, you should do the following at a minimum:
- Test to ensure that arguments being passed actually exist,
- Do a
typeof check to prevent the app from executing input that is not a valid function at all.
if (callback && typeof callback === "function"){
/* rest of your logic */
}else{
/* not a valid function */
}
Unfortunately, a simple
typeof check
isn’t enough on its own. As Angus Croll points out in his post “
Fixing the typeof operator,” you need to be aware of a number of issues with
typeof checking if you’re using them for anything other than functions.
For example, typeof null returns object, which is technically incorrect. In fact, when typeof is applied to any object type that isn’t a function, it returns object, not distinguishing between Array, Date, RegEx or whatever else.
The solution is to use 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 you can force the generic toString function on them:
Object.prototype.toString.call([1,2,3]); //"[object Array]"
You might also find Angus’s function below useful as a more reliable alternative to typeof. Try calling betterTypeOf() against objects, arrays and other types to see what happens.
function betterTypeOf( input ){
return Object.prototype.toString.call(input).match(/^\[object\s(.*)\]$/)[1];
}
Here, parseInt() is being blindly used to parse an integer value of user input, but no base is specified. This can cause issues.
In
JavaScript: The Good Parts, Douglas Crockford refers to
parseInt()
as being dangerous. Although you probably know that passing it a string
argument returns an integer, you should also ideally specify a base or
radix as the second argument, otherwise it might return unexpected
output. Take the following example:
parseInt('20'); // returns what you expect, however…
parseInt('020'); // returns 16
parseInt('000020'); // returns 16
parseInt('020', 10); // returns 20 as we've specified the base to use
You’d be surprised by how many developers omit the second argument,
but it happens quite regularly. Remember that your users (if permitted
to freely enter numeric input) won’t necessarily follow standard number
conventions (because they’re crazy!). I’ve seen 020, ,20, ;'20
and many other variations used, so do your best to parse as broad a
range of inputs as possible. The following tricks to using parseInt() are occasionally better:
Math.floor("020"); // returns 20
Math.floor("0020"); //returns 20
Number("020"); //returns 20
Number("0020"); //returns 20
+"020"; //returns 20
Problem 2
Problem: Checks for browser-specific conditions
being met are repeated throughout the code base (for example, feature
detection, checks for supported ES5 features, etc.).
Feedback: Ideally, your code base should be as DRY
as possible, and there are some elegant solutions to this problem. For
example, you might benefit from the load-time configuration
pattern here (also called load-time and init-time branching). The basic
idea is that you test a condition only once (when the application
loads) and then access the result of that test for all subsequent
checks. This pattern is commonly found in JavaScript libraries that
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 - eg. for IE */
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’);
}
}
For a great example, Stoyan Stefanov applies 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 3
Problem: The native Object.prototype is being extended regularly.
Feedback: Extending native types is generally frowned upon, and few (if any) popular code bases should dare to extend Object.prototype.
The reality is that there is not likely a situation in which you
absolutely need to extend it in this way. In addition to breaking the
object-as-hash tables in JavaScript and increasing the chance of naming
collisions, it’s generally considered bad practice, and modifying it
should only be a last resort (this is quite different from extending
your own custom object properties).
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 is aware why it’s necessary. You can use
the following code sample as a guide:
if(typeof Object.prototype.myMethod != ’function’){
Object.prototype.myMethod = function(){
//implem
};
}
Problem 4
Problem: Some of the code is heavily blocking the
page because it’s either waiting on processes to complete or data to
load before executing anything further.
Feedback: Page-blocking makes for a poor user
experience, and there are a number of ways to work around it without
impairing the application.
One solution is to use “deferred execution” (via promises and
futures). The basic idea with promises is that, rather than issuing
blocking calls for resources, you immediately return a promise for a
future value that will eventually be fulfilled. This rather easily
allows you to write non-blocking logic that can be run asynchronously.
It is common to introduce callbacks into this equation that execute once
the request completes.
I’ve written a relatively comprehensive
post
on this with Julian Aubourg, if you’re 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 and 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,
then here is Douglas Crockford’s implementation of promises:
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 5
Problem: You’re testing for explicit numeric equality of a property using the == operator, but you should probably be using === instead
Feedback: As you may or may not know, the identity ==
operator in JavaScript is fairly liberal and considers values to be
equal even if they’re of completely different types. This is due to the
operator forcing a coercion of values into a single type (usually a
number) prior to performing any comparison. The === operator will, however, not do this conversion, so if the two values being compared are not of the same type, then === 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 and is considered to be unreliable by many developers.
You might also be interested to know that in abstractions of the language, such as CoffeeScript, the == operator is completely dropped in favor of === beneath the hood due to the former’s unreliability.
Rather than take my word for it, see the examples below of boolean checks for equality using ==, some of which result in rather unexpected outputs.
3 == "3" // true
3 == "03" // true
3 == "0003" // true
3 == "+3" //true
3 == [3] //true
3 == (true+2) //true
’ \t\r\n ’ == 0 //true
"\t\r\n" == 0 //true
"\t" == 0 // true
"\t\n" == 0 // true
"\t\r" == 0 // true
" " == 0 // true
" \t" == 0 // true
" \ " == 0 // true
" \r\n\ " == 0 //true
The reason that many of the (stranger) results in this list evaluate to
true is because JavaScript is a weakly typed language: it applies type coercion
wherever possible. If you’re interested in learning more about why some of the expressions above evaluate to
true, look at the
Annotated ES5 guide, whose explanations are rather fascinating.
Back to the review. If you’re 100% certain that the values being
compared cannot be interfered with by the user, then proceed with using
the == operator with caution. Just remember that === covers your bases better in the event of an unexpected input.
Problem 6
Problem: An uncached array length is being used in all for loops. This is particularly bad because you’re using it when iterating through an HTMLCollection.
Here’s an example:
for( var i=0; i
Feedback: The problem with this approach (which I still see a number of developers using) is that the array
length
is unnecessarily re-accessed on each loop’s iteration. This can be very
slow, especially when working with HTMLCollections (in which case,
caching the
length can be anywhere up to 190 times faster than repeatedly accessing it, as Nicholas C. Zakas mentions in his book
High-Performance JavaScript). Below are some options 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 that compares the performance benefits of caching the array
length inside and outside the loop, using prefix increments, counting down and more is also
available, if you would like to study which performs the best.
Problem 7
Problem: jQuery’s $.each() is being used to iterate over objects and arrays, in some cases while for is being used in others.
Feedback: In jQuery, we have two ways to seamlessly iterate over objects and arrays. The generic
$.each iterates over both of these types, whereas
$.fn.each() iterates over a jQuery object specifically (where standard objects can be wrapped with
$() should you wish to use them with the latter). While the lower-level
$.each performs better than
$.fn.each(), both standard JavaScript
for and
while loops perform much better than either, as proven by this
jsPerf test. Below are some examples of loop alternatives that also perform better:
/* jQuery $.each */
$.each(a, function() {
e = $(this);
});
/* classic for loop */
var len = a.length;
for ( var i = 0; i < len; i++ ) {
//if this must be a jQuery object do..
e = $(a[i]);
//otherwise just e = a[i] should suffice
};
/* reverse for loop */
for ( var i = a.length; i-- ) {
e = $(a[i]);
}
/* classic while loop */
var i = a.length;
while (i--) {
e = $(a[i]);
}
/* alternative while loop */
var i = a.length - 1;
while ( e = a[i--] ) {
$(e)
};
Given that this is a data-centric application with a potentially
large quantity of data in each object or array, you should consider a
refactor to use one of these. From a scalability perspective, you want
to shave off as many milliseconds as possible from process-heavy
routines, because these can build up when hundreds or thousands of
elements are on the page.
Problem 8
Problem: JSON strings are being built in-memory using string concatenation.
Feedback: This could be approached in more optimal ways. For example, 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"}]}]}}
*/
As an extra debugging tip, if you would like to pretty-print JSON in
your console for easier reading, then the following extra arguments to stringify() will achieve this:
JSON.stringify({ foo: "hello", bar: "world" }, null, 4);
Problem 9
Problem: The namespacing pattern used is technically invalid.
Feedback: While namespacing is implemented correctly
across the rest of the application, the initial check for namespace
existence is invalid. Here’s what you currently have:
if ( !MyNamespace ) {
MyNamespace = { };
}
The problem is that !MyNamespace will throw a ReferenceError, because the MyNamespace
variable was never declared. A better pattern would take advantage of
boolean conversion with an inner variable declaration, as follows:
if ( !MyNamespace ) {
var MyNamespace = { };
}
//or
var myNamespace = myNamespace || {};
// Although a more efficient way of doing this is:
// myNamespace || ( myNamespace = {} );
// jsPerf test: http://jsperf.com/conditional-assignment
//or
if ( typeof MyNamespace == ’undefined’ ) {
var MyNamespace = { };
}
Courtesy : SmashingMag
