How can I refactor this JavaScript code to avoid making functions in a loop?

Question

I wrote the following code for a project that I'm working on:

var clicky_tracking = [
  ['related-searches', 'Related Searches'],
  ['related-stories', 'Related Stories'],
  ['more-videos', 'More Videos'],
  ['web-headlines', 'Publication']
];

for (var x = 0, length_x = clicky_tracking.length; x < length_x; x++) {
  links = document.getElementById(clicky_tracking[x][0])
                  .getElementsByTagName('a');
  for (var y = 0, length_y = links.length; y < length_y; y++) {
    links[y].onclick = (function(name, url) {
      return function() {
        clicky.log(url, name, 'outbound');
      };
    }(clicky_tracking[x][1], links[y].href));
  }
}

What I'm trying to do is:

• define a two-dimensional array, with each instance the inner arrays containing two elements: an id attribute value (e.g., "related-searches") and a corresponding description (e.g., "Related Searches");
• for each of the inner arrays, find the element in the document with the corresponding id attribute, and then gather a collection of all <a> elements (hyperlinks) within it;
• loop through that collection and attach an onclick handler to each hyperlink, which should call clicky.log, passing in as parameters the description that corresponds to the id (e.g., "Related Searches" for the id "related-searches") and the value of the href attribute for the <a> element that was clicked.

Hopefully that wasn't thoroughly confusing! The code may be more self-explanatory than that.

I believe that what I've implemented here is a closure, but JSLint complains:

http://img.skitch.com/20100526-k1trfr6tpj64iamm8r4jf5rbru.png

So, my questions are:

• How can I refactor this code to make JSLint agreeable? Or, better yet, is there a best-practices way to do this that I'm missing, regardless of what JSLint thinks?
• Should I rely on event delegation instead? That is, attaching onclick event handlers to the document elements with the id attributes in my arrays, and then looking at event.target? I've done that once before and understand the theory, but I'm very hazy on the details, and would appreciate some guidance on what that would look like - assuming this is a viable approach.

Thanks very much for any help!

Answer 1

This code isnt very maintainable! I hate shoving jQuery down your throat but this looks like a situation where is would be very useful to you, something like:

$('a.trackable').click(function(e) {

  clicky.log(url, name, 'outbound');
};

You could enable tracking by adding class 'trackable' to each link, and map links to a lookup table by using eg $(this).attr('rel').

Hope that makes sense.

Answer 2

You could create a function to build the event handlers, e.g.:

function createClickHandler(url, name) {
  return function () {
    clicky.log(url, name, 'outbound');
  };
}

for (var x = 0, length_x = clicky_tracking.length; x < length_x; x++) {
  links = document.getElementById(clicky_tracking[x][0]) // NOTE:links should be 
                  .getElementsByTagName('a');            // declared at the top
  for (var y = 0, length_y = links.length; y < length_y; y++) {
    links[y].onclick = createClickHandler(clicky_tracking[x][1], links[y].href);
  }
}

I also think that event delegation is also a really good option, you can implement it very easily:

var clicky_tracking = [
  ['related-searches', 'Related Searches']
  //...
], elem;

function createClickHandler(name) { // capture only the 'name' variable
  return function (e) {
    e = e || window.event; // cross-browser way to get the event object
    var target = e.target || e.srcElement; // and the event target

    if (target.nodeName == "A") { // make sure that the target is an anchor
      clicky.log(target.href, name, 'outbound');
    }
  };
}

for (var x = 0, len = clicky_tracking.length; x < len; x++) {
  elem = document.getElementById(clicky_tracking[x][0]); // find parent element
  elem.onclick = createClickHandler(clicky_tracking[x][1]); // bind the event
}
​

Answer 3

Delegation is a better approach. JSLint complains because you are creating a new function each time within the loop. However, instead of setting up a single event on the document to listen for all click events, I would rather setup a handler on all individual root elements that have id's assigned to them. A single handler can be reused for all these elements.

function logClick(event) {
    event = event || window.event;
    var link = event.target || event.srcElement;

    if(link.nodeName.toLowerCase() !== "a") {
        return;
    }

    var name = clicky_tracking[this.id];
    clicky.log(link.href, name, 'outbound');
}

Register the above handler with each root element.

for(var id in clicky_tracking) {
    var root = document.getElementById(id);
    root.onclick = logClick;
}

Also to avoid searching through the array, I've changed clicky_tracking from array to an object for easier keyed access.

var clicky_tracking = {
  'related-searches': 'Related Searches',
  'related-stories': 'Related Stories',
  'more-videos': 'More Videos',
  'web-headlines': 'Publication'
};