#Why it's bad.
So, the problem with this...
function formatCurrency (data) {
// format germany price
var els = document.querySelectorAll('.price');
for (var i=0; i<els.length; i++) {
var price = parseInt(els[i].innerHTML.replace(/[^0-9]*/,''));
var curr = els[i].getAttribute('data-currency');
var newPrice = price / data.rates.CAD * data.rates[curr];
els[i].innerHTML = '<small>' + currencyLookup[curr] + '</small>' + Math.round(newPrice);
}
}
Is best demonstrated with this similar function (assuming a polyfill normalizes quirks):
function mapDataToElements(data, elements) {
var els = document.querySelectorAll(elements);
//JavaScript is lexically scoped, not block scoped, the VARs should
//end up here for readability, as they're hoisted here anyway.
var content, curr, newContent;
for (var i=0; i<els.length; i++) {
//This invokes something like the HTML parser, it's actually the inverse, it's not *too*
//bad to use, but generally something that's best avoided unless you truthfully
//need to manipulate an excsssive number of nodes and some complex HTML.
content = els[i].innerHTML;
//Use of DOM API for working with values like this is actually a good thing.
curr = els[i].getAttribute('data-currency');
newContent = curr + data.foo; //Assuming data.foo is valid input
//So there's two issues with this line, lets actually take a look at it in the next comment block...
els[i].innerHTML = '<small>' + GLOBALLookupVar[curr] + '</small>' + newContent;
}
}
So let us take a look at this line:
els[i].innerHTML = '<small>' + GLOBALLookupVar[curr] + '</small>' + newContent;
This line does three things:
- Invoke HTML parser n-times for minor dom manipulation
- Calls upon a global variable (bad!) to lookup data, rather than pass it, or curry a function with callbacks
- Manipulates the dom as if it's a string. This is a form of Eval.
This is what an HTML parser looks like (well, the tokenizer specifically anyway): http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLTokenizer.cpp#L191
Every time you call upon .innerHTML you've invoked the entire document HTML parser. Typical reaction to this > "Yeah, big deal. I'm getting things done."
There's serious problems with using this.
- This is amongst the top reasons why XSS exploits are so common (source: OWASP)
- The parser is doing what you should do in the first place, except, it's very expensive (we're invoking a state machine really that tokenizes strings): Creating DOM nodes/elements and creating pointers between them in a graph-like structure which is dropped into a DOM Fragment and finally appended to the document (early browsers re-render the whole document ).
- Causes reflow: Lots of reflow is happening, sometimes for minor tweaks. This creates unresponsive pages.
- Blocking. Because of the rapid succession of calls to the DOM parsing, we've consumed massive resources, then we're modifying the DOM (which is actually quite fast). The solution doesn't scale.
- Possibility of crashing for large selections. Try his code in ie7 or ie8, maybe even ie9 for a document where the class is actually a column on a table with a few thousand rows. This is precisely why awesome plugins like DataTables crap out on large data sets: they just abuse the DOM through invoking the HTML parser inside a loop.
- Mixes seperation of concerns; maintainability/readability suffer. It's obviously a data structure that's being manipulated, but we're mixing our HTML with our JavaScript in this case. Client-side templates rarely make sense.
All of these problems can be avoided if you simply use the DOM API, or a library that is conscious of it.