Writing Better JavaScript – split up long methods

For the second time this week I’m going to pick on the usually delightful Ext JS library. Last time we discussed the overzealous use of the Module pattern; this time it’s the turn of bloated methods.

As before, I’m not really picking on Ext at all – this happens all over the place. But again, this is the library closest to my heart and the one I know the best.

The Problem

We’re going to take a look at Ext.data.XmlReader’s readRecords method. Before we get started though, I’ll repeat that this is intended as an example of an approach, not a whine at Ext in particular.

/**
 * Create a data block containing Ext.data.Records from an XML document.
 * @param {Object} doc A parsed XML document.
 * @return {Object} records A data block which is used by an {@link Ext.data.Store} as
 * a cache of Ext.data.Records.
 */
readRecords: function(doc) {
  /**
   * After any data loads/reads, the raw XML Document is available for further custom processing.
   * @type XMLDocument
   */
  this.xmlData = doc;
  var root = doc.documentElement || doc;
  var q = Ext.DomQuery;
  var recordType = this.recordType, fields = recordType.prototype.fields;
  var sid = this.meta.idPath || this.meta.id;
  var totalRecords = 0, success = true;
  if(this.meta.totalRecords){
    totalRecords = q.selectNumber(this.meta.totalRecords, root, 0);
  }

  if(this.meta.success){
    var sv = q.selectValue(this.meta.success, root, true);
    success = sv !== false && sv !== 'false';
  }
  var records = [];
  var ns = q.select(this.meta.record, root);
  for(var i = 0, len = ns.length; i < len; i++) {
    var n = ns[i];
    var values = {};
    var id = sid ? q.selectValue(sid, n) : undefined;
    for(var j = 0, jlen = fields.length; j < jlen; j++){
      var f = fields.items[j];
      var v = q.selectValue(Ext.value(f.mapping, f.name, true), n, f.defaultValue);
      v = f.convert(v, n);
      values[f.name] = v;
    }
    var record = new recordType(values, id);
    record.node = n;
    records[records.length] = record;
  }

  return {
    success : success,
    records : records,
    totalRecords : totalRecords || records.length
  };
}

Anyone care to tell me what this actually does? Personally, I have absolutely no idea. I recently found myself needing to implement an XmlReader subclass with a twist which required understanding how this works, and let’s just say it wasn’t easy!

So what is it that makes the above so terrifyingly hard to understand? Well, in no particular order:

• It’s too long – you’d need to be a genius to easily understand what’s going on here
• The variable names don’t make much sense – some of the oddest include ‘q’, ‘ns’, ‘v’, ‘f’ and ’sv’
• There’s minimal commenting – we’re given a single-line clue at the very top as to what these 40-odd lines do

A Solution

Let’s see how the reworked code below addresses each of the concerns above:

• Although we end up with more lines of code here, no single method is more than around 10 LOC
• No single letter variable names – you no longer have to decode what ’sv’ means
• Constructive commenting allows rapid comprehension by skimming the text

One additional and enormous benefit here comes directly from splitting logic into discrete methods. Previously if you’d wanted to implement your own logic to determine success, get the total number of records or even build a record from an XML node you’d be stuck. There was no way to selectively override that logic without redefining that entire monster method.

With our new approach this becomes trivial:

Ext.extend(Ext.data.XmlReader, Ext.data.DataReader, {
  readRecords: function(doc) {
    this.xmlData = doc;

    //get local references to frequently used variables
    var root    = doc.documentElement || doc,
        records = [],
        nodes   = Ext.DomQuery.select(this.meta.record, root);

    //build an Ext.data.Record instance for each node
    Ext.each(nodes, function(node) {
      records.push(this.buildRecordForNode(node));
    }, this);

    return {
      records     : records,
      success     : this.wasSuccessful(root),
      totalRecords: this.getTotalRecords(root) || records.length
    };
  },

  /**
   * Returns a new Ext.data.Record instance using data from a given XML node
   * @param {Element} node The XML node to extract Record values from
   * @return {Ext.data.Record} The record instance
   */
  buildRecordForNode: function(node) {
    var domQuery = Ext.DomQuery,
        idPath   = this.meta.idPath || this.meta.id,
        id       = idPath ? domQuery.selectValue(idPath, node) : undefined;

    var record  = new this.recordType({}, id);
    record.node = node;

    //iterate over each field in our record, find it in the XML node and convert it
    record.fields.each(function(field) {
      var mapping  = Ext.value(field.mapping, field.name, true),
          rawValue = domQuery.selectValue(mapping, node, field.defaultValue),
          value    = field.convert(rawValue, node);

      record.set(field.name, value);
    });

    return record;
  },

  /**
   * Returns the total number of records indicated by the server response
   * @param {XMLDocument} root The XML response root node
   * @return {Number} total records
   */
  getTotalRecords: function(root) {
    var metaTotal = this.meta.totalRecords;

    return metaTotal == undefined
                      ? 0
                      : Ext.DomQuery.selectNumber(metaTotal, root, 0);
  },

  /**
   * Returns true if the response document includes the expected success property
   * @param {XMLDocument} root The XML document root node
   * @return {Boolean} True if the XML response was successful
   */
  wasSuccessful: function(root) {
    var metaSuccess  = this.meta.success;

    //return true for any response except 'false'
    if (metaSuccess == undefined) {
      return true;
    } else {
      var successValue = Ext.DomQuery.selectValue(metaSuccess, root, true);
      return successValue !== false && successValue !== 'false';
    }
  }
});

(For brevity I have omitted the existing readRecords comment blocks from the above)

I suggest that you structure your code in this way at least 99% of the time. The one exception is if high performance is an issue. If you are in a situation where every millisecond counts (you probably aren’t), then taking the former route becomes more acceptable (though there’s still no excuse for not adding a few comments explaining what the code actually does).

My refactored code almost certainly runs slower than the original as it doesn’t take as much advantage of cached local variables as the monolithic version does. For library-level code this can make sense if the performance gain is significant, but for the everyday code you and I write it is rarely a good idea.

I’ll be watching.