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

Fixed bug in _.sortedIndex that would put undefined at the start FIXES: https://github.com/jashkenas/underscore/issues/1834 #2041

Closed
wants to merge 1 commit into from

Conversation

arianf
Copy link
Contributor

@arianf arianf commented Feb 9, 2015

Changed _.sortedIndex function to hangle undefined values correctly (FIXES: #1834 )

Previously

  if (iteratee(array[mid]) < value) low = mid + 1; else high = mid;

Changed it to:

var cur = iteratee(array[mid]);
if ( cur < value || (value === void 0 && cur <= Infinity || _.isNaN(cur))) low = mid + 1; else high = mid;

The logic works because Undefined should be placed after the highest number possible + 1.

(iteratee(array[mid]) <= Inifinity) // will always place undefined after the highest number, including Inifinity.

I also added in a check to see if the iteratee(array[mid]) was NaN, so NaN has a chance of stopping it.

_.sortBy([undefined, 10, 1000, 0, NaN, NaN]);
// => [ 0, 10, 1000, NaN, NaN, undefined ]

_.sortedIndex([ 0, 10, 1000, NaN, NaN, undefined ], undefined);
// => 5

without || _.isNaN(cur)

_.sortedIndex([ 0, 10, 1000, NaN, NaN, undefined ], undefined);
// => 3

So that how I built

var cur = iteratee(array[mid]);
if ( cur < value || (value === void 0 && cur <= Infinity)) low = mid + 1; else high = mid;

Since no numeric value can be greater than Infinity, it will place undefined values to the correct index.

Also added edgeCaseTests, to verify it works:

test('sortedIndex conforms to sortBy for undefined (#1834)', function() {
    var sorted = _([{a: NaN}, {a: NaN}, {a: void 0}, {a: 0}, {a: 1}, {a: 2}, {a: 3}])
                  .sortBy('a');
    _.each([0, 1, 2, 3, undefined], function(val) {
      equal(_.sortedIndex(sorted, {a: val}, 'a'), _.findIndex(sorted, {a: val}), 'For val: ' + val);
    });

    sorted = _.sortBy([undefined, 1, undefined, 2]);
    equal(_.sortedIndex(sorted, undefined, _.identity), 2);

    var edgeCaseNumbers = [-Infinity, -Infinity, 0, Number.MAX_VALUE, Infinity, Infinity, undefined, undefined];

    var indexForUndefined = _.sortedIndex(edgeCaseNumbers, undefined);
    equal(indexForUndefined, 6, 'undefined should be inserted at index 6');

    var indexForNegInfinity = _.sortedIndex(edgeCaseNumbers, -Infinity);
    equal(indexForNegInfinity, 0, 'negative infinity should be inserted at index 0');

    var indexForInfinity = _.sortedIndex(edgeCaseNumbers, Infinity);
    equal(indexForInfinity, 4, 'infinity should be inserted at index 4');

    var indexForZero = _.sortedIndex(edgeCaseNumbers, 0);
    equal(indexForZero, 2, '0 should be inserted at index 2');

    var numbers = [10, 20, 30, 40];

    var indexForUndefinedSimple = _.sortedIndex(numbers, undefined);
    equal(indexForUndefinedSimple, 5, 'undefined should be inserted at index 5');
  });

@jdalton
Copy link
Contributor

jdalton commented Feb 9, 2015

@arianf There's another WIP PR that puts NaN at the end of the sort. I think this PR should tie-in to that one.

So the order would be:

_.sortBy([undefined, 10, NaN, 1000, NaN, 0]);
// => [0, 10, 1000, undefined, NaN, NaN]

@arianf
Copy link
Contributor Author

arianf commented Feb 9, 2015

@jdalton I'll make another pull request for _.sortBy that index's NaN and undefined values like that

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

Successfully merging this pull request may close these issues.

_.sortedIndex align with _.sortBy
2 participants