Self code review for common model directory, see: #258

js/common/model/Plate.ts

@@ -51,14 +51,15 @@ export default class Plate<T extends Snack> extends PhetioObject {
   // The X position of the center of this plate relative to the center of the table.
   public readonly xPositionProperty: TProperty<number>;
 
-  // The number of snacks (candy bars or apples) on that table representation of this plate.
+  // The number of snacks (candy bars or apples) on the table representation of this plate.
   public readonly tableSnackNumberProperty: Property<number>;
 
   // The list of snacks that are on this plate in the notepad.  Depending on what the user has done, this may or may not
   // be in sync with the number on the table.  DO NOT MODIFY THE CONTENTS OF THIS ARRAY OUTSIDE OF THIS CLASS.  It's
   // only public so that clients can get to the length and lengthProperty.
   public readonly snacksOnNotepadPlate: ObservableArray<T>;
 
+  // Whether the number of snacks on the table and in the notepad are in sync.
   public readonly snacksInSyncProperty: TReadOnlyProperty<boolean>;
 
   // The plate's index, 0-indexed.  This is primarily used for debugging.
@@ -104,25 +105,22 @@ export default class Plate<T extends Snack> extends PhetioObject {
     this.isActiveProperty = new BooleanProperty( options.initiallyActive, {
 
       // phet-io
-      tandem: options.tandem.createTandem( 'isActiveProperty' ),
-
       // Takes its value from DistributeModel.numberOfPeopleProperty, so cannot be independently adjusted.
-      phetioReadOnly: true
+      phetioReadOnly: true,
+      tandem: options.tandem.createTandem( 'isActiveProperty' )
     } );
 
     this.xPositionProperty = new NumberProperty( options.initialXPosition );
 
     // So that reset of isActiveProperty and reset of tableSnackNumberProperty are in agreement, make sure their initial
     // states are compatible.
     const initialTableSnackNumber = options.initiallyActive ? options.startingNumberOfSnacks : 0;
-
     this.tableSnackNumberProperty = new NumberProperty( initialTableSnackNumber, {
       range: new Range( 0, 10 ),
-
-      // phet-io
       tandem: options.tandem.createTandem( 'tableSnackNumberProperty' )
     } );
 
+    // Create the observable array that tracks the snacks a notepad plate has.
     this.snacksOnNotepadPlate = createObservableArray( {
       phetioType: ObservableArrayIO( ReferenceIO( IOType.ObjectIO ) ),
       tandem: options.tandem.createTandem( 'snacksOnNotepadPlate' )
@@ -145,8 +143,6 @@ export default class Plate<T extends Snack> extends PhetioObject {
           this.snacksOnNotepadPlate.removeItemRemovedListener( snackRemovedListener );
         }
       };
-
-      // REVIEW: Do we not need a listener here to set the isActiveProperty to false when the snack is removed?
       this.snacksOnNotepadPlate.addItemRemovedListener( snackRemovedListener );
     } );
 
@@ -185,8 +181,6 @@ export default class Plate<T extends Snack> extends PhetioObject {
    * Get the position for the snack at the provided index position in the stack.
    */
   public getPositionForStackedItem( stackIndex: number ): Vector2 {
-
-    // Get the position.
     return this.snackStackingFunction( this.xPositionProperty.value, stackIndex );
   }
 
@@ -356,7 +350,7 @@ export default class Plate<T extends Snack> extends PhetioObject {
   }
 
   /**
-   * Remove all the snacks from this plate.  Has no effect if there are none.
+   * Remove all the snacks from this plate. Has no effect if there are none.
    */
   public removeAllSnacks(): void {
     while ( this.getNumberOfNotepadSnacks() > 0 ) {

js/common/model/SharingModel.ts

@@ -51,15 +51,17 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
   public readonly meanInfoPanelVisibleProperty: Property<boolean>;
   public readonly totalVisibleProperty: Property<boolean>;
   public readonly meanProperty: TReadOnlyProperty<number>;
+
+  // Tracks whether all active notepad plates are in sync with their ground truth (table) values.
   public readonly activePlatesInSyncProperty: TReadOnlyProperty<boolean>;
 
   // A state flag used to control whether the motion of snacks is animated or instantaneous.  This is helpful for
   // preventing animations during phet-io state setting.
   public animateAddedSnacks = false;
 
   // This ObservableArray is used to keep track of snacks that are not in use and are thus available to be moved to a
-  // plate or elsewhere. These are generally inactive and not visible in the view.  Removing a snack from this array will
-  // cause it to be activated, adding to the array will cause it to be deactivated.
+  // plate or elsewhere. These are generally inactive and not visible in the view. Adding a snack to this array will
+  // cause it to be deactivated.
   protected readonly unusedSnacks: ObservableArray<T>;
 
   // Allows PhET-iO clients to modify the max number of plates in the screen.
@@ -79,31 +81,22 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
 
     super( options );
 
+    // Create Properties
     this.numberOfPlatesRangeProperty = new Property<Range>( NUMBER_OF_PLATES_RANGE );
-
     this.maxPlatesProperty = new NumberProperty( MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_DATA_SETS, {
       numberType: 'Integer',
       range: NUMBER_OF_PLATES_RANGE,
       tandem: options.tandem.createTandem( 'maxPlatesProperty' )
     } );
-
     this.numberOfPlatesProperty = new NumberProperty( MeanShareAndBalanceConstants.INITIAL_NUMBER_OF_PEOPLE, {
       numberType: 'Integer',
       range: this.numberOfPlatesRangeProperty,
-
-      // phet-io
       tandem: options.tandem.createTandem( 'numberOfPlatesProperty' )
     } );
-
     this.meanInfoPanelVisibleProperty = new BooleanProperty( false, {
-
-      // phet-io
       tandem: options.tandem.createTandem( 'meanInfoPanelVisibleProperty' )
     } );
-
     this.totalVisibleProperty = new BooleanProperty( false, {
-
-      // phet-io
       tandem: options.tandem.createTandem( 'totalVisibleProperty' )
     } );
 
@@ -139,7 +132,6 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
     // Create the set of plates that will hold the snacks.
     assert && assert( options.initialPlateValues.length === MAX_PLATES, 'initialPlateValues must have the same length as the number of plates' );
     this.plates = [];
-
     const platesParentTandem = options.tandem.createTandem( 'plates' );
     _.times( MAX_PLATES, plateIndex => {
       const initialXPosition = plateIndex * INTER_PLATE_DISTANCE;
@@ -160,7 +152,6 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
       );
       this.plates.push( plate );
     } );
-
     const activePlateDependencies = this.plates.map( plate => plate.isActiveProperty );
     const plateSyncDependencies = this.plates.map( plate => plate.snacksInSyncProperty );
     this.activePlatesInSyncProperty = DerivedProperty.deriveAny( [ ...activePlateDependencies, ...plateSyncDependencies ],
@@ -190,7 +181,7 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
       }
     );
 
-    // Monitor the number of active plates/people and update the plate positions to keep them centered.
+    // Monitor the number of active plates and update the plate positions to keep them centered.
     this.numberOfPlatesProperty.link( numberOfPlates => {
       const totalSpan = Math.max( ( numberOfPlates - 1 ) * INTER_PLATE_DISTANCE, 0 );
       const leftPlateCenterX = -( totalSpan / 2 );
@@ -243,13 +234,13 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
    * Get the plate on which the provided snack is currently sitting.  Returns null if the snack is not on a plate.
    */
   public getPlateForSnack( snack: T ): Plate<T> | null {
-    let returnVal = null;
-    for ( const plate of this.plates ) {
+    let currentPlate = null;
+    this.plates.forEach( plate => {
       if ( plate.hasSnack( snack ) ) {
-        returnVal = plate;
+        currentPlate = plate;
       }
-    }
-    return returnVal;
+    } );
+    return currentPlate;
   }
 
   /**
@@ -307,6 +298,7 @@ export default class SharingModel<T extends Snack> extends PhetioObject implemen
             }
           } );
 
+          // Restore the state of the data before setting the new data points.
           this.resetData();
           this.numberOfPlatesProperty.value = dataPoints.length;
 

js/common/model/Snack.ts

@@ -24,19 +24,18 @@ type SelfOptions = {
 export type SnackOptions = SelfOptions & WithRequired<PhetioObjectOptions, 'tandem'>;
 
 // constants
-const TRAVEL_SPEED = 300; // in screen coordinates per second, empirically determined to look decent
+const TRAVEL_SPEED = 300; // In screen coordinates per second, empirically determined to look decent.
 
-// Total number of snack allocated, for debugging.
-let instanceCount = 0;
+let instanceCount = 0; // Total number of snack allocated, for debugging.
 
 export default class Snack extends PhetioObject {
 
   // This Property controls the snack's visibility and participation in data calculations in the sim.
-  // Subclass handles reset.
+  // The subclass handles reset.
   public readonly isActiveProperty: Property<boolean>;
 
   // For the Distribute screen the positionProperty is set by the parentPlateProperty and the drag handler.
-  // Subclass handles reset.
+  // The subclass handles reset.
   public readonly positionProperty: Property<Vector2>;
 
   // An animation for moving this snack from one location to another in a continuous fashion.
@@ -139,7 +138,7 @@ export default class Snack extends PhetioObject {
 
   /**
    * If there is an in-progress animation, force it to finish immediately.  If there is no in-progress animation, this
-   * does nothing.  This is primarily intended to be used in conditions like a reset or a change in conditions where
+   * does nothing. This is primarily intended to be used in conditions like a reset or a change in conditions where
    * having a moving snack could be problematic.
    */
   public forceAnimationToFinish(): void {