views:

2342

answers:

3

After becoming slightly uncomfortable with multiple calls to the same function, with different parameters (as shown in the dom ready section of the code below), I decided to test out passing the parameters for this function by iterating through an array instead (as shown in mouse_function_two). To my surprise I found that mouse_function_two ran considerably faster.

My question is, would mouse_function_two be considered better JavaScript practice than mouse_function_one?

Note: I am slightly concerned I may not be using firebugs console.time utility correctly!

Initial attempt

function mouse_function_one (hover_selector, class_name, add_link) {

  hover_item = $(hover_selector)

  hover_item.each(function(){
    $(this).hover(
      function () {
        $(this).addClass(class_name);
        if ( add_link == true ) {
          $(this).click(function(){
            var href = $(this).find('a').attr('href');
            window.location.href = href;
          });

        }
        else return false;
      },
      function () {
        $(this).removeClass(class_name);
    });
  });
}

Second (faster) attempt:

function mouse_function_two (args) {

  for (var i=0; i < args.length; i++) {

    hover_selector = $(args[i][0]);
    class_name = args[i][1];
    add_link = args[i][2];

    hover_item = $(hover_selector)

    hover_selector.each(function(){
      $(this).hover(
        function () {
          $(this).addClass(class_name);
          if ( add_link == true ) {
            $(this).click(function(){
              var href = $(this).find('a').attr('href');
              window.location.href = href;
            });

          }
          else return false;
        },
        function () {
          $(this).removeClass(class_name);
        });
    });

  }
}

Client code:

// on dom ready
$(function(){

  console.time('f1');
  mouse_function_one('#newsletter .right', 'hovered', true);
  mouse_function_one('.block-couple div.right', 'hovered', false);
  mouse_function_one('.bulletin', 'selected', true);
  console.timeEnd('f1'); //speed is calculated as 104ms

  args = [];
  args[0] = ['#newsletter .right', 'hovered', true]; 
  args[1] = ['.block-couple div.right', 'hovered', false]; 
  args[2] = ['.bulletin', 'selected', true]; 

  console.time('f2');
  mouse_function_two(args);
  console.timeEnd('f2'); //speed is calculated as 10ms

});
+3  A: 

Is it a bottleneck piece of code? Using arrays as args opens you up to a whole category of bugs that named parameters protect you from. The 2nd code reads pretty badly, and if not already, given time, will prove to be a complete arse to debug. So, unless it's critical, keep it sane. Only when it's really problematic code should you start throwing away sensible language features to scratch out a bit of speed.

EDIT: The more I look at the code, the less balanced your benchmark seems. There are a number of things that you have not factored out that might lead one to a different conclusion such as the cost of repeated method invocation over a single invocation and that filling the arrays etc is not benchmarked in you second case.

spender
A: 

I'd consider passing a JSON object in your case. Frameworks like ExtJs and JQuery rely on these heavily as doing such gives you a lot of flexibility (esp. as your API evolves) and it lets you pack a lot of data in a single object.

function mouse_function_three (args) { ... }

where args looks like ...

[{'hover_selector': ... , 'class_name': ... , 'add_link': ...}, {'hover_selector': ... , 'class_name': ... , 'add_link': ...}, ... ]

you could even extend this by including the ability to specify default properties for each object's property as such:

{
'default_hover_selector': 'asdf',
'default_class': 'asdf',
'add_link: false,
'items': [{'hover_selector': ... , 'class_name': ... , 'add_link': ...}, {'hover_selector': ... , 'class_name': ... , 'add_link': ...}, ... ]
}

I think this solution will give you the performance AND intuitivity (is that even a word) you want.

wgpubs
I tried using objects before settling for an array, but I was not quite sure how to go about it, this technique is appealing, especially with the default properties. I'll give it a shot.
Dr. Frankenstein
+4  A: 

If the second routine is faster, it's probably because it doesn't do what it is supposed to do. Have a look at this snippet:

  for (var i=0; i < args.length; i++) {

    hover_selector = $(args[i][0]);
    class_name = args[i][1];
    add_link = args[i][2];

    hover_item = $(hover_selector)

    hover_selector.each(function(){
      $(this).hover(
        function () {

Two problems here:

  1. You're using implicit global variables.
  2. Blocks in JavaScript do not introduce a new scope.

Either of these would cause the same bug, together they just make it doubly certain that it will occur: the closures created for the hover() event handler functions contain only the values of the final loop iteration. When these handlers are finally called, class_name will always be "selected", and add_link will always be true. In contrast, your original function would be called with a different set of parameters each time, which would be captured in the function's scope by the event handlers, and consequently work as expected.


As for the style... It's messy. You've encased the entire function body in a loop, removed descriptive arguments, and greatly complicating the calling of the function.

Fortunately, you can address the issue I point out above, simplify the function, and simplify how it is called all in one go:

// For starters, I've eliminated explicit parameters completely.
// args wasn't descriptive, and all JavaScript functions have an implicit
// arguments array already - so let's just use that.
function mouse_function_three () {

  // use jQuery's array iteration routine:
  // this takes a function as an argument, thereby introducing scope and
  // avoiding the problem identified previously
  $.each(arguments, function() {
    var class_name = this.class_name;
    var add_link = this.add_link;
    var selected = $(this.hover_selector);

    // no need to use selected.each() - jQuery event binding routines
    // always bind to all selected elements
    selected.hover(function() {
      $(this).addClass(class_name);
    },
    function() {
      $(this).removeClass(class_name);
    });

    // bring this out of the hover handler to avoid re-binding
    if ( add_link ) {
      $(selected).click(function(){
        var href = $(this).find('a').attr('href');
        window.location.href = href;
      });

    }
  }); // each set of arguments
}

You'd then call this new routine like so:

console.time('f3');
mouse_function_three( 
 {hover_selector: '#newsletter .right', class_name: 'hovered', add_link: true},
 {hover_selector: '.block-couple div.right', class_name: 'hovered', add_link: false},
 {hover_selector: '.bulletin', class_name: 'selected', add_link: true}
);
console.timeEnd('f3');

Note that these changes may very well eliminate any speed difference from your initial attempt, as the code effectively does the very same thing, but with the additional step of packing and then extracting parameters...

Shog9
Thank you Shog9, this is my first question on stackoverflow, I am amazed by the responses and detail shown here and this answer has really hit the nail on the head, I should edit the question to forget about speed and go for best practice which is clearly what is evident in your response. Though in doing so, I will knock out the context of most of the responses. For the record this clocked in at 94ms, though I think this is immaterial as we are comparing it to unreliable code, without a thorough understanding (on my part) of how to properly benchmark these functions.
Dr. Frankenstein
+1 so much great explanation packed into one answer.
Daniel Earwicker