Add mutable functions to Range

[chrisklus]

From slack:

Chris Klusendorf [4:04 PM]
@jbphet and I are thinking of adding some mutable functions to Range.js, likely setMin(), setMax(), and setMinMax(). Any objections?

Michael Kauzmann [4:04 PM]
No objections here. I like the idea.

Chris Malley [4:30 PM]
what’s the use case?

Chris Klusendorf [4:40 PM]
I’m refactoring HorizontalSurface (the top and bottom surface of a movable element) in EFAC, which is going to have a position and a range, so we’d like to update that range on drag instead of reallocating

Sam Reid [6:04 PM]
Seems reasonable to me. Vector2 and Bounds2 are mutable, so it seems reasonable for Range to be mutable too.

Chris Malley [6:05 PM]
Don’t forget about subtype RangeWithValue. If you mutate min or max such that defaultValue is out of range, boom.
That’s going to be a tricky assertion to pull off.
If it were me, I’d probably leave Range and RangeWithValue alone, and make HorizontalSurface use {min:number, max:number} instead of Range.
… unless you need other things that Range provides.

Chris Klusendorf [6:10 PM]
Gotcha, yeah we thought about that but are using some of Range’s methods so were thinking it’d be nice to just change Range. I wasn’t aware of RangeWithValue though, so I will investigate how much it would take to refactor usages of HorizontalSurface with {min:number, max:number}.

Chris Malley [6:12 PM]
In addition to adding setMin and setMax to Range, you’ll need to override in RangeWithValue, something like:

/**
* Sets the range’s minimum.
* @param {number} min
* @public
* @override
*/
setMin: function( min ) {
 assert && assert( this.defaultValue >= min, ‘min breaks defaultValue: ’ + min );
 Range.prototype.setMin.call( this, min );
} 

… and similarly for setMax.

Chris Klusendorf [6:13 PM]
nice, thanks!

Chris Malley [6:14 PM]
Also change fields (in Range) to _min, _max, then define setMin, getMin, setMax, getMax and associated ES5 setters. (edited)

Sam Reid [6:14 PM]
RangeWithValue.equals looks buggy
Can you fix that while you’re in there?

Chris Malley [6:15 PM]
yes, it does. looks like someone missed a this.defaultValue === other.defaultValue test.
and it should be other instanceof RangeWidthValue
toString is also buggy, it prints [Range…

[chrisklus]

@pixelzoom please review at your convenience.

I was not sure if I should use this._min and this._max for every internal use case or to instead leave them as this.min and this.max for the internal read cases. I decided on _ for all cases so that it was consistent and slightly better performance.

[pixelzoom]

Based on problems reported on Slack, I'm guessing that this is not ready for review at the moment. Please assign back to me when it's ready.

[chrisklus]

Now ready for review @pixelzoom. To add to my question/thinking in #77 (comment), I realized that only makes sense in Range.js itself (I think), so I reverted the _ usages in RangeWithValue in the above commit.

[pixelzoom]

I made some changes, see above commit.

Three corrections:

• visibility of _min and _max should be @private, not @public
• assertions should validate constructor args, and should be the first line in the constructor
• Range equals needs to check ( other instanceof Range )
• RangeWithValue equals should chain to Range equals, instead of duplicating validation of min and max

One subjective change:

• replaced verbage like "greater than or equal to" with ">=" in comments

One recommendation:

• create unit tests RangeTests and RangeWithValueTests

Two things to discuss:

• The current implementations of equals results in the behavior shown in the example below. Is this desired behavior? Should we be comparing constructors in equals instead of using instanceof?

const r1 = new phet.dot.Range( 0, 1 );
const r2 = new phet.dot.RangeWithValue( 0, 1, 0.5 );
r1.equals( r2 ) -> true
r2.equals( r1 ) -> false

• Defaulting defaultValue to min made sense when that feature was part of Range. When it was broken into Range and RangeWithValue (see Why do Ranges have default values? #57) we ended up with RangeWithValue uses that really should be Range. Clients relying on the defaulting of defaultValue would be better served by using Range, see example below. So I propose that defaultValue should be made a required parameter.

const range = new RangeWithValue( -10, 10 );
...
something = range.defaultValue;  // obfuscates 
something = range.min; // much clearer what it going on 

[pixelzoom]

Labeling for developer meeting to discuss the "Two things to discuss" in the previous comment.

[jonathanolson]

The current implementations of equals results in the behavior shown in the example below. Is this desired behavior? Should we be comparing constructors in equals instead of using instanceof?

Yes, generally. We have usually avoided equals() in things that get inherited, but there are many cases where we don't have constructor/type checks.

[pixelzoom]

@jonathanolson Which question are you answering "yes" to?

[pixelzoom]

The defaultValue parameter is now required for RangeWithValue, completed in #78.

[pixelzoom]

Range and RangeWithValue equals now use this comparison for type equivalence:

( object.constructor === this.constructor )

[pixelzoom]

The remaining bit of work for this issue is unit tests RangeTests and RangeWithValueTests.

[pixelzoom]

Something this fundamental should not be left "in progress" for this long. So I've raised the priority to "high". @chrisklus let me know if you need any help finishing this up.

[chrisklus]

Thanks for the help @pixelzoom, please review.

Since min and max for Range and RangeWithValue are inclusive, I wasn't sure if I should test the cases where, for example, min === max or min < defaultValue === max for all of the method tests (setMin(), setMax(), etc.). Maybe I should at least add = to the comparison operators in the testing messages so that they read more correctly, e.g. setMax() succeeds when max >= defaultValue >= min instead of setMax() succeeds when max > defaultValue > min?

[pixelzoom]

Yeah, messages should be "<=" and ">=". I didn't realize the min === max was a valid Range. As for which tests to add... Technically, you want to be as complete as possible, and that means testing boundary conditions (as you've noted). That said, I don't know how "complete" PhET wants to (or needs) to be. I'm not a fan of "test driven development", where there's a test case for everything possible. Maybe a topic for developer meeting discussion...

[chrisklus]

Technically, you want to be as complete as possible, and that means testing boundary conditions.

Yeah, I'm leaning towards this as well. Might be nice to discuss at dev meeting for general/future reference though, as you said.

For now, I've updated the messages and will mark for discussion.

[chrisklus]

From 01/10/19 dev meeting.

@jonathanolson said:

For something used this much, probably best to exhaustively test all cases.

@samreid said:

Write tests for all cases if it doesn't require a large addition of time.

I'll go ahead and add in the rest of the "or equal to" cases.

[chrisklus]

Boundary condition tests added. @pixelzoom please review.

[pixelzoom]

👍 I skimmed the commits and verified that the unit tests are passing. Closing.