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

replace RangeWithValue with Range where appropriate #78

Closed pixelzoom closed 6 years ago

pixelzoom commented 6 years ago

In https://github.com/phetsims/dot/issues/57, Range was split into Range and RangeWithValue. In https://github.com/phetsims/dot/issues/57#issuecomment-233428450, @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 https://github.com/phetsims/dot/issues/77#issuecomment-426851631, 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.

jonathanolson commented 6 years ago

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

pixelzoom commented 6 years ago

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' ); }

pixelzoom commented 6 years ago

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.

pixelzoom commented 6 years ago

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.

jbphet commented 6 years ago

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