Skip to content

Conversation

FabriceGaudin
Copy link

From issue #2676

I adapt this function from existing one findKey, but tell me if you think about other needed test/documentation.

@coveralls
Copy link

coveralls commented Jun 19, 2017

Coverage Status

Coverage increased (+0.02%) to 96.786% when pulling 9d66959 on FabriceGaudin:master into 20e7c6e on jashkenas:master.

@anvyne
Copy link

anvyne commented Jul 22, 2017

_.findKeys is a nice method! For what ever it is worth, you could also save some bytes on _.filter by using _.findKeys:

_.filter = function(obj, predicate, context) {
  return _.map(_.findKeys(obj, predicate, context), function(key) {
    return obj[key];
  });
};

That may or may not be desirable because it would probably entail a little bit less efficiency. Much of that efficiency could probably be gained back by not declaring allKeys = _.keys(obj) within _.findKeys and letting _.each decide when to use _.keys instead, depending on whether the object is array-like. This would mean calling _.each on the object itself, and using the second argument that _.each gives to the iteratee.

This version of _.findKeys would be a little bit shorter and look something like this:

_.findKeys = function(obj, predicate, context) {
  predicate = cb(predicate, context);
  var res = [];
  _.each(obj, function(value, key) {
    if (predicate(value, key, obj)) res.push(key);
  });
  return res;
};

In that respect, _.findKeys could be a collection method. Regardless of what you think about that _.filter method implementation, I think it is worth considering the update to _.findKeys. _.findKeys is just as useful for finding indexes in an array or array-like as in an object, so _.each's behavior with array-likes is desirable.

N.B. I have not tested these implementations of _.filter or _.findKeys, but they are accurate to the best of my knowledge.

@FabriceGaudin
Copy link
Author

@anvyne I think this implementation is great, but after testing it, _.each doesn't make findKeys pass this test (which comes directly from this one).
The array [ 1, 2, 3, 4, match: 55 ] passes isArrayLike, so it doesn't use _.keys method and length is still 4. I don't really understand when we have to deal with this kind of array, do you know what should be the correct behavior ?

If we manage to modify findKeys as a collection method, I totally agree to use it in _.filter

@anvyne
Copy link

anvyne commented Jul 25, 2017

There is a sort of precedence, actually, if we look at _.find, which is analogous to _.filter. _.find returns the first value that passes a truth test, while _.filter returns all values that pass a truth test.

_.find decides whether to use _.findIndex or _.findKey depending on whether the passed in object is array-like or not. It would be symmetrical to implement _.filter in an analogous way. However, there is no method _.findIndexes.

Arguably, if there exists the object method _.findKeys, then there should also exist the array method _.findIndexes, and _.findKeys should only behave like an object method (that is, in a way that searches all keys regardless of whether the passed object is array-like or not). It would be appropriate to treat array-likes differently than other objects only if _.findKeys were a collection method.

However, adding another method like _.findIndexes would need careful consideration because it is ideal to keep Underscore short. See #2060. That's not to say that _.findIndexes definitely should not also be implemented.

If _.findIndexes were to never be implemented, then I would prefer a _.findKeys collection method to a _.findKeys object method because it would probably be more useful even though it would be a slight misnomer and a break of precedence. The ideal situation, however, is to have both _.findIndexes and _.findKeys.

@jgonggrijp jgonggrijp added contrib probably belongs in Underscore-contrib enhancement labels Jan 11, 2021
This was referenced Jan 11, 2021
@jgonggrijp jgonggrijp self-assigned this Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contrib probably belongs in Underscore-contrib enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants