phetsims / plinko-probability

"Plinko Probability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
5 stars 7 forks source link

problems with Ball model #80

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

In Histogram:

    /**
     * Updates the array elements for the number of balls in a bin and the horizontal final position of the last ball.
     *
     * @param {IntroBall|LabBall} ball
     * @public
     */
    updateBinCountAndOrientation: function( ball ) {
      this.bins[ ball.binIndex ].binCount++;
      this.bins[ ball.binIndex ].orientation = ball.binOrientation;
    },

This function indicates that ball has type {IntroBall|LabBall}, and it accesses ball.binOrientation.

Problems:

(1) LabBall has no binOrientation field. So this is likely working only because binOrientation is undefined and is getting coerced to 0. Add an assertion to verify.

(2) If ball really is of type {IntroBall|LabBall}, then it should be of type {Ball}, since those are the only 2 subtypes of Ball. And the binOrientation field should be part of the Ball subtype.

In IntroBall:

    // @public {number} describes final vertical offset of ball within a bin {number}
    this.finalBinVerticalOffset = yMinimum + ( ( binStackLevel - 1 ) * delta ) - this.ballRadius;

    // @public {number} describes final horizontal offset of the ball within a bin {number}
    this.finalBinHorizontalOffset = ( this.binOrientation * ( ( cylinderInfo.cylinderWidth / 2 ) - this.ballRadius ) );

... and in LabBall:

    // describes final vertical position offset (measure from the bottom of the galton board) of ball within a bin {number}
    this.finalBinVerticalOffset = yMin;

    // describes final horizontal offset (measured from the middle position) of ball within a bin {number}
    this.finalBinHorizontalOffset = 0;

Problems:

(3) These 2 fields (finalBinVerticalOffset and finalBinHorizontalOffset) are owned by supertype Ball. They should therefore be initialized via options, so that initialization takes place once, in the supertype constructor.

pixelzoom commented 7 years ago

Passing initial values via options was not possible, since those values are computed based on other things (e.g. radius, binCount) initialized by Ball (supertype) constructor. I considered passing in functions to compute the initial values, but that got complicated and I ran into ordering/dependency issues. So I decided to resolve this issue by simply documenting that these fields are owned by the supertype, and explaining why their values are changed by the subtypes.

Closing.