davidbau / seedrandom

seeded random number generator for Javascript
2.06k stars 160 forks source link

Should not override Math.random #1

Closed qbolec closed 10 years ago

qbolec commented 11 years ago

I see that you enclosed your code in a self-executing anonymous function, so I believe that you are aware of problems with global scope polution.

Even if this is a 99% of use-cases, I would never say that overwriting the Math.random globally is a good idea. Observe that adding a single line

Math.random = seedrandom();

would not be complicated for these 99% users, while adding

var realRandom=Math.random;
Math.seedrandom(...);
var newRandom=Math.random;
Math.random=realRandom;

is a bit complicated and not even guaranteed to work for this 1%.

My usecase is that I am not sure what other widgets are actually using Math.random for and I wouldn't like to interefere with their execution.

davidbau commented 10 years ago

Two questions. (1) Why wouldn't the code snippet you suggest work (guaranteed)? In a single-threaded environment, there shouldn't be any danger that anybody will access Math.random while you are swapping it, right?

(2) But I agree, it's ugly, and when you don't want to touch it, it would be nice to avoid touching Math.random at all. How about this calling convention? A third argument to Math.seedrandom which may be a callback function. If it is supplied, then Math.random is not touched, and instead the function is called with the new PRNG (as argument 1) and the compact seed (as argument 2), and you can do what you like with them. If the callback returns a value, that value is also returned as the return value from seedrandom itself.

So for example, you could write (assuming underscore.js "_.identity" is equal to function(x) { return x; }).

var myprng = Math.seedrandom("hello", false, _.identity);

Without the third argument, seedrandom would behave as today. Since seedrandom doesn't currently have a third argument, this convention shouldn't break existing code.

ryandotsmith commented 10 years ago

:+1:

I would like to use something like this:

var r = Math.seedrandom("hello");
r.random()
ryandotsmith commented 10 years ago

I ended up modifying the source as such:

//...
math['seededRandom'] = function() {
//...
davidbau commented 10 years ago

Yes, sure; but that would break existing users of seedrandom. (The current API returns the seed, not the PRNG.)

I'd like to update the release to address this concern (a) without breaking any existing uses of seedrandom.js and (b) without adding more than a few bytes to the file.

What do you think of the API that I suggested earlier on the thread? I.e., try the version on http://davidbau.com/encode/seedrandom-test.js

On Thu, Dec 19, 2013 at 5:34 PM, Ryan Smith notifications@github.comwrote:

[image: :+1:]

I would like to use something like this:

var r = Math.seedrandom("hello");r.random()

— Reply to this email directly or view it on GitHubhttps://github.com/davidbau/seedrandom/issues/1#issuecomment-30973202 .

ryandotsmith commented 10 years ago

I can always appreciate maintaining backwards compatibility. And it is for that reason that I don't have a lot of good solutions for this problem. I suppose your proposed API is OK. It still requires a bit of book keeping to be done my the user. The tedious act of setting/un-setting the seed on a global object worries me. I am afraid that myself or someone else on the project might not remember to unset the seed and then other parts of our program will be advancing our random sequence thus providing an unsafe environment.

ryandotsmith commented 10 years ago

I would like to suggest the Golang's implementation as prior art. http://golang.org/pkg/math/rand/#Rand Here is an example of a seeded random number generator in Go: http://play.golang.org/p/gNjp04D8d0

ryandotsmith commented 10 years ago

FIWIW: I ended up using a JS implementation of Mersenne twister.

https://gist.github.com/banksean/300494

davidbau commented 10 years ago

Mersenne is a fine prng, but not designed to be cryptographically strong; also be aware that your impl is just 32 bits, so you're not getting a full precision double random.

I am experimenting with backward-compatible APIs. Here is an idea: use "new" to get what we want without breaking old users.

I have modified http://davidbau.com/encode/seedrandom-test.js to try out the following convention:

(1) Math.seedrandom(); with zero, one, or two args unchanged from before, so old users can upgrade without having to think about breakage. (2) Except that a "null" seed behaves the same as omitting it, as requested in the other issue and as programmers should expect. (3) Here is what is new: var prng = new Math.seedrandom(...); using the "new" now returns a prng closure without mutating Math.random. var r = prng(); gives [0..1]. (4) It also adds support for module imports in node.js and require.js/AMD loaders, with an API that doesn't mutate Math.random.

The new API costs 192 bytes more (18% increase, from 1045 -> 1237) in the uglified code.

On Fri, Dec 20, 2013 at 3:13 PM, Ryan Smith notifications@github.comwrote:

FIWIW: I ended up using a JS implementation of Mersenne twister.

https://gist.github.com/banksean/300494

— Reply to this email directly or view it on GitHubhttps://github.com/davidbau/seedrandom/issues/1#issuecomment-31038172 .

davidbau commented 10 years ago

Released v2.3 with this new API and marking this issue as fixed. Old callers still work the same. New callers can create a local PRNG using "new" as follows:

var myrng = new Math.seedrandom('seed.'); document.write(myrng());

For those being really modern and clean, we can now also load this library via AMD or node.js require.

Protonk commented 10 years ago

For those being really modern and clean, we can now also load this library via AMD or node.js require.

Yay! Thanks.