problems with Random.randomIntegerBetween

[pixelzoom]

(1) Documentation

Random line 118:

// inclusive, to match underscore
randomIntegerBetween: function( min, max, floaty ) {

Lots of questions here... Why does this mention underscore? Do you mean lodash? And exactly which function are you matching?

I think what you probably mean is:

/**
 * This is a replacement for lodash's _.random function.
 * @param {number} min
 * @param {number} max
 * @param {boolean} falsy - not currently supported!
 * @returns {number} a value between min and max, inclusive
 */   
randomIntegerBetween: function( min, max, floaty ) {

(2) Return type and function name

Note that lodash's _.random documentation (https://lodash.com/docs#random) says:

If floating is true, or either lower or upper are floats, a floating-point number is returned instead of an integer.

Whether intentional or not, this seems to be supported in the current implementation. Testing in the console:

phet.joist.random.randomIntegerBetween( 1, 5 ) 2 phet.joist.random.randomIntegerBetween( 1.3, 5.2 ) 3.3

So randomIntegerBetween is a poor name for this, because it doesn't necessarily return an integer.

[pixelzoom]

Related to https://github.com/phetsims/phet-io/issues/551.

[pixelzoom]

My recommendation would be to rename randomIntegerBetween to nextBetween.

[pixelzoom]

There are currently 68 usages of randomIntegerBetween.

[pixelzoom]

And while we're at it... Maybe we should simplify this by requiring both min and max params, and just getting rid of the falsy param. I don't see a lot of value in having the same function signature as _.random, and it adds complexity.

[pixelzoom]

Of the 68 usages of randomIntegerBetween, I see only 3 (all in fraction-matcher) that are omitting the min arg. And (imo) those should all be using nextInt instead.

E.g. LevelModel line 111:

var scaleFactor = numericScaleFactors[ phet.joist.random.randomIntegerBetween( numericScaleFactors.length - 1 ) ]; //random scaleFactor

... would be clearer as:

var scaleFactor = numericScaleFactors[ phet.joist.random.nextInt( numericScaleFactors.length ) ]; //random scaleFactor

[samreid]

In the above commits, I:

• Improved JSDoc for Random.randomIntegerBetween
• Rename Random.randomIntegerBetween to Random.nextBetween
• Enforce 2-args for Random.nextBetween

I see only 3 (all in fraction-matcher) that are omitting the min arg. And (imo) those should all be using nextInt instead.

I haven't done this yet, instead I converted the 1-arg calls to use nextBetween( 0, .... After doing this, I see 9 cases that match this pattern, all of which are candidates for nextInt().

phet.joist.random.randomIntegerBetween( 1.3, 5.2 ) 3.3

I'm somewhat worried about this because with that method name it seems like it could return any floating point number between 1.3 and 5.2. Whereas with the implementation, it could only return integral steps above the min, which are: 1.3, 2.3, 3.3, 4.3, 5.3. NOTE this last value is out of range. Should we rename the method to indicate it is designed for integers? Should we assert that the values passed in are integers? Should we provide another method designed for floating point? It would be crazy to provide one method with this behavior: nextBetween(0,3) => 0,1,2,3 nextBetween(0.5,0.6) => 0.500000000001, 0.50000000000002, etc.... 0.599999999, 0.6

Right now my only remedy for this situation is the elaboration in the JSDoc which says:

* Randomly select a random value an integral number of steps above min (inclusive).

But I am concerned with a name like nextBetween() this method will be misinterpreted and used incorrectly for floating points.

[samreid]

It would be crazy to provide one method with this behavior

WOW, this is exactly what lodash does:

This sounds like bugs waiting to happen.

[pixelzoom]

My vote would be to require nextBetween to have integer args, and rename it nextIntBetween, for symmetry with nextInt.

[samreid]

Fixed above, @pixelzoom can you take a peek?

[pixelzoom]

Replaced Number.isInteger with DOT.Util.isInteger. Back to @samreid for review.

[pixelzoom]

@samreid I still see 10 usages of randomIntegerBetween and 2 sims are failing (BLL, RPAL). Looks like you missed some renaming.

[pixelzoom]

randomIntegerBetween renamed to nextIntBetween for BLL and RPAL in above commits.

[samreid]

I ran the load tester after your changes and everything seems like it is working now, and usage of DOT.Util.isInteger looks good. Anything else to do for this issue?

[pixelzoom]

Nothing else to do, closing.