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

_.indexOf doesn't work on arrays of objects #557

Closed
zetlen opened this issue Apr 14, 2012 · 15 comments
Closed

_.indexOf doesn't work on arrays of objects #557

zetlen opened this issue Apr 14, 2012 · 15 comments
Labels

Comments

@zetlen
Copy link

zetlen commented Apr 14, 2012

_.indexOf doesn't use _.isEqual for its test, so you can't look up the index of an object in an array.
var a = [{23:'hello'},{283:'goodbye'}],
b = {283:'goodbye'};

_.indexOf(a,b); // produces -1

I propose the following change to _.indexOf.

old version

indexOf: function (array, item, isSorted) {
    if (array == null) return -1;
    var i, l;
    if (isSorted) {
      i = _.sortedIndex(array, item);
      return array[i] === item ? i : -1;
    }
    if (nativeIndexOf && array.indexOf === nativeIndexOf) return array.indexOf(item);
    for (i = 0, l = array.length; i < l; i++) if (i in array && array[i] === item) return i;
    return -1;
  }

New version uses an optional deepEquality flag. Deep equality test cannot use nativeIndexOf and is significantly slower.

indexOf: function (array, item, isSorted, deepEquality) {
    var eqfn = deepEquality ? _.isEqual : function (a, b) { return a === b };
    if (array == null) return -1;
    var i, l;
    if (isSorted) {
        i = _.sortedIndex(array, item);
        return eqfn(array[i], item) ? i : -1;
    }
    if (!deepEquality && nativeIndexOf && array.indexOf === nativeIndexOf) return array.indexOf(item);
    for (i = 0, l = array.length; i < l; i++) if (i in array && eqfn(array[i], item)) return i;
    return -1;
}
@ghost
Copy link

ghost commented Apr 14, 2012

The change to indexOf will break compatibility with the native Array#indexOf, though...it doesn't use isEqual internally, and there's no way to specify a custom comparison function.

@octatone
Copy link
Contributor

Can you clarify why you chose to rewrite _.indexOf instead of modifying _.intersection?

@zetlen
Copy link
Author

zetlen commented Apr 15, 2012

I did make a mistake in my proposed code. Doing the deep equality check would mean you couldn't use the native Array.prototype.indexOf, so that's a definite performance concern. I'd say there should definitely be some kind of deepEquality flag.

I also made a mistake in naming the issue! I discovered this when trying to use .intersection, but it's more properly a missing feature in .indexOf. Here's my proposed update.

indexOf2: function (array, item, isSorted, deepEquality) {
    var eqfn = deepEquality ? _.isEqual : function (a, b) { return a === b };
    if (array == null) return -1;
    var i, l;
    if (isSorted) {
        i = _.sortedIndex(array, item);
        return eqfn(array[i], item) ? i : -1;
    }
    if (!deepEquality && nativeIndexOf && array.indexOf === nativeIndexOf) return array.indexOf(item);
    for (i = 0, l = array.length; i < l; i++) if (i in array && eqfn(array[i], item)) return i;
    return -1;
}

Passing the flag through all the methods that use .indexOf would obviously require a more extensive refactor, but you get the basic idea.

@zetlen
Copy link
Author

zetlen commented Apr 15, 2012

Modified the original post to reflect this.

@jashkenas
Copy link
Owner

JavaScript uses object identity equality, not deep equality. Underscore follows suit. isEqual is a special-case function. The rest of Underscore shouldn't use its equality semantics by default.

@zetlen
Copy link
Author

zetlen commented Apr 18, 2012

I agree with you that object identity equality should be the default! I was at least hoping for a flag one could set that would use isEqual.

@paulbjensen
Copy link

+1 for suggestion made by @zetlen, deep Equality comparison of objects within an array would be very useful.

@kendagriff
Copy link

+1

@ghost
Copy link

ghost commented Nov 26, 2012

Best not to extend the semantics of indexOf due to conflicts with Array::indexOf—you can use something like this, though:

_.mixin({
  'deepIndex': function (array, item) {
    var result = -1;
    _.some(array, function (value, index) {
      if (_.isEqual(value, item)) {
        result = index;
        return true;
      }
    });
    return result;
  }
});

@kendagriff
Copy link

Thanks, I basically did just that.

@bravadomizzou
Copy link

If we are going to do everything the way JavaScript does by default, why not just delete the underscore.js library then?

@jakecraige
Copy link

Really bravadomizzou? It provides fallbacks for browsers that don't support it and provides some extra functionality that default javascript doesn't have.

@tapsboy
Copy link

tapsboy commented Jan 7, 2014

I wonder why the following two samples work differently:

var a = [{23:'hello'},{283:'goodbye'}]
, b = {283:'goodbye'};
console.log(b);
console.log(_.indexOf(a,b)); // returns -1

var c = .findWhere(a, {283:'goodbye'});
console.log(c);
console.log(
.indexOf(a,c)); // returns 1

I have created a fiddle here: http://jsfiddle.net/rjcXL/1/

@caseywebdev
Copy link
Contributor

@tapsboy It's the same reason

({a: 1}) === ({a: 1});

is false, but

({a: 1}).a === ({a: 1}).a;

is true. The first is comparing references while the second is comparing values.

@rtorino
Copy link

rtorino commented Mar 11, 2014

I encountered the same issue, I used _.findWhere instead of _.indexOf. Thanks.

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

No branches or pull requests

10 participants