Add _.comparator and align sortedIndex to sortBy

[megawac]

Resolves #1768 and fixes #1834

This could use some review and thoughts

[megawac]

LMK if stylistically you prefer above or

   if (_.comparitor(iteratee(array[mid]), value) < 0) low = mid + 1; else high = mid;

[michaelficarra]

It's spelled "comparator".

[jashkenas]

Is the idea that this is something we should be exposing as a public method? If so, what's the use case?

If not, let's make it an internal function.

[michaelficarra]

Why is this exposed? I'd prefer we not expose it.

edit: I didn't refresh the page, and crossed paths with @jashkenas.

[megawac]

@jashkenas @michaelficarra I exposed it because of #1768 (comment)

Also allows some sanity checking unit tests which will be inheritted by sortBy and sortedIndex

[megawac]

Can we get this in/reviewed? I'll rebase this in a minute

[michaelficarra]

LGTM.

[jdalton]

How does this affect the performance of _.sortedIndex, _.max, & _.min?
I have a hunch it's a substantial hit.

Also is comparator the best name?
It shows up in Java docs, is there any other language precedents for a related name?

Reducing duplication and exposing a comparator function is nice, but not critical, so IMO it's not worth a substantial loss in perf, clarity, or anything else really.

[megawac]

I think whatever the hit on sortedIndex is fine for consistency as its doing a binary search anyway, but I agree #1995 definitely needs to be benchmarked

As for the name I'm not partial to comparator, but its definition is spot on for whats happening

[jdalton]

I think whatever the hit on sortedIndex is fine for consistency as its doing a binary search anyway, but I agree #1995 definitely needs to be benchmarked

I've worked pretty hard at maintaining the perf of sortedIndex for my implementation while making the match/sorting consistent. The offhand comment of one dev isn't enough draw for me to include it in my implementation at the moment.

The perf concern for sortedIndex is valid as while experimenting in my own branch a while back I saw some engines may actually perform worse using the binary search for things like _.indexOf than the non-binary option if _.sortedIndex performance is reduced.

[megawac]

Eh, this will need another look I suppose

http://jsperf.com/sortedindex/6

[megawac]

Anyway I'm a big fan of exposing comparator as it allows devs easy
customization for oddball data structures and frankly I've needed a solid
comparator in my own code in the past
On Dec 30, 2014 5:05 AM, "John-David Dalton" [email protected]
wrote:

The perf concern for sortedIndex is valid as while experimenting in my
own branch a whole back I saw some engines may actually perform worse using
the binary search for things like _.indexOf than the non-binary option if
_.sortedIndex performance is reduced.

—
Reply to this email directly or view it on GitHub
#1939 (comment).

[jdalton]

Anyway I'm a big fan of exposing comparator as it allows devs easy
customization for oddball data structures and frankly I've needed a solid
comparator in my own code in the past

That's great but Underscore isn't your personal toy store. These things need a bit more hammering before introducing into core.

Answering my previous question:

How does this affect the performance of _.sortedIndex, _.max, & _.min?

Up to a 73% hit for _.sortedIndex and an 83% hit for _.max & _.min.

[jashkenas]

Right. Let's please back this out for now.

[megawac]

I don't think we need to back this out. We can improve the perf of the implementation for sure and argue over whether it should be exposed or not but I think its a change for the better

[jdalton]

I don't think we need to back this out.

Backing it out isn't the end of the road.
It's a chance to refine your implementation without dragging down core.

[braddunbar]

Anyway I'm a big fan of exposing comparator as it allows devs easy
customization for oddball data structures and frankly I've needed a solid
comparator in my own code in the past

Can you expand on this? Wouldn't you just return a different value or use #sort directly in those cases?

[jashkenas]

To repeat -- let's please back this out for the time being. (I don't want to forget about it.)

[megawac]

@braddunbar lets say you want to to do add support to something similar to pythons class __cmp__

_.mixin({
   comparator: _.wrap(_.compatator, function(baseCompare, a, b) {
     if (_.isFunction(b.__cmp__)) return b.__cmp__(a);
     if (_.isFunction(a.__cmp__) return a.__cmp__(b);
     return baseCompare(a, b);
  })
});