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

duplicated constants for phase value #70

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

In Ball, values are defined for the "phase" of a ball's journey from hopper to bin:

20  var PHASE_INITIAL = 0;         // ball leaving hopper
21  var PHASE_FALLING = 1;         // ball falling within bounds of board
22  var PHASE_EXIT = 2;            // ball exits the lower bounds of board and enters the bins
23  var PHASE_COLLECTED = 3;       // ball lands in final position

43  this.phase = PHASE_INITIAL; // @public (read-only) see PHASE_* constants

I noticed that this.phase is marked as @public, but none of the values are part of the public API. Looking around I find that these constants duplicated in multiple places:

// BallsNode
20  var PHASE_COLLECTED = 3; // the ball has landed on its final position

// LabModel
19 var PHASE_LANDED = 3;
20 var PHASE_EXIT = 2;

Note that LabModel.PHASE_LANDED even has a different name than PHASE_COLLECTED.

This is not acceptable.

pixelzoom commented 7 years ago

There are a couple of ways I could have addressed this. I could have changed the phase values to strings, then used string literals everywhere (as with hopperModeProperty and histogramModeProperty). But since these were already integer values, using the enumeration pattern seemed to be in the spirit of the current implementation, and was the least disruptive. So the phase values have been factored out into enumeration BallPhase.

Closing.