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

Alternative implementation of randInt #8

Closed
wants to merge 1 commit into from

Conversation

pmcelhaney
Copy link

What do you think about this? Instead of snapping to the max or min number, generate a number min <= n < max + 1, and round that number down.

@millermedeiros
Copy link
Owner

this implementation is indeed more elegant. but shouldSnap shouldn't be removed since it is used to return only the min or max values see docs and specs

@millermedeiros
Copy link
Owner

ahhh, just realized that there is a problem with this approach, if Math.random() returns 1 it will return a number greater than max, so it would also require "clamping" the value inside range.

@millermedeiros
Copy link
Owner

to work properly without snap it would need to be something like:

function randInt(min, max) {
  var diff = max - min,
      rnd = Math.random();
  return rnd !== 1? Math.floor((diff + 1) * rnd) + min : min + diff;
}

then it isn't that much simpler :(

@pmcelhaney
Copy link
Author

Have you considered putting the shouldSnap behavior in a new function?

choice(-1,1) // 50% chance -1, 50% chance of 1 -- the same as randInt(-1,1,true)
choice(2,3,5,7) // 25% chance each of 2, 3, 5, or 7

BTW, I just read your blog post. I hope you didn't take my tweet or pull request the wrong way. I admire your work and was under the impression that you had a computer science background.

@millermedeiros
Copy link
Owner

good idea, a new method would indeed be better... I will open a new issue.

and BTW I just realized we are over complicating things. while I was editing the post right now to explain what is the issue I realized that it could be implemented as:

function randInt(min, max) {
   return Math.round( rand(min - 0.5, max + 0.5) );
}

since the problem only happens on the min/max values.. now I feel dumb....

@millermedeiros
Copy link
Owner

and somebody just commented before I updated the post with this solution :D

@pmcelhaney
Copy link
Author

if Math.random() returns 1 it will return a number greater than max

Good catch. I'm pretty sure Math.random() doesn't ever return 1. (Okay, according to MDN it's not supposed to return 1. It could, but the chance is so tiny I don't know if it's worth worrying about. )

@millermedeiros
Copy link
Owner

and I also realized that the max + 0.5 can make rounding overflow range... thanks for the comments and suggestions. I updated the post with info about the pull request and new solutions. cheers!

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

Successfully merging this pull request may close these issues.

None yet

2 participants