From c37374f06fefc84a8a88583322a960e17f661d91 Mon Sep 17 00:00:00 2001 From: jbphet Date: Tue, 22 Dec 2020 17:15:06 -0700 Subject: [PATCH] added review comments, see #286 --- js/common/model/RAPModel.js | 27 +++++++++++++++++-- js/common/model/RAPRatio.js | 9 ++++++- .../view/BothHandsInteractionListener.js | 2 ++ js/common/view/BothHandsPDOMNode.js | 2 ++ js/common/view/RAPKeyboardHelpContent.js | 2 +- js/common/view/RAPScreenView.js | 4 +-- js/common/view/RatioHalf.js | 2 ++ js/common/view/RatioHandNode.js | 6 +++-- js/common/view/describers/RatioDescriber.js | 6 ++--- js/common/view/sound/BoundarySoundClip.js | 2 +- .../sound/StaccatoFrequencySoundGenerator.js | 2 +- js/common/view/sound/ViewSounds.js | 1 - js/create/view/CreateScreenSummaryNode.js | 2 ++ 13 files changed, 53 insertions(+), 14 deletions(-) diff --git a/js/common/model/RAPModel.js b/js/common/model/RAPModel.js index 7506e7fe..787e3e75 100644 --- a/js/common/model/RAPModel.js +++ b/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.} // 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
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.} // 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; diff --git a/js/common/model/RAPRatio.js b/js/common/model/RAPRatio.js index 96f3604c..afbe8e13 100644 --- a/js/common/model/RAPRatio.js +++ b/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.} 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.} - 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 ) { diff --git a/js/common/view/BothHandsInteractionListener.js b/js/common/view/BothHandsInteractionListener.js index cf197973..b987df9b 100644 --- a/js/common/view/BothHandsInteractionListener.js +++ b/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.} ratioTupleProperty diff --git a/js/common/view/BothHandsPDOMNode.js b/js/common/view/BothHandsPDOMNode.js index d2087802..71a00ac8 100644 --- a/js/common/view/BothHandsPDOMNode.js +++ b/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.} ratioTupleProperty * @param {Property.} enabledRatioTermsRangeProperty diff --git a/js/common/view/RAPKeyboardHelpContent.js b/js/common/view/RAPKeyboardHelpContent.js index de825792..2093e8cc 100644 --- a/js/common/view/RAPKeyboardHelpContent.js +++ b/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 diff --git a/js/common/view/RAPScreenView.js b/js/common/view/RAPScreenView.js index bb484d6b..a0057c58 100644 --- a/js/common/view/RAPScreenView.js +++ b/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(); diff --git a/js/common/view/RatioHalf.js b/js/common/view/RatioHalf.js index 51a0531d..6b2f1ae1 100644 --- a/js/common/view/RatioHalf.js +++ b/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.} enabledRatioTermsRangeProperty - the current range that the hand can move diff --git a/js/common/view/RatioHandNode.js b/js/common/view/RatioHandNode.js index ddf2e686..097fab6a 100644 --- a/js/common/view/RatioHandNode.js +++ b/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() { diff --git a/js/common/view/describers/RatioDescriber.js b/js/common/view/describers/RatioDescriber.js index 15a9a52c..740af15c 100644 --- a/js/common/view/describers/RatioDescriber.js +++ b/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 ) ) ]; } /** diff --git a/js/common/view/sound/BoundarySoundClip.js b/js/common/view/sound/BoundarySoundClip.js index cd329839..82ab3e29 100644 --- a/js/common/view/sound/BoundarySoundClip.js +++ b/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) diff --git a/js/common/view/sound/StaccatoFrequencySoundGenerator.js b/js/common/view/sound/StaccatoFrequencySoundGenerator.js index 109ba20c..98b2cb06 100644 --- a/js/common/view/sound/StaccatoFrequencySoundGenerator.js +++ b/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 * diff --git a/js/common/view/sound/ViewSounds.js b/js/common/view/sound/ViewSounds.js index 80396fef..0ee4aba4 100644 --- a/js/common/view/sound/ViewSounds.js +++ b/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.} tickMarkViewProperty * @param {BooleanProperty} playTickMarkBumpSoundProperty diff --git a/js/create/view/CreateScreenSummaryNode.js b/js/create/view/CreateScreenSummaryNode.js index 8ba70054..fee07e4a 100644 --- a/js/create/view/CreateScreenSummaryNode.js +++ b/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.} ratioFitnessProperty * @param {Property.} antecedentProperty