Add code review comments, see: #109

js/coins/model/CoinSet.ts

@@ -290,6 +290,8 @@ class CoinSet extends PhetioObject {
     // New values are generated by creating a new random seed and setting the value of a Property.  A listener for this
     // Property does the actual generation of the values.  Note that the value of 0 is not allowed for the random seed
     // because it has special significance - it means all the values should be set to 0.
+    // REVIEW: Don't you have a similar situation if the seed === 1? In your seedProperty.link a seed value of 1
+    // also sets all values to be element 1 of the validValues array.
     let newSeed;
     do {
       newSeed = dotRandom.nextDouble();

js/coins/view/CoinExperimentMeasurementArea.ts

@@ -137,6 +137,8 @@ class CoinExperimentMeasurementArea extends VBox {
     } );
 
     const createRadioButtonGroupItem = ( value: number ) => {
+
+      // REVIEW: You can pass a number directly to Text. It will handle that conversion there.
       const valueText = value.toString();
       return {
         createNode: () => new Text( valueText, { font: RADIO_BUTTON_FONT } ),
@@ -200,6 +202,9 @@ class CoinExperimentMeasurementArea extends VBox {
     this.singleCoinInTestBoxProperty = singleCoinInTestBoxProperty;
     this.coinSetInTestBoxProperty = coinSetInTestBoxProperty;
 
+    // REVIEW: Hmmm I added a review comment elsewhere about the word "mask" and replacing it with "hidden" since
+    // there hadn't been much mention of "mask" when I got there... I'm a little less certain now but it still felt
+    // confusing when added to that type.
     // Create the node that will be used to cover (aka "mask") the coin so that its state can't be seen.
     const maskRadius = InitialCoinStateSelectorNode.INDICATOR_COIN_NODE_RADIUS * 1.02;
     const coinMask = new Circle( maskRadius, {

js/coins/view/CoinExperimentPreparationArea.ts

@@ -155,7 +155,7 @@ class CoinExperimentPreparationArea extends VBox {
 
   /**
    * Get the position of the coin that indicates the initial orientation or prepared state in the global coordinate
-   * frame.  This is generally used when trying to animate the movement of coin between parent nodes.
+   * frame. This is generally used when trying to animate the movement of coin between parent nodes.
    */
   public getIndicatorCoinGlobalCenter(): Vector2 {
     const orientationIndicatorGlobalBounds = this.initialCoinStateSelectorNode.orientationIndicatorCoinNode.getGlobalBounds();

js/coins/view/CoinMeasurementHistogram.ts

@@ -102,6 +102,7 @@ export default class CoinMeasurementHistogram extends QuantumMeasurementHistogra
       providedOptions
     );
 
+    // REVIEW: This is another example where you should probably use PatternStringProperty instead of StringUtils.fillIn
     const numberOfCoinsStringProperty = new DerivedStringProperty(
       [ coinSet.numberOfActiveCoinsProperty ],
       numberOfCoins => StringUtils.fillIn(

js/coins/view/MaxCoinsViewManager.ts

@@ -80,8 +80,9 @@ class MaxCoinsViewManager {
       sceneGraphParent.addManyCoinsNode( pixelRepresentation );
 
       // The tricky bit about this animation is that the test box where these coins are headed could itself be moving
-      // due to the way the measurement area works.  This unfortunately means we need to have a bit of the "tweak
+      // due to the way the measurement area works. This unfortunately means we need to have a bit of the "tweak
       // factor" to get the destination right.
+      // REVIEW: -92 is the same "tweak" number that is used in MultipleCoinsViewManager. Coincidence or worth abstracting?
       const testAreaXOffset = forReprepare ? 0 : -92; // empirically determined
       const multipleCoinTestBoxBounds = sceneGraphParent.globalToLocalBounds( multipleCoinTestBox.getGlobalBounds() );
       const destinationCenter = multipleCoinTestBoxBounds.center.plusXY( testAreaXOffset, 0 );

js/coins/view/MultiCoinTestBox.ts

@@ -87,7 +87,7 @@ class MultiCoinTestBox extends Node {
     coinSet.measuredDataChangedEmitter.addListener( () => {
 
       // When phet-io state is being set, the measured data can change without any change to the measurement state of
-      // the coin set.  This makes sure that the coin nodes are updated in that situation.
+      // the coin set. This makes sure that the coin nodes are updated in that situation.
       if ( isSettingPhetioStateProperty.value ) {
         this.updateCoinNodes( coinSet, coinSet.measurementStateProperty.value );
       }

js/coins/view/MultipleCoinsViewManager.ts

@@ -41,8 +41,8 @@ class MultipleCoinsViewManager {
     // map of the size of the coin set to the nodes used for that set
     const coinNodeSets = new Map<number, SmallCoinNode[]>();
 
-    // Set up the coin nodes for each of the quantities that can be selected by the user.  As of Jan 2025, this includes
-    // 10 and 100 coins.  The 10k case is handled differently and is not included here.
+    // Set up the coin nodes for each of the quantities that can be selected by the user. As of Jan 2025, this includes
+    // 10 and 100 coins. The 10k case is handled differently and is not included here.
     MULTI_COIN_ANIMATION_QUANTITIES.forEach( quantity => {
       const radius = MultiCoinTestBox.getRadiusFromCoinQuantity( quantity );
       const coinNodes: SmallCoinNode[] = [];
@@ -99,14 +99,14 @@ class MultipleCoinsViewManager {
       const coinsToAnimate = coinNodeSets.get( sceneModel.coinSet.numberOfActiveCoinsProperty.value );
 
       // Add the coins to our parent node. This is done so that we don't change the local bounds of the measurement
-      // area, since this would break the layout.  These will be added back to the measurement area when they reach the
-      // desired position and are thus withing the bounds of the measurement area.
+      // area, since this would break the layout. These will be added back to the measurement area when they reach the
+      // desired position and are thus within the bounds of the measurement area.
       sceneGraphParent.addCoinNodeSet( coinsToAnimate! );
 
       const multipleCoinTestBoxBounds = sceneGraphParent.globalToLocalBounds( multipleCoinTestBox.getGlobalBounds() );
 
       // The tricky bit about this animation is that the test box where these coins are headed could itself be moving
-      // due to the way the measurement area works.  This unfortunately means we need to have a bit of the "tweak
+      // due to the way the measurement area works. This unfortunately means we need to have a bit of the "tweak
       // factor" to get the destination right.
       const testAreaXOffset = forReprepare ? 0 : -92; // empirically determined
       const destinationCenter = multipleCoinTestBoxBounds.center.plusXY( testAreaXOffset, 0 );
@@ -124,6 +124,8 @@ class MultipleCoinsViewManager {
         // Get the final destination for this coin node in terms of its offset from the center of the test box.
         const finalDestinationOffset = multipleCoinTestBox.getOffsetFromCenter( index );
 
+        // REVIEW: Is this a typo? This seems like it's the 1st portion of the animation, or at least I don't see another animation
+        // defined before this.
         // Start the 2nd portion of the animation, which moves the coin into the test box.
         const animationToTestBox = new Animation( {
           setValue: value => { coinNode.center = value; },
@@ -164,6 +166,7 @@ class MultipleCoinsViewManager {
           );
         } );
 
+        // REVIEW: Yeah I'm confused maybe it's late and I'm tired, but I can't find the first animation...
         // Kick off the 2nd animation, which moves the coin from the edge of the test box to inside.
         animationToTestBox.start();
       } );

js/coins/view/OutcomeProbabilityControl.ts

@@ -34,6 +34,7 @@ import ProbabilityValueControl from './ProbabilityValueControl.js';
 type SelfOptions = EmptySelfOptions;
 type OutcomeProbabilityControlOptions = SelfOptions & PickRequired<VBox, 'tandem' | 'visibleProperty'>;
 
+// REVIEW: Not sure what it means to not "rely on systemType for the color"
 // constants that don't rely on systemType for the color
 const TITLE_AND_LABEL_FONT = new PhetFont( 16 );
 const ALPHA = MathSymbols.ALPHA;
@@ -204,6 +205,8 @@ export default class OutcomeProbabilityControl extends VBox {
           const alphaValue = Utils.toFixed( Math.sqrt( outcomeProbability ), 3 );
           const betaValue = Utils.toFixed( Math.sqrt( 1 - outcomeProbability ), 3 );
 
+          // REVIEW: This StringUtils.fillIn feels particularly alarming. This feels like a classic case where
+          // PatternStringProperty will do what you need. If not, very clearly document.
           equationAccessibleParagraphStringProperty.value = StringUtils.fillIn( equationPattern, {
             alpha: alphaValue,
             beta: betaValue

js/coins/view/ProbabilityValueControl.ts

@@ -39,6 +39,8 @@ export default class ProbabilityValueControl extends NumberControl {
                       ) {
 
     super( titleStringProperty, probabilityProperty, RANGE, optionize<ProbabilityValueControlOptions, SelfOptions, NumberControlOptions>()( {
+
+      // REVIEW: Add documentation describing why you need to create provide your own layoutFunction.
       layoutFunction: ( titleNode, numberDisplay, slider, leftArrowButton, rightArrowButton ) => {
         assert && assert( leftArrowButton && rightArrowButton );
         return new VBox( {

js/coins/view/SceneSectionHeader.ts

@@ -1,6 +1,8 @@
 // Copyright 2024-2025, University of Colorado Boulder
 
 /**
+ * REVIEW: The code doesn't indicate that the line is wider than the text. The code indicates that it is the same width as the text.
+ *
  * SceneSectionHeader is a composite Scenery Node that consists of a textual header and a line below it, where the line
  * is generally wider than text. It looks something like this:
  *

js/coins/view/SingleCoinViewManager.ts

@@ -1,8 +1,8 @@
 // Copyright 2024-2025, University of Colorado Boulder
 
 /**
- * SingleCoinViewManager is responsible for creating an animating the motion of the single coin that is used in the
- * experiment.  This includes the animation of the coin moving from the preparation area to the measurement area, as
+ * SingleCoinViewManager is responsible for creating and animating the motion of the single coin that is used in the
+ * experiment. This includes the animation of the coin moving from the preparation area to the measurement area, as
  * well as providing support for the coin flipping animation.
  *
  * @author John Blanco, PhET Interactive Simulations
@@ -78,6 +78,7 @@ class SingleCoinViewManager {
     this.abortIngressAnimationForSingleCoin = () => {
 
       // Create a typed reference to the parent node, since we'll need to invoke some methods on it.
+      // REVIEW: Might be helpful to add a specific message for this assertion.
       assert && assert( measurementArea.getParent() instanceof CoinsExperimentSceneView );
       const sceneGraphParent = measurementArea.getParent() as CoinsExperimentSceneView;
 
@@ -155,7 +156,7 @@ class SingleCoinViewManager {
 
         assert && assert( singleCoinNode, 'There should be a singleCoinNode instance at the end of this animation.' );
 
-        // Get a reference to the coin Node that allows the code to omit all the exclamation points and such.
+        // Get a reference to the coin Node that allows the code to omit all the non-null assertions.
         const assuredSingleCoinNode = singleCoinNode!;
         assuredSingleCoinNode.moveToBack();
 

js/coins/view/SmallCoinNode.ts

@@ -30,6 +30,8 @@ import { QuantumCoinStates } from '../model/QuantumCoinStates.js';
 type SelfOptions = EmptySelfOptions;
 export type SmallCoinNodeOptions = SelfOptions & WithRequired<NodeOptions, 'tandem'>;
 
+// REVIEW: What does it mean for a coin to be "masked"? Oh... I think I get it. It's masked when it's hidden? Maybe
+// just switch that to "hidden" since that's terminology you're already using elsewhere.
 // Define a type for the display mode that composes all possible coin values plus one for when the coin is masked.
 export type SmallCoinDisplayMode = ClassicalCoinStates | QuantumCoinStates | 'masked';
 
@@ -122,6 +124,9 @@ class SmallCoinNode extends Node {
 
   public get isFlipping(): boolean { return this.flippingAnimation !== null; }
 
+  // REVIEW: I would recommend documenting this a bit more especially to describe the flipPhase, previousXScale,
+  //  destinationPhaseMultiplier, and rotationalAxis. That will help future maintainers from having to re-read multiple
+  // times or visit the sim to understand what is going on.
   public startFlipping(): void {
     let flipPhase = 0;
     let previousXScale = 0;