After being slightly unfortable 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
});
After being slightly unfortable 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
});
- definitely depends on the JS engine. why don't you benchmark and see for yourself? – Mehrdad Afshari Commented Sep 11, 2009 at 20:57
- Is it possible that the time cost here is function invocation? – spender Commented Sep 11, 2009 at 21:03
- 1 According to the times in the 3rd code block, method f1 looks to be most winningest. Typo? – spender Commented Sep 11, 2009 at 21:06
- Thanks spender I did make a typo - have ammended now, and yes as stated I am unsure of the integretity of my use of console.time, I have done 20 tests now and am clear that the times now represent the code accurately, but am unsure of the fairness of the parison. – Dr. Frankenstein Commented Sep 11, 2009 at 21:17
3 Answers
Reset to default 4If 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:
- You're using implicit global variables.
- 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 plicating 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 pletely.
// 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...
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 plete 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.
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.