phetsims/dot

replace RangeWithValue with Range where appropriate

Closed this issue · 5 comments

In #57, Range was split into Range and RangeWithValue. In #57 (comment), @mbarlow12 said:

To minimize disruption, RangeWithValue has replaced Range everywhere, even where only two parameters are passed in. Moving forward, the plan should be to gradually replace the existing RangeWithValue instances with Range when it can be verified that no default value is being used. ...

The "gradually replace" part has apparently never happened. There are occurrences of RangeWithValue that should be using Range. That became obvious when reviewing change to Range and RangeWithValue in #77 (comment), wherein I proposed that the defaultValue parameter to RangeWithValue should be required.

Should this be something that we "chip away" at? I'm planning to address it immediately in all of my sims, because I think it will take me < 30 minutes.

It's something I've brought up in code reviews. I'm fine with a chip-away.

How about doing this all at once using this approach? :

(1) Change anything new RangeWithValue that has 2 args to Range
(2) Add a temporary ES5 getter defaultValue to Range that errors when it's called, i.e.:

get defaultValue() { throw new Error( 'defaultValue is undefined' ); }

I completed conversion of all my sims and the common-code demos. Since that went so fast, I'll look at doing all other sims.

All sims have been converted.

I added this method to Range, to catch programming errors:

    /**
     * In https://github.com/phetsims/dot/issues/57, defaultValue was moved to RangeWithValue.
     * This ES5 getter catches programming errors where defaultValue is still used with Range.
     */
    get defaultValue() {
      throw new Error( 'defaultValue is undefined, did you mean to use RangeWithValue?' );
    }

And I made defaultValue a required parameter for RangeWithValue constructor:

  /**
   * @param {number} min - the minimum value of the range
   * @param {number} max - the maximum value of the range
   * @param {number} defaultValue - default value inside the range
   * @constructor
   */
  function RangeWithValue( min, max, defaultValue ) {

    Range.call( this, min, max );

    assert && assert( defaultValue !== undefined, 'default value is required' );
    assert && assert( defaultValue >= min && defaultValue <= max, 'defaultValue is out of range: ' + defaultValue );

    // @private
    this._defaultValue = defaultValue;
  }

Leaving this labeled for developer meeting so that developers are aware of these changes.

Reviewed in 10/4/2018 dev meeting, changes approved, awareness distributed, closing.