Add _.chunk to Array functions. #1919
Conversation
Generate a non-negative integers Array by decomposing non-negative integer number **int** into **count** random integer terms.
| _.chunk = function(array, int) { | ||
| var result = []; | ||
|
|
||
| if (int <= 0) return []; |
There was a problem hiding this comment.
or if array == null (add test)
|
That said, 👍 for the method |
| var result = []; | ||
|
|
||
| if (array == null || count <= 0) return []; | ||
| if (count >= array.length) return array; |
There was a problem hiding this comment.
First, seems unnecessary, second, we should always produce a new array in case they decide to modify it. I would kill this line
|
@keinkonzept mind addressing those comments and squashing the commits |
|
@michaelficarra While I'm 👍 on this. We use similar functions for batching DB writes constantly. |
|
|
||
| for (var i = 0, lng = array.length; i < lng;) { | ||
| result.push(array.slice(i, i += count)); | ||
| } |
There was a problem hiding this comment.
I would probably write line 655-659 as
var i = 0, length = array && array.length, j = 0;
while (i < length && count >= 0) {
if (i % count) result[j++] = [];
result[j].push(array[i]);
}(untested). I would be curious to see some benchmarks
|
Mind removing the trailing spaces on lines 649, 654, 656, and 660? |
|
|
||
| if (array == null || count <= 0) return []; | ||
|
|
||
| for (var i = 0, lng = array.length; i < lng;) { |
There was a problem hiding this comment.
Mind using length instead of lng?
|
This would be good. |
|
|
||
| if (array == null || count <= 0) return []; | ||
|
|
||
| for (var i = 0, length = array.length; i < length;) { |
There was a problem hiding this comment.
Mind using a while-loop? Every other for-loop in the codebase has an increment expression...
|
@keinkonzept: Some final nitpicks, and I think this'll be good to merge. |
|
Based on how often this has been requested, I think it should be added |
|
I think I agree at this point. |
|
That's three 👍, then. Any last comments on the source before merging? |
|
I think this implementation is broken for a count of 1. Also, I don't like having a default count. I'll comment more when in front of a computer. |
|
Seems fine for count 1 (its a tested case for default => 1 and 1). I've squashed the commits down and fixed style issues here: |
|
No, definitely broken. It should return an array of singletons. |
| _.chunk = function(array, count) { | ||
| count = count || 1; | ||
|
|
||
| if (count <= 1) return array.slice(); |
There was a problem hiding this comment.
Direct call to slice should instead use slice helper.
There was a problem hiding this comment.
No, definitely broken. It should return an array of singletons.
Should be count < 1 to fix.
Fixes #1891