phetsims / curve-fitting

"Curve Fitting" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

use Property validation options #150

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

Related to this item in code review #143:

  • [ ] Are Property value validation options (valueType, validValues, etc...) utilized? Is their presence or lack thereof properly documented?

There are currently no usages of validation options in this sim. This info is important for catching programming errors, and for future PhET-iO instrumentation.

For example, in Curve.js:

      // @public {Property.<number>} r^2 deviation value, a number ranging from 0 to 1
      this.rSquaredProperty = new NumberProperty( 0 );

should be:

      // @public {Property.<number>} r^2 deviation value
      this.rSquaredProperty = new NumberProperty( 0, {
        range: new Range( 0, 1 )
      }  );

In Point.js:

      this.isInsideGraphProperty = new DerivedProperty(
        [ this.positionProperty ],
        position => CurveFittingConstants.GRAPH_BACKGROUND_MODEL_BOUNDS.containsPoint( position )
      );

should be:

      this.isInsideGraphProperty = new DerivedProperty(
        [ this.positionProperty ],
        position => CurveFittingConstants.GRAPH_BACKGROUND_MODEL_BOUNDS.containsPoint( position ), {
      valueType: 'boolean'
     } );

So....

pixelzoom commented 5 years ago

Using validation options will also make assertions like this in CurveFittingModel unnecessary:

      // validate Property values and update curve fit
      this.orderProperty.link( order => {

        // ensure the order is 1, 2 or 3: linear, quadratic or cubic
        assert && assert( order === 1 || order === 2 || order === 3, `invalid order: ${order}` );
...
SaurabhTotey commented 5 years ago

I have now added more property validation. Assigning to @pixelzoom to review.

pixelzoom commented 5 years ago

Looks good, closing.