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

Add _.sample(array, number) functionality to underscore.js #963

Merged
merged 4 commits into from Sep 4, 2013

Conversation

@vincentwoo
Copy link
Contributor

@vincentwoo vincentwoo commented Feb 4, 2013

Ruby's Array#sample method is extremely useful to me, at least as useful as shuffle.

I've replicated that functionality here in underscore and added a test. One thing I'm not sure if I should have done was make this work with javascript objects as well as arrays. Most of ujs' functions work across they keys/values of arbitrary objects, and I wasn't sure if it was right to do that here, or what exactly that should look like if I did.

Please advise if this is up to scratch, and thanks for the great library.

} else {
var sampled_indices = _.shuffle(_.range(obj.length));
var sampled_values = [];
while (number-- > 0 && sampled_indices.length > 0) {

This comment has been minimized.

@michaelficarra

michaelficarra Feb 4, 2013
Collaborator

for (; number > 0 && sampled_indices.length > 0; --number) {

I try never to side effect in a sub-expression. Always make it a separate statement.

@vincentwoo
Copy link
Contributor Author

@vincentwoo vincentwoo commented Feb 4, 2013

Sure thing! How's this?

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Feb 12, 2013

Very cool, and great as an _.mixin function, but probably a bit too special case for Underscore proper. Thanks for sending.

@jashkenas jashkenas closed this Feb 12, 2013
@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Feb 13, 2013

Really? Choosing a random member of a list doesn't seem like a very uncommon task to me.

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Feb 13, 2013

I can't say I've ever needed it, and the requirements would likely be rather specific to the situation.

@vincentwoo
Copy link
Contributor Author

@vincentwoo vincentwoo commented Feb 13, 2013

I personally disagree. Shuffle is already a core ujs method, and I believe it is almost never used. Here are some usage statistics on .sample and .shuffle as they appear in all of the ruby files on Github:

sample has 88k usages
shuffle has 1k usages

The more useful method seems clear to me, here.

@bencevans
Copy link

@bencevans bencevans commented Feb 13, 2013

👍

@caseywebdev
Copy link
Contributor

@caseywebdev caseywebdev commented Jun 25, 2013

I'm generating test data and wish this was in core.

var parents = ...; // assume populated
var children = _.times(100, function (n) {
  return {id: ++n, name: 'Child ' + n, parent: _.sample(parents)};
});
@agrberg
Copy link

@agrberg agrberg commented Aug 7, 2013

I was just going to implement this myself but checked the issues first. I think this is a great addition to underscore.
_.sample(myArray) would be preferable to myArray[_.random(myArray.length - 1)] and the much longer solution to randomly grab two unique elements.

@vincentwoo
Copy link
Contributor Author

@vincentwoo vincentwoo commented Aug 10, 2013

@jashkenas I know you're super busy but I think this deserves a second look.

@caseywebdev
Copy link
Contributor

@caseywebdev caseywebdev commented Aug 10, 2013

Still 👍 for _.sample as a companion to _.shuffle.

@agrberg
Copy link

@agrberg agrberg commented Aug 11, 2013

Also @vincentwoo brings up a very good point. Looking at my own ruby code (whose book I believe underscore takes page from) sample is use a significantly.

@jashkenas jashkenas reopened this Aug 11, 2013
@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Aug 11, 2013

Sure thing. Maybe the implementation could be tightened up though? Generating a complete list of indexes, most of which might not be used, doesn't seem ideal.

@vincentwoo
Copy link
Contributor Author

@vincentwoo vincentwoo commented Aug 11, 2013

I've nixed using a sampled index array - it's not actually any faster given how JS is pass by reference for all objects, anyway. I've also added a couple more tests. Let me know what you think.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Aug 11, 2013

If number is not specified, returns only a single sampled object

What do folks think about that behavior -- returning an array sometimes, and a value others. Inconsistent, no?

@vincentwoo
Copy link
Contributor Author

@vincentwoo vincentwoo commented Aug 11, 2013

I find the behavior pretty intuitive. At the very least, it's Ruby's behavior:

2.0.0-p247 :001 > [1,2,3].sample
 => 2 
2.0.0-p247 :002 > [1,2,3].sample 2
 => [3, 1] 
2.0.0-p247 :003 > [1,2,3].sample 0
 => [] 
@agrberg
Copy link

@agrberg agrberg commented Aug 11, 2013

While the return value is inconsistent, the usage and expected value from the use semantics are consistent.

@tomconroy
Copy link

@tomconroy tomconroy commented Aug 12, 2013

+1; I was surprised to find that sample wasn't implemented yet.

if (number < 0) {
throw new Error('sample cannot be called with a negative number of picks');
}
return _.shuffle(obj).slice(0, number);

This comment has been minimized.

@braddunbar

braddunbar Aug 12, 2013
Collaborator

I'd just as soon have negative numbers return zero results like _.range.

return _.shuffle(obj).slice(0, Math.max(number, 0));

This comment has been minimized.

@caseywebdev

caseywebdev Aug 12, 2013
Contributor

👍 for empty array over throw.

@braddunbar
Copy link
Collaborator

@braddunbar braddunbar commented Aug 12, 2013

The different return types do bother me a bit since, as currently implemented, passing a null argument will likely cause an error due to an unexpected return type. Passing a string by accident would do the same. If we're going to return different types I think we should use arguments.length instead of the type check.

}
return _.shuffle(obj).slice(0, number);
} else {
return obj.length > 0 ? obj[_.random(obj.length - 1)] : null;

This comment has been minimized.

@caseywebdev

caseywebdev Aug 12, 2013
Contributor

return obj[_.random(obj.length - 1)];

I think this should suffice here. Also, might as well drop it out of the else block.

This comment has been minimized.

@vincentwoo

vincentwoo Aug 12, 2013
Author Contributor

Doing so returns undefined when sampling an empty array. Is that what you intended?

This comment has been minimized.

@caseywebdev

caseywebdev Aug 12, 2013
Contributor

Yup. _.find([], function () {}), _.first([]), _.last([]), etc... all return undefined rather than null. Also, fewer bytes 😉

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Aug 12, 2013

I'd prefer to see throw when you try to sample an empty array. And keep the throw for negative samples.

@vincentwoo
Copy link
Contributor Author

@vincentwoo vincentwoo commented Aug 12, 2013

On null vs throw for sampling empty arrays - I think we should keep null. You very often sample arrays that might be empty, and having a result that you can expect to be falsy is pretty useful. Ruby also behaves this way.

On empty array vs throw for negative numbers - I don't have a strong opinion on this one. How are decisions like this made in underscore.js?

@jdalton

This comment has been minimized.

Copy link

@jdalton jdalton commented on 34d1865 Aug 16, 2013

Is this an "Arrays" method? It's using _.shuffle which is a "Collections" method, and in the same code location as other "Collections" methods.

This comment has been minimized.

Copy link

@jdalton jdalton replied Aug 16, 2013

Should also add a guard so that this method can be used with _.map.

This comment has been minimized.

Copy link
Owner Author

@vincentwoo vincentwoo replied Aug 16, 2013

I'm not sure of the difference between an Arrays and Collections methods.

I am also not quite sure how guard functions with map. I see other methods that take the parameter, I'm just not sure what needs to be done.

jashkenas added a commit that referenced this pull request Sep 4, 2013
Add _.sample(array, number) functionality to underscore.js (Will tweak, in a moment.)
@jashkenas jashkenas merged commit 8f616a6 into jashkenas:master Sep 4, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Sep 4, 2013

Tweaked in 8778df3.

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

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.