Self code review for distribute directory, see: #258

js/distribute/model/CandyBar.ts

@@ -2,7 +2,9 @@
 
 /**
  * Individual candy bars in the notepad snackType.
- * These candy bars are draggable therefore their position and parentPlate are important.
+ * These candy bars are draggable.
+ *
+ * TODO: JB, do we just need this for clarity? It doesn't seem to add any functionality. https://github.com/phetsims/mean-share-and-balance/issues/258
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)

js/distribute/model/DistributeModel.ts

@@ -1,8 +1,7 @@
 // Copyright 2022-2024, University of Colorado Boulder
 
 /**
- * Model for the Distribute Screen which includes people, candy bars, visual mean snackType, and a numerical
- * mean snackType.
+ * Model for the Distribute Screen which includes people, candy bars, and mean prediction.
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
@@ -52,8 +51,6 @@ export default class DistributeModel extends SharingModel<CandyBar> {
   public readonly meanPredictionProperty: Property<number>;
   public readonly predictMeanDragRange = new Range( 0, MeanShareAndBalanceConstants.MAX_NUMBER_OF_SNACKS_PER_PLATE );
 
-  public readonly meanWithRemainderProperty: TReadOnlyProperty<MeanWithRemainder>;
-
   // Tracks whether the snacks are distributed evenly across the plates or at least distributed as much as is possible
   // with the data provided.
   public readonly snacksDistributedProperty: TReadOnlyProperty<boolean>;
@@ -66,6 +63,11 @@ export default class DistributeModel extends SharingModel<CandyBar> {
   // phet-io specific Properties
   public readonly successIndicatorsOperatingProperty: Property<boolean>;
 
+  // TODO: This is does not need to be a class member, but I'm not sure how to get around linting errors since
+    // it is needed for PhET-iO studio. https://github.com/phetsims/mean-share-and-balance/issues/258
+  public readonly meanWithRemainderProperty: TReadOnlyProperty<MeanWithRemainder>;
+
+
   public constructor( providedOptions?: DistributeModelOptions ) {
 
     const createCandyBar = ( options: SnackOptions ) => new CandyBar( options );
@@ -94,7 +96,6 @@ export default class DistributeModel extends SharingModel<CandyBar> {
     this.predictMeanVisibleProperty = new BooleanProperty( false, {
       tandem: options.tandem.createTandem( 'predictMeanVisibleProperty' )
     } );
-
     this.meanPredictionProperty = new NumberProperty( 0, {
       range: this.predictMeanDragRange,
       tandem: options.tandem.createTandem( 'meanPredictionProperty' )
@@ -124,7 +125,6 @@ export default class DistributeModel extends SharingModel<CandyBar> {
 
     // Initialize the plates and set up plate-related behavior that is specific to the Distribute screen.
     this.plates.forEach( plate => {
-
       plate.snacksOnNotepadPlate.addItemAddedListener( snack => {
         snack.isActiveProperty.value = true;
         const index = plate.snacksOnNotepadPlate.indexOf( snack );
@@ -165,9 +165,7 @@ export default class DistributeModel extends SharingModel<CandyBar> {
 
           // Remove notepad snacks from this plate, or from another if this plate is empty.
           _.times( Math.abs( delta ), () => {
-
-            // REVIEW: We should probably be checking the min number of snacks on a plate, not hard coding 0.
-            if ( plate.getNumberOfNotepadSnacks() > 0 ) {
+            if ( plate.getNumberOfNotepadSnacks() > MeanShareAndBalanceConstants.MIN_NUMBER_OF_SNACKS_PER_PLATE ) {
               plate.removeTopSnack();
             }
             else {
@@ -177,19 +175,16 @@ export default class DistributeModel extends SharingModel<CandyBar> {
             }
           } );
         }
-
         this.stackChangedEmitter.emit();
       } );
 
       // Monitor the isActiveProperty for each plate and do any redistribution of candy bars that is necessary when
       // changes occur.
       plate.isActiveProperty.link( isActive => {
-
         if ( !isActive ) {
 
           // Handle the situation where this plate went inactive and had excess candy bars on it.  The inverse
           // situation, i.e. when goes inactive with a deficit, is handled elsewhere.
-          // REVIEW QUESTION: Where is that handled?
           const excess = Math.max( plate.getNumberOfNotepadSnacks() - plate.tableSnackNumberProperty.value, 0 );
           if ( excess > 0 ) {
 
@@ -198,7 +193,9 @@ export default class DistributeModel extends SharingModel<CandyBar> {
               plate.removeTopSnack();
               const plateWithFewestSnacks = this.getPlateWithFewestSnacks();
 
-              // REVIEW: Should we confirm that there are indeed plates with space before adding a snack?
+              assert && assert( plateWithFewestSnacks &&
+              plateWithFewestSnacks.snacksOnNotepadPlate.length < MeanShareAndBalanceConstants.MAX_NUMBER_OF_SNACKS_PER_PLATE,
+              'A plate should have space to add another snack.' );
               plateWithFewestSnacks!.addASnack();
             } );
           }
@@ -210,6 +207,8 @@ export default class DistributeModel extends SharingModel<CandyBar> {
       } );
     } );
 
+    // Track whether the snacks are distributed evenly across the plates or at least distributed as much as is possible
+    // to trigger success indicators for the predict mean tool.
     const activePlateDependencies = this.plates.map( plate => plate.isActiveProperty );
     const notepadPlateSnackCountDependencies = this.plates.map( plate => plate.snacksOnNotepadPlate.lengthProperty );
     this.snacksDistributedProperty = DerivedProperty.deriveAny( [
@@ -227,6 +226,7 @@ export default class DistributeModel extends SharingModel<CandyBar> {
         phetioValueType: BooleanIO
       } );
 
+    // For phet-io client use only.
     this.meanWithRemainderProperty = new DerivedProperty( [
       this.numberOfPlatesProperty,
       this.totalSnacksProperty
@@ -241,7 +241,7 @@ export default class DistributeModel extends SharingModel<CandyBar> {
       phetioValueType: ObjectLiteralIO
     } );
 
-    // For phet-io client use only.
+
     this.successIndicatorsOperatingProperty = new BooleanProperty( true, {
       tandem: options.tandem.createTandem( 'successIndicatorsOperatingProperty' )
     } );

js/distribute/view/DistributeScreenView.ts

@@ -2,7 +2,7 @@
 
 /**
  * Representation for the Distribute Screen. Contains a table with people, each of whom have a plate with candy bars
- * on them.  It also includes a notepad that also show plates and candy bars that can be dragged and 'leveled out'.
+ * on them. It also includes a notepad that also show plates and candy bars that can be dragged and 'leveled out'.
  *
  * @author Marla Schulz (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
@@ -142,6 +142,7 @@ export default class DistributeScreenView extends SharingScreenView<CandyBar> {
     model.stackChangedEmitter.addListener( this.updateMouseSortCueNode.bind( this ) );
     model.stackChangedEmitter.addListener( this.updateKeyboardSortCueNode.bind( this ) );
 
+    // Create the notepad plates and candy bars.
     const notepadPlateNodes = model.plates.map( plate => {
       plate.xPositionProperty.link( this.updateMouseSortCueNode.bind( this ) );
       plate.xPositionProperty.link( this.updateKeyboardSortCueNode.bind( this ) );
@@ -163,15 +164,15 @@ export default class DistributeScreenView extends SharingScreenView<CandyBar> {
         }
       )
     );
-    const notepadCandyBarsNode = new InteractiveHighlightingNode( {
+    const notepadCandyBarsHighlightNode = new InteractiveHighlightingNode( {
       focusable: true,
       tagName: 'div',
       children: notepadCandyBarNodes,
       excludeInvisibleChildrenFromBounds: true
     } );
 
     this.notepadSnackLayerNode.addChild( this.cueingHighlight );
-    this.notepadSnackLayerNode.addChild( notepadCandyBarsNode );
+    this.notepadSnackLayerNode.addChild( notepadCandyBarsHighlightNode );
     this.notepadSnackLayerNode.addChild( mouseSortCueNode );
     this.notepadSnackLayerNode.addChild( this.keyboardSortCueNode );
 
@@ -258,12 +259,11 @@ export default class DistributeScreenView extends SharingScreenView<CandyBar> {
         lastPlate.xPositionProperty.value + MeanShareAndBalanceConstants.NOTEPAD_PLATE_DIMENSION.width / 2
       );
     } );
-
     this.notepadSnackLayerNode.addChild( meanPredictionSlider );
 
     this.groupSortInteractionView = new GroupSortInteractionView(
       model.groupSortInteractionModel,
-      notepadCandyBarsNode,
+      notepadCandyBarsHighlightNode,
       {
         getNextSelectedGroupItem: ( delta, candyBar ) => {
           const platesWithSnacks = model.getPlatesWithSnacks();
@@ -331,7 +331,9 @@ export default class DistributeScreenView extends SharingScreenView<CandyBar> {
     } );
   }
 
-  // Handle a candy bar being dropped in the notepad.
+  /**
+   * Handle a candy bar being dropped in the notepad by mouse or touch.
+   */
   private candyBarDropped( candyBarNode: NotepadCandyBarNode ): void {
     const candyBar = candyBarNode.candyBar;
 
@@ -416,6 +418,9 @@ export default class DistributeScreenView extends SharingScreenView<CandyBar> {
     }
   }
 
+  /**
+   * Update the visibility and position of the keyboard sort cue node based on the model's state.
+   */
   private updateKeyboardSortCueNode(): void {
     const selectedCandyBar = this.groupSortInteractionModel.selectedGroupItemProperty.value;
     if ( !this.groupSortInteractionModel.hasGroupItemBeenSortedProperty.value && selectedCandyBar ) {

js/distribute/view/NotepadCandyBarNode.ts

@@ -129,10 +129,10 @@ export default class NotepadCandyBarNode extends InteractiveHighlighting( Node )
   /**
    * A pattern is used for the outline of the candy bar. Because of this, the pattern must be rotated and translated to
    * match the bounds of the rectangle. This method returns the nodes that make up the outline of the candy bar.
-   *
-   * When creating partial candy bars the width and rightYTranslation may need to adjust accordingly.
    */
-  public static getSketchOutline( candyBarWidth = MeanShareAndBalanceConstants.CANDY_BAR_WIDTH, rightYTranslation = 0.975 ): Node[ ] {
+  public static getSketchOutline( candyBarWidth = MeanShareAndBalanceConstants.CANDY_BAR_WIDTH ): Node[ ] {
+
+    const rightYTranslation = 0.975;
     const horizontalStrokePattern = new Pattern( graphiteTexture_png ).setTransformMatrix(
       Matrix3.affine( 0.15, 0, 0, 0, 0.15, 0.9 )
     );