respect iterator return value of Infinity/-Infinity in _.min/_.max

[davidchambers]

A colleague and I were just bitten by a bug in this code:

var bestMatch = _.max(inPosteds, function(inPosted) {
  // only match to posted transactions that haven't been resolved
  if (inPosted._pendingTransaction) {
    return -Infinity;
  } else {
    return pendingSimilarity(
      pendingTransform(outPending),
      postedTransform(inPosted)
    );
  }
});

We were surprised to discover that bestMatch would in some cases be -Infinity rather than an object. This results from Underscore performing > comparisons against an initial max of -Infinity. Since -Infinity is not greater than -Infinity, the initial value is erroneously returned if the iterator returns -Infinity for each element in a nonempty collection.

I've implemented the least invasive fix, which is to use >= and <= in place of > and <. Note that this does change undocumented behaviour not covered by the test suite: the last min/max element will be returned rather than the first. Consider a slightly modified version of the example from the documentation:

var stooges = [{name: 'moe', age: 40}, {name: 'larry', age: 50}, {name: 'curly', age: 50}];
_.max(stooges, function(stooge){ return stooge.age; });
// before => {name: 'larry', age: 50}
// after  => {name: 'curly', age: 50}

If we wish to preserve this undocumented behaviour, I'll add appropriate test cases and adjust the implementations of _.min and _.max accordingly. This may simply involve enumerating the given collection in reverse order.

[jdalton]

I'm not a fan of mucking with the comparison logic with <= and variations.
I'd say spec would agree with the existing behavior.

[davidchambers]

I don't see how the spec relates to this specific issue, @jdalton, since Math.max does not accept an iterator. It's awkward to state in the documentation: Iterators should not return Infinity/-Infinity. Use Number.MAX_VALUE/Number.MIN_VALUE instead.

[jdalton]

since Math.max does not accept an iterator

It's the result of the iterator that is the important thing. The result is what's used to resolve values to compare. The comparison is what aligns with the spec.

It's awkward to state in the documentation: Iterators should not return Infinity/-Infinity. Use Number.MAX_VALUE/Number.MIN_VALUE instead.

Naw. If you want to get specific you can say that it compares values according to the abstract comparison algorithm, which is just a fancy way to say x < y.

[davidchambers]

@jdalton, are you suggesting that this is correct behaviour?

> _.max([{name: 'Alice', score: -Infinity}, {name: 'Bob', score: -Infinity}], _.property('score'))
-Infinity

I would expect to get back the Alice object or the Bob object, not -Infinity.

[jdalton]

I would expect to get back the Alice object or the Bob object, not -Infinity.

Ok, I could see that, though I wouldn't want to change the < behavior maybe a check for Infinities directly.

[davidchambers]

I not fussed either way, so I'll go ahead and update the PR to avoid changing the operators.

[davidchambers]

Thanks very much for your feedback, @jdalton. The PR is in a better state as a result. It no longer uses >= or <=, and no longer returns the last min/max element in the case of a tie.

[davidchambers]

This test ensures that the first element in the list is returned.

[davidchambers]

Are you happy with this PR as it currently stands, @jdalton?

[jdalton]

Yap, rocks!