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

Avoid valueProperties.concat in multilink. #53

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

For code review #35.

In DistanceStatementNode.js:

    Property.multilink(
      valueProperties.concat( [
        model.distanceRepresentationProperty,
        model.isPrimaryControllerSwappedProperty,
        model.numberLine.orientationProperty
      ] ),
      ( value0, value1, distanceRepresentation, isPrimaryNodeSwapped, orientation ) => {

The way that the dependencies array is set up is feels kind of dangerous, and is not at all obvious. valueProperties is defined far away, I have to do looking for it. The number of dependenceis does not obviously equal the number of parameters in the observer. And it assumes that valueProperties will forever contain only 2 NumberProperties, something that could break.

This is more robust and more obvious:

    assert && assert( valueProperties.length === 2 );
    Property.multilink(
      [
        valueProperties[ 0 ],
        valueProperties[ 1 ],
        model.distanceRepresentationProperty,
        model.isPrimaryControllerSwappedProperty,
        model.numberLine.orientationProperty
      ] ),
      ( value0, value1, distanceRepresentation, isPrimaryNodeSwapped, orientation ) => {
SaurabhTotey commented 3 years ago

I made the change, so I will close this issue.