Many of the Enumerable methods accept a context argument. I suppose this interface feels more natural to some, since it allows the invocation of bind to be hidden inside the Enumerable method. In any case, I don't want to argue against the wisdom of context arguments. Since context arguments are here to stay, this patch aims to improve how they are handled.
To understand what I'm getting at, consider Enumerable#eachSlice:
eachSlice: function(number, iterator, context) {
iterator = iterator ? iterator.bind(context) : Prototype.K;
var index = -number, slices = [], array = this.toArray();
while ((index += number) < array.length)
slices.push(array.slice(index, index+number));
return slices.collect(iterator, context);
}
eachSlice binds iterator to the given context, but it does not explicitly call the iterator. Instead, it passes the newly bound iterator and (useless) context to slices.collect, another Enumerable method. Here's Enumerable#collect:
collect: function(iterator, context) {
iterator = iterator ? iterator.bind(context) : Prototype.K;
var results = [];
this.each(function(value, index) {
results.push(iterator(value, index));
});
return results;
}
See what's wrong here? The already-bound iterator gets bound yet again, so that the operation performed on every value-index pair requires three function calls instead of one. At a minimum one might hope to avoid the second bind, but in fact we can do better.
First of all, since eachSlice doesn't actually use iterator or context except to pass them on to slices.collect, there's no reason to call iterator.bind(context) in eachSlice. Not very exciting, but definitely an improvement:
eachSlice: function(number, iterator, context) {
var index = -number, slices = [], array = this.toArray();
while ((index += number) < array.length)
slices.push(array.slice(index, index+number));
return slices.collect(iterator, context);
}
Now what about collect? First let me say that I love Function#bind. It's an elegant solution in a lot of use cases. Here, however, I think it's overkill. The iterator.bind(context) call serves only to fix the context of the iterator. None of the currying functionality of bind is needed. For such a minimal use case, the built-in Function#call is more appropriate (i.e., both prettier and faster):
collect: function(iterator, context) {
iterator = iterator || Prototype.K;
var results = [];
this.each(function(value, index) {
results.push(iterator.call(context, value, index));
});
return results;
}
With these changes, two unnecessary function calls are saved each time the iterator is invoked against a value-index pair. Plus, the code is shorter. Great success.
If you look at my patch, you'll see that similar changes have been applied wherever a context argument is supported. I will grant that the eachSlice/collect case is extreme, but only just. Any opportunity to avoid binding an iterator is a chance to save at least one nested function call for every invocation of the iterator.
Furthermore and finally:
- If the purpose of context arguments is to avoid using bind unnecessarily, then we should avoid using bind unnecessarily. That's a large part of what this patch accomplishes.
- Any object that supports a call method can now be used as an iterator. This insight is actually what got me started thinking about the patch.
- All the Array and Enumerable unit tests still pass.
This ticket began with misgivings about context arguments, but it turned into an argument in their favor. Fancy that :)
Best,
Ben
