millermedeiros / amd-utils

modular JavaScript utilities written in the AMD format
http://millermedeiros.github.com/amd-utils
142 stars 11 forks source link

Better randomization, especially for GUID generation #99

Closed jeffrose closed 11 years ago

jeffrose commented 11 years ago

In general you should not depend on Math.random() for anything related to GUID generation. This page explains the issue in detail as well provides JavaScript algorithms that provide better randomization. Consider adopting one those, likely Kybos since it is clearly licensed.

millermedeiros commented 11 years ago

I agree that Math.random() isn't good enough and that a seeded random is extremely useful. I will take a look on the document and implementations and check out if we can use it or not.

bebraw commented 11 years ago

Do you think Mersenne Twister would do the trick? I used that once to get repeatable random numbers (you can set the seed). Perhaps it will work for your purposes too.

jeffrose commented 11 years ago

The page I referenced mentions Mersenne Twister a couple times and precludes using it since it fails Crush randomness tests.

millermedeiros commented 11 years ago

I was thinking about it and I fell that providing a single algorithm is not going to be a good idea and is also out of the scope of this project (besides the fact that it is really hard to test PRNG properly).

I think we should add a way to swap the PRNG used by each method so you could curry each method individually and use different seeds for each one:

var mt = new MersenneTwister('magic beans seed');
var mtRandom = function(){ return mt.random() };
var randInt_1 = randInt.create( mtRandom );
var randInt_2 = randInt.create( KISS07('sunflower seed') );

And also add a way to replace it on an existing instance:

// default value = Math.random;
randInt.random = Kybos('foobar');

And maybe create a new module random/random that is used by all the methods inside the random package (avoid calling Math.random() directly), so replacing it would be enough to propagate the change to all random methods:

define(function(){
   function random(){
     return random.random();
   }
   // expose a `random` property so user can swap the behavior dynamically
   random.random = Math.random;
   return random;
});

Could also use the RequireJS map config to replace the amd-utils/random/random module, but I like the idea of exposing the random property since on node.js there is no such a thing as a map config.

PS: that way we also avoid any licensing issues.

jeffrose commented 11 years ago

I definitely like the flexibility that having a separate module, random/random, provides. I assume the default implementation will still be Math.random(). There should probably be some comment about the unreliability of Math.random() especially for UUID generation.

Having said that random/random looks a little funny. Maybe random/gen? random/generator? random/ng? random/prng?

millermedeiros commented 11 years ago

I added the random/random, since I think it will only be used internally (easier to call Math.random directly) I think the name is good enough.

I didn't implemented the randInt.create(), choice.create(), etc.. or added a new property random to each method since I think it will increase complexity a lot and the global replacement should be enough for most use cases (overriding amd-utils/random/random behavior).

I improved the documentation a lot adding info on the top about Math.random usage and also improved the guid documentation to mention safety problems. random/random gets into a lot of details so it should be clear to everyone: http://millermedeiros.github.com/amd-utils/random.html

Thanks for all the feedback and for helping me to improve amd-utils. Feel free to reopen the issue.

millermedeiros commented 11 years ago

for future reference this article is great: http://www.empiricalzeal.com/2012/12/21/what-does-randomness-look-like/

jeffrose commented 11 years ago

Thanks! I'll check it out.