Code reviews are a perfect opportunity to continue learning and improving your coding style. I work with a team of experienced developers, so I don’t see many really glaring errors. But when I am called on to review third-party code, I’ve noticed certain bad practices crop up over and over again. Some of these may seem obvious, but hopefully there will be a useful tip or two here for JavaScript developers of every skill level. All examples are based on real production code I’ve reviewed, although (variable) names have been changed to protect the innocent.
Consider this gem:
var otherElements = element.classList.contains('my-class') ?
element.parentNode.parentNode.parentNode.childNodes :
element.parentNode.childNodes;
The project in question already included jQuery, so one obvious improvement would be to use selectors instead of the execrable DOM API. But that doesn’t address the fundamental issue with this code: it will break if the slightest change is made to the associated markup. The best solution would be to rethink the approach entirely, perhaps using HTML classes to assign semantics to specific elements instead of chaining DOM calls.
Let’s look at some more subtle examples. Even small changes can improve code readability and robustness. This is especially significant when new developers join a project that is already underway. They will get up to speed more quickly and make less mistakes if they don’t need to decipher the existing code base. Clearer code will also prevent them from trying to find hidden meaning where there is none. Consider this event handler:
MyPage.prototype.resize = function() {
this.renderer.view.style.width = window.innerWidth + 'px';
this.renderer.view.style.height =
window.innerWidth * 0.48828125 + 'px';
};
Do we need to be so precise? Probably the developer just copied and pasted a constant from their calculator. The less significant digits are completely useless and serve only to obfuscate the intent of the code. Just writing 0.49 would be an improvement but still doesn’t make it clear what is going on. It would be much better to assign the number to a constant while making it clear how it was derived:
const SCALING_FACTOR = 500/1024;
...
MyPage.prototype.resize() = function() {
...
this.renderer.view.style.height =
window.innerWidth * SCALING_FACTOR + 'px';
}
Another example is an event handler attached to a button:
MyButton.prototype.new = function(e) {
var myTarget = e.target;
...
};
That’s right, a function declared on a prototype can be named using a reserved word. Surprisingly this works, but to say it’s confusing would be an understatement. Clear and careful naming is as important as code structure itself.
Many anti-patterns relate to basic collections and operations. A classic example:
items.forEach(function(item) {
doSomething(item);
});
Similar constructs can be seen for promises, click handlers and anywhere where you unthinkingly type the function keyword. Keep it simple:
items.forEach(doSomething);
The situation is more complicated if you need to invoke an object’s method with the right context. Code like this is very common:
var _this = this;
items.forEach(function(item) {
_this.doSomething(item);
});
Developers tend to forget about the thisArg parameter of forEach and similar functions, which leads to cleaner, clearer code:
items.forEach(function(item) {
this.doSomething(item);
}, this);
And once again, if we are calling a single function, we can change it to a one-liner:
items.forEach(this.doSomething, this);
But what about functions that don’t take a thisArg parameter? Imagine you want to avoid the clumsy _this variable when access an object property inside a jQuery click handler. Assigning to _this is unnecessary, thanks to the underused Function.prototype.bind() method:
el.click(function(e) {
this.clickCount++;
}.bind(this));
This isn’t the only practical use of the bind function. Less used but no less useful is the ability to create partials with fixed argument values. Remember the dreaded lint error “Don’t make functions within a loop”? Imagine you have an array of DOM elements:
var i, elems = document.getElementsByClassName("myClass");
…and the following (wrong) code:
for (i = 0; i < elems.length; i++) {
elems[i].addEventListener("click", function() {
this.innerHTML = i;
});
}
The traditional solution is to define a factory function to create the click handler.
function makeClickHandler(i) {
return function() {
this.innerHTML = i;
};
}
for (i = 0; i < elems.length; i++) {
elems[i].addEventListener("click", makeClickHandler(i));
}
But Function.prototype.bind() is essentially a factory built into the language itself:
function handleClick(i) {
this.innerHTML = i;
}
for (i = 0; i < elems.length; i++) {
elems[i].addEventListener("click", handleClick.bind(this, i));
}
Back to forEach. It is such common construct that is sometimes over- (and mis-)used:
var left = 0, right = 1;
[left, right].forEach(function(i) {
...
}, this);
Here an old-fashioned for loop would be a better choice. You might protest that the author’s choice of variables names is intended to make the code’s semantics more apparent. In this case you can use a code comment or choose a more appropriate data representation. Which representation depends on the specific code, but something like the following is clearly superior to the previous example (note the use of the more powerful Lo-Dash/Underscore forEach):
data = {left: ..., right: ...};
_.forEach(data, function(item, key) {
...
});
Another classic example of forEach abuse:
var items = [];
data.forEach(function(dataItem) {
var item = dataItem * 4;
items.push(item);
});
In this simple example it is easy to see that map would be a better choice:
var items = data.map(function(dataItem) {
return dataItem * 4;
});
In more complex cases the correct array method can less obvious:
var lowCounter = 0;
var items = data.map(function(dataItem) {
if (dataItem < 10) lowCounter++;
return dataItem * 5;
});
Is it appropriate to use map here, considering that the loop has an unrelated side effect? Unless the array is large enough for performance to be a consideration, it would probably be better to use two separate calls, one for the map and one for the side effect:
var items = data.map(function(dataItem) { return dataItem*5; });
var lowCounter = data.reduce(function(total, current) {
return total + (current < 10);
}, 0);
One final example: imagine you are using Lo-Dash/Underscore and want merge an array of objects into a single new object.
var confs = [{a: 1, b: 14}, {foo: 6}, ... ]
A naive approach might use forEach and _.merge:
var result = {};
confs.forEach(function(item) {
_.merge(result, item)
});
A shorter functional approach:
var result = {} confs.forEach(_.partial(_.merge, result));
A similar approach using apply (don’t try this with a long array though since each item will be passed to merge as a parameter)
var result = _.partial(_.merge, {}).apply(this, confs);
Best of all, of course, is reduce:
var result = confs.reduce(_.merge, {});
Use of _.partial makes the code shorter but arguably harder to read. Less code isn’t always better when the result is less understandable. Fortunately we don’t need to weigh the trade-offs in this case since the final variant using reduce is clearly superior.