phetsims/dot

issues with Random.js

Closed this issue · 5 comments

pixelzoom commented

Some issues with Random.js

  • this.seed is not defined in constructor and has no visibility annotation where it's defined in setSeed
  • this.seedrandom is not defined in constructor and has no visibility annotation where it's defined in setSeed
  • missing visibility annotations for several methods
  • there is no getSeed method
  • Math.setrandom is not documented, and apparently is a 3rd-party monkey patch
pixelzoom commented

All issues addressed in the above commit. @ariel-phet please assign someone to review. The authors listed in the source code are:

 * @author John Blanco (PhET Interactive Simulations)
 * @author Aaron Davis (PhET Interactive Simulations)
 * @author Sam Reid (PhET Interactive Simulations)
 * @author Mohamed Safi
ariel-phet commented

@samreid will review

samreid commented

The commits look straightforward and correct. I additionally converted to an ES6 class and used an arrow function (after confirming there are no inherit( Random occurrences.

It looks like there is an additional opportunity to clean up because there is no usage of setSeed. I'll look at that next.

samreid commented

Review complete, everything looks good. Back to @pixelzoom to review changes made during the review.

pixelzoom commented

👍🏻 closing.