sortBy comparator handles arrays

[megawac]

Fixes #1779, #1880

Another try at getting sortBy support for multiple criteria since #1359 was rejected. I'd argue this approach is more robust as its implementation is based on the return of the iterator instead of only handling multi prop lookups. Thus we can implement the same behaviour as #1359 with a properties mixin if desired later.

_.mixin({
   properties: function(props) {
     return function(obj) {
         return _.map(props, function(prop) {
             return obj[prop];
         });
     }
  }
});

_.sortBy([{a: 1, b: 2}, {a: 1, b: 3}, {a: 1}, {a: 2, b: -1}, {b: 5}], _.properties(["a", "b"]));

---

Post implementation enables:

_.sortBy([{a: 1, b: 2}, {a: 1, b: -1}, {a: 1, b: 3}, {a: 2, b: -5}], function(x) {
    return [x.a, x.b];
});
// => [{"a": 1,"b": -1},{"a": 1,"b": 2},{"a": 1,"b": 3},{"a": 2,"b": -5}]

---

Exmple Usage (naive semver sort):

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"]

---

The thing that tripped me up implementing this is that the comparitor ranks undefined > anything. I'd argue that undefined < anything but that's just me.

I'd really like to see this behaviour in v2.

[jdalton]

I'm on the fence on this one (though leaning more towards 👎). Most other methods that use an iterator to resolve the computed value (like _.max, _.min, _.sortedIndex, _.uniq) don't have special behaviors based on the return value (in this case array).

[jdalton]

Also this implementation prioritizes the specialized case (sorting by multiple properties) over the common case (not sure if there's a way around that other than a separate function or mixin which may be more appropriate anyway).

[akre54]

Yeah this is very cool but probably not all that common. sortBy already does a lot of work, let's leave it as a mixin or contrib method for now.

[megawac]

I've often found myself sorting by multiple fields more often than not in my code and it's not something particularly easy to mixin (a lot of cases to handle, can't call lookupIterator, etc). In these cases I find _.sortBy completely useless.

For the computing the value of the return rather than the parameter as with _.uniq, et. al., I'd argue sortBy is a completely different beast as we're not comparing just equality but disparities, so it may make sense to do this in max and min as well (and wouldn't be particularly difficult to abstract some of this code for that iterator case which could then be optimized for the non array case). If that's desired I can spend some time tonight refactoring

[jdalton]

This would fit better as a more specialized method rather than trying to cram it into _.sortBy. I don't like juggling on the return value of iterator, I think the behavior should be consistent without traps for return values.

In these cases I find _.sortBy completely useless.

Not surprised a small set of methods aren't appropriate for all scenarios.

it's not something particularly easy to mixin (a lot of cases to handle, can't call lookupIterator, etc).

Then expose it. I've exposed the helper as _.callback.

I'd argue sortBy is a completely different beast

The methods _.max, _.sortedIndex, & _.uniq are all different from each other. They just share allowing an iterator to resolve a computed value just like _.sortBy.

so it may make sense to do this in max and min as well (and wouldn't be particularly difficult to abstract some of this code for that iterator case which could then be optimized for the non array case)

The answer isn't to stuff more into these skinny jeans, you're gonna bust a zipper.

[megawac]

Not surprised a small set of methods aren't appropriate for all scenarios.

Sure, but I'd argue that this is a common situation based on the number of tickets requesting the feature:

• _.multisortby: sorting on multiple properties (code attached) #283,
• Extend _.sortBy to be able to sort by multiple fields #1359,
• sort by multiple nested fields lodash/lodash#581,
• sortBy multiple fields don't sort by numbers lodash/lodash#641,
• Underscore: sortBy() based on multiple attributes,
• How can I use lodash/underscore to sort by multiple nested fields?,
• Multi-sorting in underscore

etc... and something core should support.

This pr can be optimized further ofc for the common non array case.

[jdalton]

Sure, but I'd argue that this is a common situation

That doesn't mean _.sortBy should be expanded or expanded in this way. Your persistence on juggling based on the return value makes this PR DOA for me. I've handled it in a different way that keeps consistent iterator behavior and allows optimizing for the common case but even then I've received requests to specialize it further. This points to devs needing a separate method (or a couple of methods) to handle their use cases.

[olleicua]

What about a _.sortByMultiple method where the second array is a list of property strings and/or functions. Alternately what if _.sortBy checked whether it's second argument was an array and did this in that case.

[megawac]

SMT like this maybe (quickly written untested) if you guys really are against switching on return values :/

function baseComparitor(left, right) {
  var a = left.criteria;
  var b = right.criteria;
  if (a !== b) {
    if (a > b || a === void 0) return 1;
    if (a < b || b === void 0) return -1;
  }
  return left.index - right.index;
}

function arrayComparitor(left, right) {
  var leftArr = left.criteria;
  var rightArr = right.criteria;
  for (var i = 0, len = leftArr.length, blen = rightArr.length; i < len; i++) {
    if (i >= blen) return 1;
    var a = leftArr[i], b = rightArr[i];
    if (a !== b) {
      if (a > b || a === void 0) return 1;
      if (a < b || b === void 0) return -1;
    }
  }
  return blen == len ? left.index - right.index : -1;
}

function sortByComparitor(comparator) {
  return function(obj, iterator, context) {
    iterator = lookupIterator(iterator, context);
    return _.pluck(_.map(obj, function(value, index, list) {
      return {
        value: value,
        index: index,
        criteria: iterator(value, index, list)
      };
    }).sort(comparator), 'value');
  };
}

// Sort the object's values by a criterion produced by an iterator.
_.sortBy = sortByComparitor(baseComparitor);
_.sortByArray = sortByComparitor(arrayComparitor);

[jashkenas]

@megawac — you seem to be around this afternoon. Mind popping on IRC for a minute?

[jgonggrijp]

_.sortBy is for when you can reduce your values to a single string or number so that you can meaningfully apply the < operator. If you need to do something more fancy, pass your own custom comparator function to Array.prototype.sort instead. Note that this function is also available in Underscore OOP notation and in chains as _(yourArray).sort.

Related to #2848.