larlet-fr-david-cache/cache/e670da5770c6ca884d4915552766f8d0/index.md

title: Real-World JavaScript Anti-Patterns (Part One)
url: http://blog.javascripting.com/2014/11/06/real-world-javascript-anti-patterns/
hash_url: e670da5770c6ca884d4915552766f8d0

<p>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.</p>

<p>Consider this gem:  </p>

<pre><code class="language-javascript">var otherElements = element.classList.contains('my-class') ?  
    element.parentNode.parentNode.parentNode.childNodes :
    element.parentNode.childNodes;
</code></pre>

<p>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.</p>

<p>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:</p>

<pre><code class="language-javascript">MyPage.prototype.resize = function() {  
    this.renderer.view.style.width = window.innerWidth + 'px';
    this.renderer.view.style.height =
        window.innerWidth * 0.48828125 + 'px';
};
</code></pre>

<p>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 <code>0.49</code> 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:  </p>

<pre><code class="language-javascript">const SCALING_FACTOR = 500/1024;  
...
MyPage.prototype.resize() = function() {  
    ...
    this.renderer.view.style.height = 
        window.innerWidth * SCALING_FACTOR + 'px';
}
</code></pre>

<p>Another example is an event handler attached to a button:  </p>

<pre><code class="language-javascript">MyButton.prototype.new = function(e) {  
    var myTarget = e.target;
    ...
};
</code></pre>

<p>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.</p>

<p>Many anti-patterns relate to basic collections and operations. A classic example:  </p>

<pre><code class="language-javascript">items.forEach(function(item) {  
    doSomething(item);
});
</code></pre>

<p>Similar constructs can be seen for promises, click handlers and anywhere where you unthinkingly type the <code>function</code> keyword. Keep it simple:  </p>

<pre><code class="language-javascript">items.forEach(doSomething);  
</code></pre>

<p>The situation is more complicated if you need to invoke an object’s method with the right context. Code like this is very common:  </p>

<pre><code class="language-javascript">var _this = this;  
items.forEach(function(item) {  
    _this.doSomething(item);
});
</code></pre>

<p>Developers tend to forget about the <code>thisArg</code> parameter of <code>forEach</code> and similar functions, which leads to cleaner, clearer code:  </p>

<pre><code class="language-javascript">items.forEach(function(item) {  
    this.doSomething(item);
}, this);
</code></pre>

<p>And once again, if we are calling a single function, we can change it to a one-liner:  </p>

<pre><code class="language-javascript">items.forEach(this.doSomething, this);  
</code></pre>

<p>But what about functions that don’t take a <code>thisArg</code> parameter? Imagine you want to avoid the clumsy <code>_this</code> variable when access an object property inside a jQuery click handler. Assigning to <code>_this</code> is unnecessary, thanks to the underused <code>Function.prototype.bind()</code> method:  </p>

<pre><code class="language-javascript">el.click(function(e) {  
    this.clickCount++;
}.bind(this));
</code></pre>

<p>This isn’t the only practical use of the <code>bind</code> 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:  </p>

<pre><code class="language-javascript">var i, elems = document.getElementsByClassName("myClass");  
</code></pre>

<p>…and the following (wrong) code:</p>

<pre><code class="language-javascript">for (i = 0; i &lt; elems.length; i++) {  
    elems[i].addEventListener("click", function() { 
        this.innerHTML = i;
    });
}
</code></pre>

<p>The traditional solution is to define a factory function to create the click handler.  </p>

<pre><code class="language-javascript">function makeClickHandler(i) {  
    return function() {
        this.innerHTML = i;
    };
}

for (i = 0; i &lt; elems.length; i++) {  
    elems[i].addEventListener("click", makeClickHandler(i)); 
}
</code></pre>

<p>But <code>Function.prototype.bind()</code> is essentially a factory built into the language itself:  </p>

<pre><code class="language-javascript">function handleClick(i) {  
    this.innerHTML = i;
}

for (i = 0; i &lt; elems.length; i++) {  
    elems[i].addEventListener("click", handleClick.bind(this, i));
}
</code></pre>

<p>Back to <code>forEach</code>. It is such common construct that is sometimes over- (and mis-)used:  </p>

<pre><code class="language-javascript">var left = 0, right = 1;  
[left, right].forEach(function(i) {
    ...
}, this);
</code></pre>

<p>Here an old-fashioned <code>for</code> 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 <code>forEach</code>):  </p>

<pre><code class="language-javascript">data = {left: ..., right: ...};  
_.forEach(data, function(item, key) {  
    ...
});
</code></pre>

<p>Another classic example of <code>forEach</code> abuse:  </p>

<pre><code class="language-javascript">var items = [];  
data.forEach(function(dataItem) {  
    var item = dataItem * 4;
    items.push(item);
});
</code></pre>

<p>In this simple example it is easy to see that <code>map</code> would be a better choice:  </p>

<pre><code class="language-javascript">var items = data.map(function(dataItem) {  
    return dataItem * 4;
});
</code></pre>

<p>In more complex cases the correct array method can less obvious:  </p>

<pre><code class="language-javascript">var lowCounter = 0;  
var items = data.map(function(dataItem) {  
    if (dataItem &lt; 10) lowCounter++;
    return dataItem * 5;
});
</code></pre>

<p>Is it appropriate to use <code>map</code> 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 <code>map</code> and one for the side effect:  </p>

<pre><code class="language-javascript">var items = data.map(function(dataItem) { return dataItem*5; });  
var lowCounter = data.reduce(function(total, current) {  
    return total + (current &lt; 10);
}, 0);
</code></pre>

<p>One final example: imagine you are using Lo-Dash/Underscore and want merge an array of objects into a single new object.  </p>

<pre><code class="language-javascript">var confs = [{a: 1, b: 14}, {foo: 6}, ... ]  
</code></pre>

<p>A naive approach might use <code>forEach</code> and <code>_.merge</code>:  </p>

<pre><code class="language-javascript">var result = {};  
confs.forEach(function(item) {  
    _.merge(result, item)
});
</code></pre>

<p>A shorter functional approach:  </p>

<pre><code class="language-javascript">var result = {} confs.forEach(_.partial(_.merge, result));  
</code></pre>

<p>A similar approach using <code>apply</code> (don’t try this with a long array though since each item will be passed to <code>merge</code> as a parameter)  </p>

<pre><code class="language-javascript">var result = _.partial(_.merge, {}).apply(this, confs);  
</code></pre>

<p>Best of all, of course, is <code>reduce</code>:  </p>

<pre><code class="language-javascript">var result = confs.reduce(_.merge, {});  
</code></pre>

<p>Use of <code>_.partial</code> 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 <code>reduce</code> is clearly superior.</p>