Skip to content
New issue

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

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

Already on GitHub? # to your account

_.max and _.min may behave incorrectly if iteratee returns infinite number #2688

Open
jpli opened this issue Jul 9, 2017 · 3 comments · May be fixed by #2908
Open

_.max and _.min may behave incorrectly if iteratee returns infinite number #2688

jpli opened this issue Jul 9, 2017 · 3 comments · May be fixed by #2908

Comments

@jpli
Copy link

jpli commented Jul 9, 2017

The iteratee "swaps" the contents of the array, so _.max should return the smaller member:

_.max([2, 5], function (value) { return value === 5 ? 2 : 5; });  // 2, correct

When the smaller member is -Infinity:

_.max([-Infinity, 5], function (value) { return value === 5 ? -Infinity : 5; });  // 5, wrong

Similar for _.min:

_.min([10, 5], function (value) { return value === 5 ? 10 : 5; });  // 10, correct
_.min([Infinity, 5], function (value) { return value === 5 ? Infinity : 5; });  // 5, wrong

I know which part of the code causes the bug, for example in _.min, it's the conditional expression after ||:

if (computed < lastComputed || computed === Infinity && result === Infinity) {
    ...

But I don't know how to fix it because I can't figure out the exact intent of that expression.

@anvyne
Copy link

anvyne commented Jul 21, 2017

I think that the conditional expression after || is used to favor some element in the collection over the default Infinity result.

For _.min, Infinity is the default result. It is preferred that some element from the collection is returned instead of the default result, even if its computed value is also Infinity.

@jgonggrijp
Copy link
Collaborator

I would like to fix this by generalizing min and max so they also work for strings and they return undefined for empty collections. Below is just a draft to illustrate the principle, as a proper implementation should invoke the iteratee only once for each element and it should also pass the key and the collection when doing that:

function max(collection, iteratee, context) {
    if (isEmpty(collection)) return;
    iteratee = cb(iteratee, context);
    return reduce(collection, function(memo, value) {
        return iteratee(value) < iteratee(memo) ? value : memo;
    });
}

This would be a breaking change.

@jgonggrijp
Copy link
Collaborator

Update: I have a "compromise" fix on a private work-in-progress branch, which implements the generalization I mentioned above and then jumps through a few additional hoops in order to keep it a non-breaking change. I'll probably publish that branch somewhere in the next month or so.

@jgonggrijp jgonggrijp removed this from the 2.0 (planned breaking changes) milestone Dec 19, 2020
jgonggrijp added a commit to jgonggrijp/underscore that referenced this issue Feb 4, 2021
@jgonggrijp jgonggrijp linked a pull request Feb 7, 2021 that will close this issue
19 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants