Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

another take at allowing for array of iteratees in sortBy #1815

Closed
wants to merge 4 commits into from

Conversation

krawaller
Copy link

This pull request takes another stab at the enhancement of sortBy that was suggested in #1359, namely allowing an array of iteratees. This implementation complements #1359 so that each entry in the iteratees array is run through _.iteratee, so it can contain both property names and functions.

Here's a silly example sorting on both name and age (but using a function for the latter):

var people = [{name : 'curly', age : 50},{name: 'moe', age: 30},{name : 'curly', age : 27}];
_.sortBy(people, ['name',function(p){return p.age;}] );
// => [{name : 'curly', age : 27},{name : 'curly', age : 50},{name: 'moe', age: 30}];

I've toyed with some different approaches to this problem, including what's been suggested in #1751, and I think this is the most intuitive answer. Although it doesn't directly cover all use cases for the #1751 version, such as @megawac 's semver example:

var versions = ["0.1", "0.1.1", "2.0", "10.1", "6.2", "2.0.1", "1.7.3", "1.10.2", "0.2.0", "0.3.0", "0.6.0", "1.0.0"];
_.sortBy(versions, function(version) {
   return _.map(version.split("."), _.partial(parseInt, _, 10));
});
// => ["0.1", "0.1.1", "0.2.0", "0.3.0", "0.6.0", "1.0.0", "1.7.3", "1.10.2", "2.0", "2.0.1", "6.2", "10.1"]

Catering cleanly to that too would be nice, but I suspect simply sorting on multiple fields is by far the most common use case.

@jdalton
Copy link
Contributor

jdalton commented Aug 29, 2014

I think this fits better as a separate method like _.sortByArray.

@krawaller
Copy link
Author

Because of performance, or do you think the API gets too messy?

@jdalton
Copy link
Contributor

jdalton commented Aug 29, 2014

Both, though the main one being muddying the API.

@krawaller
Copy link
Author

Done some more thinking. Although allowing a param to be one or many is pretty straight-forward, I agree there is a value to having a more defined signature.

But sorting on more than one column is such a common use case, and so well understood, I'd argue that this mitigates the muddying cost. For many, I think NOT allowing multiple columns feels unintuitive.

Having it as a separate function would work, I guess, but to me that just feels like a different kind of mud. Instead of muddying the sortBy API, we'd be muddying the API of Underscore.

So, my chain of thought; multiple column sort is commonplace and expected to be there, so inclusion is probably warranted. That means a more complex API, and since the new functionality is so closely tied to sorting on a single column, it seems better to restrict the muddying to sortBy.

Performance might be a strong counter argument, though. My way would incur a performance penalty on all sorting, while having multiple column sorting as a separate method mitigates that.

@jashkenas
Copy link
Owner

This is nice to compare, but I think that the approach in #1751 is currently a little bit more likely of a candidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants