phetsims / number-line-distance

"Number Line: Distance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 3 forks source link

Should TemperaturePointController colorProperty be a DerivedProperty? #51

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

From code review #35:

  • [ ] Are all dependent Properties modeled as DerivedProperty instead of Property?

In TemperaturePointController.js:

    // @public (read-only) {PaintColorProperty} is different from this.color where this.color is the point controller's
    // color for the purposes of the other number-line classes; this colorProperty instead is just representative of
    // the temperature of this point controller. No unlink necessary since all TemperaturePointControllers are present
    // for the sim's lifetime.
    this.colorProperty = new PaintColorProperty( NO_TEMPERATURE_COLOR );
    this.positionProperty.link( () => {
      if ( this.isControllingNumberLinePoint() ) {
        this.colorProperty.value = temperatureToColorMap( this.numberLinePoints[ 0 ].valueProperty.value );
      }
      else {
        this.colorProperty.value = NO_TEMPERATURE_COLOR;
      }
    } );

The only place that I seecolorProperty.valueset is in this observer. So it seems likecolorProperty ` should be derived, something like:

this.colorProperty = new DerivedProperty( [ this.positionProperty ],
  () => if ( this.isControllingNumberLinePoint() ) {
        return temperatureToColorMap( this.numberLinePoints[ 0 ].valueProperty.value );
      }
      else {
        return NO_TEMPERATURE_COLOR;
      }
} );
SaurabhTotey commented 3 years ago

The colorProperty should indeed be a DerivedProperty, so I made that change.