Comparator edge-case handling

[megawac]

There are several inconsistencies with the comparator functions I feel should be addressed in 2.0. Mentioned this earlier in jashkenas/backbone#3243

• max: NaN including undefined is considered the lowest item in the array. Even lower then no item

_.max([{a: {}}], 'a'); // => -Infinity

• min: same as max but NaN values are considered the highest items
• sortBy: special cases all undefined values to the end of the array. All NaN values are moved to the front of the array
• sortedIndex: different from sortBy (see lodash/lodash@b07337f#commitcomment-7164123): computed NaN is moved to the end of the list instead of the front

var array = [{a: NaN}, {a: NaN}, {a: {}}, {a: 1}, {a: 2}];
_.sortedIndex(array, {a: 3}, 'a'); // => 0
var sorted = _.sortBy(array.concat({a: 3}), 'a');
_.findIndex(sorted, {a: 3}); // => 5

---

I propose a baseComparator(a, b) used by those 4 functions (special case min?) which handles these cases consistently. I feel strongly about changing sortBy's handling of NaN vs undefined. I think they should be considered NaN < -Infinity < x.

// Reasoning
_.sortBy([{a: 1}, {a: -Infinity}, {a: 2}, {}], 'a')
// Should => [{}, {a: -Infinity}, {a: 1}, {a: 2}]

[jdalton]

max: NaN including undefined is considered the lowest item in the array. Even lower then no item

normally undefined coerces to NaN (ie +undefined)

sortBy: special cases all undefined values to the end of the array. All NaN values are moved to the front of the array

That's not really the case with NaN:

_.sortBy([1, 2, NaN, {}, NaN, undefined]);
// => [1, 2, NaN, {}, NaN, undefined]

As for _.min and _.max:

_.max([NaN, {}]); // -Infinity

Is fine as NaN isn't comparable.

I found the one valid case was to adjust _.sortedIndex. It wasn't to move all NaN to the front, but to simply skip them if I detected a compare hint of number to align to _.sortBy.

Other than that it looks like you should be filtering your values to avoid headaches with NaN & undefined.

[megawac]

_.sortBy([1, 2, NaN, {}, NaN, undefined]);
// => [1, 2, NaN, {}, NaN, undefined]

I don't think thats a good argument seeing

_.sortBy([1, 2, NaN, {}, NaN, undefined, 1]);
// => [1, 2, NaN, {}, NaN, 1, undefined]

[jdalton]

I'm saying your statement is incorrect about NaN being moved to the front and I proved it, not that the order isn't wonky.

In ES6, if the compareFn result is NaN it is treated like 0 (Underscore's compareFn result is never NaN but will end up being 0 for NaN as it will fail a < b and a > b comparisons). The ES6 behavior is standardizing on the existing behavior. That said I've noticed slight differences between engines and some NaN order.

NaN are a cluster because they aren't comparable. Moving NaN to the front or back seems kind of pointless as you'd still want to filter them out. The reason undefined was aligned to spec was because of a dev request, though I'd argue the same thing, filtering is the best way to avoid issues.

[jashkenas]

Sorting non-comparable or undefined values is generally undefined behavior — and I don't think it really matters what Underscore happens to do when it occurs.

But that said, if there's a change that makes things internally consistent, and that makes the codebase simpler and not more complicated, that sounds fine.

[megawac]

I'm thinking a comparator like this should be used internally. Let me know if I should work on a pr

  // Default internal comparitor for determining whether a is greater (1),
  // equal (0) or less than (-1) some object b
  var internalComparator = function(a, b) {
    if (a === b) return 0;
    var isANaN = isNaN(a), isBNaN = isNaN(b);
    if (isANaN || isBNaN) {
      if (isANaN && !isBNaN) return -1;
      if (isBNaN && !isANaN) return 1;
      return a > b ? 1 : (b > a) ? -1 : 0;
    }
    return a > b ? 1 : -1;
  };

[jdalton]

What's the comparator do (example input/output)? If that's native isNaN wouldn't that lump undefined, NaN, some strings etc. together?

[megawac]

Yeah, again I'm arguing undefined should be a special cased NaN when sorting -- mainly for the _.sortBy([{a: 1}, {b: 2}], 'a') cases. Strings, and anything with a valueOf that doesn't evaluate to NaN will be sorted greater than NaN

On Fri, Aug 29, 2014 at 11:39 AM, John-David Dalton <
[email protected]> wrote:

What's the comparator do (example input/output)? If that's native isNaN
wouldn't that lump undefined, NaN, some string etc. together?

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

[megawac]

Some examples

// examples
internalComparitor({}, 'a'); // => -1
internalComparitor('a', {}); // => 1
internalComparitor(1, {}); // => 1
internalComparitor({}, 1); // => -1
internalComparitor(1, void 0); // => 1
internalComparitor(void 0, 1); // => -1
internalComparitor(2, 1); // => 1
internalComparitor(1, 2); // => -1

[jdalton]

I meant input/output of _.sortBy using the proposed change. I think with this undefined and NaN will be lumped together in no particular order (by that I mean a mix of NaN and undefined, not like all undefined before NaN or smth).

[megawac]

I'd like issue to change the behaviour below

_.sortBy([{}, {a: 1}, {a: 2}, {a: 3}], 'a');
// => [{}, {a: 1}, {a: 2}, {a: 3}]
// instead of => [{a: 1}, {a: 2}, {a: 3}, {}]

I think with this undefined and NaN will be lumped together in no particular order

Yeah, I don't care either way for this case, I just want undefined to be like NaN at the start of the result.

[jdalton]

I'm cool with that.

[megawac]

This may look a bit funny, but for grouping undefined separate from other NaN values how about

var internalComparator = function(a, b) {
    var isANaN = isNaN(a), isBNaN = isNaN(b);
    if (isANaN || isBNaN) {
      if (isANaN && !isBNaN) return -1;
      if (isBNaN && !isANaN) return 1;
      return a > b ? 1 : (b > a) ? -1 : (a == void 0) - (b == void 0);
    }
    return a == b ? 0 : a > b ? 1 : -1;
};

internalComparator(void 0, NaN); // => 1
internalComparator(NaN, void 0); // => -1
internalComparator(void 0, void 0); // => 0

This will place undefined before NaN

[joshuabambrick]

As I mentioned in a pull request on backbone, it might be worth adding this comparison function to the underscore api, at least as a "mostly internal" function, like iteratee.

Both because this might genuinely be useful to users, and because it would play nicely with that pull request - which is, itself, an example of how this comparison function might be useful to users :)

[smmoosavi]

console.log(_.sortBy([1, undefined, 0]));       // [  0,  1,    undefined]
console.log(_.sortBy([[1], [undefined], [0]])); // [[0], [undefined], [1]]