Skip to content

Commit

Permalink
Allow _.groupBy to create dictionaries for O(n) lookups
Browse files Browse the repository at this point in the history
  • Loading branch information
@caseywebdev
caseywebdev committed Jul 11, 2013
1 parent 14c3f9a commit decb97f
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 2 deletions.
9 changes: 8 additions & 1 deletion test/collections.js
View file
Expand Up @@ -261,7 +261,7 @@ $(document).ready(function() {
result = _.where(list, {b: 2});
equal(result.length, 2);
equal(result[0].a, 1);

result = _.where(list, {a: 1}, true);
equal(result.b, 2, "Only get the first object matched.")
result = _.where(list, {a: 1}, false);
Expand Down Expand Up @@ -377,6 +377,13 @@ $(document).ready(function() {
];
deepEqual(_.groupBy(matrix, 0), {1: [[1,2], [1,3]], 2: [[2,3]]})
deepEqual(_.groupBy(matrix, 1), {2: [[1,2]], 3: [[1,3], [2,3]]})

var stooges = [{name: 'Larry'}, {name: 'Curly'}, {name: 'Moe'}];
deepEqual(_.groupBy(stooges, 'name', true), {
'Larry': stooges[0],
'Curly': stooges[1],
'Moe': stooges[2]
}, 'creates a dictionary with when a property is given');
});

test('countBy', function() {
Expand Down
7 changes: 6 additions & 1 deletion underscore.js
View file
Expand Up @@ -335,8 +335,13 @@

// Groups the object's values by a criterion. Pass either a string attribute
// to group by, or a function that returns the criterion.
_.groupBy = function(obj, value, context) {
_.groupBy = function(obj, value, dict, context) {

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Jul 11, 2013

This has been brought up before as another api (see #1064 & #1008).

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Jul 11, 2013

It's also trickier to make this optimize _.intersection and friends because it will conflate things like the number 2 and the string "2" and treat most objects as [object Object].

This comment has been minimized.

Sign in to view

Copy link
@caseywebdev

caseywebdev Jul 11, 2013

Author Owner

I'm indifferent to the API, I was trying to latch onto an existing method due to the general resistance to new methods. _.indexBy also seems like a fine choice. An intersection between dictionaries shouldn't care about the values as it should be the user's responsibility to create the correct dictionary. The intersection implementation would look something like

_.intersection = function (first) {
  if (_.isArray(first)) // do current array intersection...
  var rest = slice.call(arguments, 1);
  return _.filter(first, function (__, key) {
    return _.all(rest, function (other) { return _.has(other, key); });
  });
};

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Jul 11, 2013

An intersection between dictionaries shouldn't care about the values as it should be the user's responsibility to create the correct dictionary.

I thought you were talking about using a dictionary to optimize general _.intersection use.

This comment has been minimized.

Sign in to view

Copy link
@caseywebdev

caseywebdev Jul 11, 2013

Author Owner

That could be done as well, but as you say, unreliably.

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Jul 11, 2013

Not a fan of this being bolted on to _.groupBy with a boolean. It's not descriptive or obvious what the intended result would be (simply passing a boolean). I think a special case method would be better as it is so different than groupBy.

This comment has been minimized.

Sign in to view

Copy link
@caseywebdev

caseywebdev Jul 11, 2013

Author Owner

Personally I agree and vote for _.indexBy. This implementation was a response to @jashkenas's comment.
if (!_.isBoolean(dict)) {
context = dict;
dict = false;
}
return group(obj, value, context, function(result, key, value) {
if (dict) return result[key] = value;
(_.has(result, key) ? result[key] : (result[key] = [])).push(value);
});
};
Expand Down

0 comments on commit decb97f

Please sign in to comment.