jQuery/DOM/best-practices discussion with Ryan McGreal

.md

#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:

1. Invoke HTML parser n-times for minor dom manipulation
2. Calls upon a global variable (bad!) to lookup data, rather than pass it, or curry a function with callbacks
3. 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.

1. This is amongst the top reasons why XSS exploits are so common (source: OWASP)
2. 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 ).
3. Causes reflow: Lots of reflow is happening, sometimes for minor tweaks. This creates unresponsive pages.
4. 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.
5. 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.
6. 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.