added review comments, see #286

js/common/model/RAPModel.js

@@ -1,6 +1,6 @@
 // Copyright 2020, University of Colorado Boulder
 
-//REVIEW: This is a pretty central class in the sim and should probably have a description.
+// REVIEW: This is a pretty central class in the sim and should probably have a description.
 /**
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
@@ -29,12 +29,21 @@ class RAPModel {
     // @public - The desired ratio of the antecedent as compared to the consequent. As in 1:2 (initial value).
     this.targetRatioProperty = new NumberProperty( .5 );
 
+    // REVIEW: The target ratio property (above) and the ratio (below) rely on hard-coded constants to be correctly
+    // REVIEW: initialized.  Why not construct the ratio with values, then use its currentRatio to initialize targetRatioProperty?
+
     // @public - the current state of the ratio (value of terms, if its locked, etc)
     this.ratio = new RAPRatio();
 
+    // REVIEW - The rest of the code doesn't seem to ever change fitnessRange.  Can it be a constant and a static?
     // @public (read-only) - the Range that the ratioFitnessProperty can be.
     this.fitnessRange = new Range( 0, 1 );
 
+    // REVIEW: The comment below says, in part, "the min can be arbitrarily negative, depending how far away the current
+    // ratio is from the targetRatio".  The phrase "arbitrarily negative" seems problematic.  It can't really be
+    // arbitrary if it's the result of a calculation, which it almost certainly is.  What calculation?  How negative?
+    // How are clients expected to interpret these negative values?
+
     // @public {DerivedProperty.<number>}
     // How "correct" the proportion currently is. Max is this.fitnessRange.max, but the min can be arbitrarily negative,
     // depending how far away the current ratio is from the targetRatio
@@ -48,11 +57,13 @@ class RAPModel {
 
       let unclampedFitness = this.calculateFitness( antecedent, consequent, ratio );
 
-      // If either value is small enough, then we don't allow an "in proportion" fitness level, so make it just below that threshold.
+      // If either value is small enough, then we don't allow an "in proportion" fitness level, so make it just below
+      // that threshold.
       if ( this.inProportion( unclampedFitness ) && this.valuesTooSmallForInProportion() ) {
         unclampedFitness = this.fitnessRange.max - this.getInProportionThreshold() - .01;
       }
 
+      // REVIEW: This looks weird in terms of indentation.  Why not use \n or <br> and the + operator to keep it neater?
       phet.log && phet.log( `
 left: ${antecedent}, 
 right: ${consequent}, 
@@ -66,10 +77,15 @@ unclampedFitness: ${unclampedFitness}\n` );
       isValidValue: value => value <= this.fitnessRange.max
     } );
 
+    // REVIEW: Somewhere it should be explained why the clamped and unclamped values are both needed.
+
+    // REVIEW: Suggest more consistent names for fitness properties, e.g. clampedRatioFitnessProperty and unclampedRatioFitnessProperty
+
     // @public {DerivedProperty.<number>}
     // How "correct" the proportion currently is. clamped within this.fitnessRange. If at max (1), the proportion of
     // the two values is exactly the value of the targetRatioProperty. If min (0), it is outside the tolerance
     // allowed for the proportion to give many feedbacks.
+    // REVIEW: "...it is outside the tolerance allowed for the proportion to give many feedbacks." -> Can this be clarified and/or reworded?
     this.ratioFitnessProperty = new DerivedProperty( [ this.unclampedFitnessProperty ],
       unclampedFitness => Utils.clamp( unclampedFitness, this.fitnessRange.min, this.fitnessRange.max ), {
         isValidValue: value => this.fitnessRange.contains( value )
@@ -108,6 +124,8 @@ unclampedFitness: ${unclampedFitness}\n` );
     return this.fitnessRange.max - FITNESS_TOLERANCE_FACTOR * Math.abs( consequent - antecedentOptimal / targetRatio );
   }
 
+  // REVIEW: Suggest adding an explanation somewhere of how the fitness value works.
+
   /**
    * @param {number} antecedent
    * @param {number} consequent
@@ -117,6 +135,11 @@ unclampedFitness: ${unclampedFitness}\n` );
    */
   calculateFitness( antecedent, consequent, targetRatio ) {
 
+    // REVIEW: The comment below says, in part, "...because the model values only span from 0-1".  This is the first
+    // time this is mentioned that this reviewer (jbphet) has encountered.  Is this a constraint of the model?  Is it
+    // enforced anywhere?  Why is it like this?  Also, why is it necessary to multiply by any number?  A ratio is a
+    // ratio, so it seems kind of odd.
+
     // multiply because the model values only span from 0-1
     const a = antecedent * 10;
     const b = consequent * 10;

js/common/model/RAPRatio.js

@@ -17,7 +17,7 @@ import ratioAndProportion from '../../ratioAndProportion.js';
 import RAPConstants from '../RAPConstants.js';
 import RAPRatioTuple from './RAPRatioTuple.js';
 
-// The threshold for velocity of a moving ratio value to indivate that it is "moving."
+// The threshold for velocity of a moving ratio value to indicate that it is "moving."
 const VELOCITY_THRESHOLD = .01;
 
 const DEFAULT_TERM_VALUE_RANGE = RAPConstants.TOTAL_RATIO_TERM_VALUE_RANGE;
@@ -32,6 +32,8 @@ class RAPRatio {
     // @public (read-only) {Property.<Range>}
     this.enabledRatioTermsRangeProperty = new Property( DEFAULT_TERM_VALUE_RANGE );
 
+    // REVIEW: Why not construct this with initial values instead of using what seems like arbitrary ones below?
+
     // @public {Property.<RAPRatioTuple>} - central Property that holds the value of the ratio
     this.tupleProperty = new Property( new RAPRatioTuple( .2, .4 ), {
       valueType: RAPRatioTuple,
@@ -75,6 +77,11 @@ class RAPRatio {
     // RAPRatioTuple instance.
     this.lockRatioListenerEnabled = true;
 
+    // REVIEW: The linkage below seems quite complex, and appears to be a consequence of the design choice to use a
+    // tuple for the ratio instead of just independent antecedent and consequent properties.  And yet, there are derived
+    // versions of antecedent and consequent properties above anyway.  Wouldn't it be simpler to do away with the tuple
+    // and thus never be in a situation where it needed to be recreated because half of it had changed?
+
     // Listener that will handle keeping both ratio tuple values in sync when the ratio is locked.
     this.tupleProperty.link( ( tuple, oldTuple ) => {
       if ( this.lockedProperty.value && this.lockRatioListenerEnabled ) {

js/common/view/BothHandsInteractionListener.js

@@ -20,6 +20,8 @@ const TOTAL_RANGE = RAPConstants.TOTAL_RATIO_TERM_VALUE_RANGE;
 
 class BothHandsInteractionListener {
 
+  // REVIEW: This is a ton of parameters, and seems like a good application of a "config" object.
+
   /**
    * @param {Node} targetNode
    * @param {Property.<RAPRatioTuple>} ratioTupleProperty

js/common/view/BothHandsPDOMNode.js

@@ -22,6 +22,8 @@ import ViewSounds from './sound/ViewSounds.js';
 
 class BothHandsPDOMNode extends Node {
 
+  // REVIEW: This is a ton of parameters, and seems like a good application of a "config" object.
+
   /**
    * @param {Property.<RAPRatioTuple>} ratioTupleProperty
    * @param {Property.<Range>} enabledRatioTermsRangeProperty

js/common/view/RAPKeyboardHelpContent.js

@@ -45,7 +45,7 @@ class RAPKeyboardHelpContent extends TwoColumnKeyboardHelpContent {
   }
 }
 
-//REVIEW - was this intended to be a description of the class that didn't get finished?
+// REVIEW - was this intended to be a description of the class that didn't get finished?
 /**
  * @param {Object} [options]
  * @constructor

js/common/view/RAPScreenView.js

@@ -1,6 +1,6 @@
 // Copyright 2020, University of Colorado Boulder
 
-//REVIEW: Missing description.
+// REVIEW: Missing description.
 /**
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
@@ -109,7 +109,7 @@ class RAPScreenView extends ScreenView {
     soundManager.addSoundGenerator( this.inProportionSoundGenerator );
     soundManager.addSoundGenerator( this.movingInProportionSoundGenerator );
 
-    //REVIEW: Is the following comment incomplete?
+    // REVIEW: Is the following comment incomplete?
     // Properties that keep track of
     const cueArrowsState = new CueArrowsState();
 

js/common/view/RatioHalf.js

@@ -58,6 +58,8 @@ const TOTAL_RANGE = RAPConstants.TOTAL_RATIO_TERM_VALUE_RANGE;
 
 class RatioHalf extends Rectangle {
 
+  // REVIEW: Lots of parameters here, would a config object be better?
+
   /**
    * @param {NumberProperty} valueProperty
    * @param {Property.<Range>} enabledRatioTermsRangeProperty - the current range that the hand can move

js/common/view/RatioHandNode.js

@@ -44,7 +44,8 @@ class RatioHandNode extends Node {
 
     const shiftKeyboardStep = RAPConstants.toFixed( keyboardStep * RAPConstants.SHIFT_KEY_MULTIPLIER );
 
-    // conserve keypresses while allowing keyboard input to access any "in-proportion" state, even if more granular than the keyboard step size allows.
+    // Conserve keypresses while allowing keyboard input to access any "in-proportion" state, even if more granular than
+    // the keyboard step size allows.
     const mapKeyboardInput = getKeyboardInputSnappingMapper( getIdealValue, keyboardStep, shiftKeyboardStep );
 
     options = merge( {
@@ -166,7 +167,8 @@ class RatioHandNode extends Node {
   }
 
   /**
-   * Call to reset input characteristics for alternative input. See https://github.com/phetsims/ratio-and-proportion/issues/175#issuecomment-729292704
+   * Call to reset input characteristics for alternative input. See
+   * https://github.com/phetsims/ratio-and-proportion/issues/175#issuecomment-729292704
    * @public
    */
   reset() {

js/common/view/describers/RatioDescriber.js

@@ -1,6 +1,6 @@
 // Copyright 2020, University of Colorado Boulder
 
-//REVIEW: I (jbphet) don't know much about describers.  Would a description be useful, or is it obvious from the name what it does?
+// REVIEW: I (jbphet) don't know much about describers.  Would a description of what this is be useful, or is it obvious from the name what it does?
 /**
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
@@ -83,9 +83,9 @@ class RatioDescriber {
 
     const unclampedFitness = this.unclampedFitnessProperty.value;
 
-    const mappingFuntion = unclampedFitness > 0 ? greaterThanZeroMapping : lessThanZeroMapping;
+    const mappingFunction = unclampedFitness > 0 ? greaterThanZeroMapping : lessThanZeroMapping;
 
-    return ratioRegions[ Math.floor( mappingFuntion( unclampedFitness ) ) ];
+    return ratioRegions[ Math.floor( mappingFunction( unclampedFitness ) ) ];
   }
 
   /**

js/common/view/sound/BoundarySoundClip.js

@@ -4,7 +4,7 @@
  * A short sound to indicate when a movable component has reached the boundary of its movable bounds. This sound supports
  * playing a boundary sound based on horizontal motion, as well as vertical motion, but treats each as separate values,
  * and not as a Bounds2. This is to support some interactions (alternative input through keyboard) that only support vertical
- * movement (and thus only vertical boundary sounds). While terms are named and groked in x/y directions, this could be
+ * movement (and thus only vertical boundary sounds). While terms are named and grokked in x/y directions, this could be
  * used generally when you need a boundary sound to occur always on one axis, and optionally on another.
  *
  * @author Michael Kauzmann (PhET Interactive Simulations)

js/common/view/sound/StaccatoFrequencySoundGenerator.js

@@ -1,6 +1,6 @@
 // Copyright 2020, University of Colorado Boulder
 
-//REVIEW: The specifics of the description seems like it's out of date (they are no longer marimba bonks anymore (I think)).
+// REVIEW: The specifics of the description seems like it's out of date (they are no longer marimba bonks anymore (I think)).
 /**
  * "Marimba bonks" that change frequency based on the fitness of the Proportion
  *

js/common/view/sound/ViewSounds.js

@@ -25,7 +25,6 @@ const TOTAL_RANGE = RAPConstants.TOTAL_RATIO_TERM_VALUE_RANGE;
 class ViewSounds {
 
   /**
-   *
    * @param {NumberProperty} tickMarkRangeProperty
    * @param {Property.<TickMarkView>} tickMarkViewProperty
    * @param {BooleanProperty} playTickMarkBumpSoundProperty

js/create/view/CreateScreenSummaryNode.js

@@ -14,6 +14,8 @@ import ratioAndProportionStrings from '../../ratioAndProportionStrings.js';
 
 class CreateScreenSummaryNode extends Node {
 
+  // REVIEW: This is a ton of parameters, and seems like a good application of a "config" object.
+
   /**
    * @param {Property.<number>} ratioFitnessProperty
    * @param {Property.<number>} antecedentProperty