davidbau / seedrandom

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

Clean up #15

Closed TimothyGu closed 9 years ago

TimothyGu commented 9 years ago

Hi,

The seedrandom.js, is, for a lack of a better word, a freaking maze.

Comments

First, the extra-long comment in the beginning of the file that takes literally more than half the file is already covered completely in README.md. Why spend the time to duplicate it in the source?

Second, the intermixing of (useless) jsdoc comments with // comments is extremely unpleasant to the eye.

/**
 * All code is in an anonymous closure to keep the global namespace clean.
 */

... OK? This is not JSDoc at all.

Then there is

/** @constructor */
function ARC4(key) {
  [...]
}

Yes, we know, it's a constructor.

Closure

Then, there is the closure:

(function (
    global, pool, math, width, chunks, digits, module, define, rngname) {
[...]
})(
  this,   // global window object
  [],     // pool: entropy pool starts empty
  Math,   // math: package containing random, pow, and seedrandom
  256,    // width: each RC4 output is 0 <= x < 256
  6,      // chunks: at least six RC4 outputs for each double
  52,     // digits: there are 52 significant digits in a double
  (typeof module) == 'object' && module,    // present in node.js
  (typeof define) == 'function' && define,  // present with an AMD loader
  'random'// rngname: name for Math.random and Math.seedrandom
);

Why not just declare all of them in the closure itself? This is not making it customizable or anything.

Node.js integration

  try {
    // When in node.js, try using crypto package for autoseeding.
    nodecrypto = require('crypto');
  } catch (ex) {}

Is there a reason why this is at the end of the file, ~200 lines from var nodecrypto declaration?

If you want, I volunteer to fix all these issues. I just thought that the code is utterly unreadable.