issues with Random.js - Githubissues

phetsims / dot

A math library with a focus on mutable and immutable linear algebra for 2D and 3D applications.

http://scenerystack.org/

MIT License

13 stars 6 forks source link

issues with Random.js #103

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Some issues with Random.js

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

pixelzoom commented 4 years ago

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 4 years ago

@samreid will review

samreid commented 4 years ago

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 4 years ago

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

pixelzoom commented 4 years ago

👍🏻 closing.