REVIEW comments, #32

js/common-vsm/model/VSMField.ts

@@ -81,6 +81,7 @@ export default class VSMField extends Field {
     super( launchers, launcherProperty, PDLUtils.colorForFieldIndex( VSMFieldIdentifierValues.indexOf( identifier ) ), options );
     this.continuousLaunchTimer = new PDLEventTimer( PDLConstants.MINIMUM_TIME_BETWEEN_LAUNCHES, options.tandem.createTandem( 'continuousLaunchTimer' ) );
 
+    //REVIEW Why is this not a NumberProperty?
     this.latestLaunchAngleProperty = new Property<number>( this.meanAngleProperty.value, {
       tandem: providedOptions.tandem.createTandem( 'latestLaunchAngleProperty' ),
       phetioReadOnly: true,
@@ -90,6 +91,7 @@ export default class VSMField extends Field {
       phetioFeatured: true
     } );
 
+    //REVIEW Why is this not a NumberProperty?
     this.latestLaunchSpeedProperty = new Property<number>( 0, {
       tandem: providedOptions.tandem.createTandem( 'latestLaunchSpeedProperty' ),
       phetioReadOnly: true,

js/common/model/Histogram.ts

@@ -93,6 +93,8 @@ export default class Histogram {
     shouldPlayMeanTone: () => boolean, // See the method declaration in PDLModel.ts
     playMeanTone: () => void,  // See the method declaration in PDLModel.ts
     providedOptions: HistogramOptions ) {
+
+    //REVIEW Why is this not a NumberProperty?
     this.selectedBinWidthProperty = new Property<number>( 1, {
       validValues: [ 0.5, 1, 2, 5, 10 ],
       tandem: providedOptions.tandem.createTandem( 'selectedBinWidthProperty' ),
@@ -101,6 +103,7 @@ export default class Histogram {
       phetioValueType: NumberIO
     } );
 
+    //REVIEW Why is this not a NumberProperty?
     this.selectedTotalBinsProperty = new Property<number>( 10, {
       validValues: [ 10, 20, 50, 100, 200 ],
       tandem: providedOptions.tandem.createTandem( 'selectedTotalBinsProperty' ),

js/common/model/Launcher.ts

@@ -42,6 +42,7 @@ export default class Launcher extends PhetioObject {
   public readonly meanLaunchSpeedProperty: DynamicProperty<number, number, LauncherMechanism>;
   public readonly standardDeviationSpeedProperty: TReadOnlyProperty<number>;
 
+  //REVIEW Is the doc for param launcherNumber correct here? For PDLColors.mysteryLauncherColorProfiles, custom is not index 0.
   public constructor( public readonly mysteryOrCustom: MysteryOrCustom, launcherMechanism: LauncherMechanism, standardDeviationAngle: number,
                       // 1-6 for the mystery launchers.
                       // 0 for custom

js/measures/model/IntervalTool.ts

@@ -19,6 +19,7 @@ import IOType from '../../../../tandem/js/types/IOType.js';
 import NumberIO from '../../../../tandem/js/types/NumberIO.js';
 import Property from '../../../../axon/js/Property.js';
 
+//REVIEW Vague names, constants should be uppercase, group all constants together.
 const max = 100;
 const min = 0;
 
@@ -30,6 +31,7 @@ const DEFAULT_EDGE_2 = 60;
 
 export default class IntervalTool extends PhetioObject {
 
+  //REVIEW document fields
   private _edge1: number;
   private _edge2: number;
   public readonly changedEmitter = new Emitter();
@@ -48,6 +50,7 @@ export default class IntervalTool extends PhetioObject {
     this._edge1 = DEFAULT_EDGE_1;
     this._edge2 = DEFAULT_EDGE_2;
 
+    //REVIEW Why is this not a NumberProperty?
     this.dataFractionProperty = new Property<number>( 0, {
       tandem: options.tandem.createTandem( 'dataFractionProperty' ),
       phetioFeatured: true,

js/measures/model/MeasuresField.ts

@@ -28,6 +28,8 @@ export type MeasuresFieldOptions = SelfOptions & StrictOmit<SMFieldOptions, 'isL
 
 export default class MeasuresField extends SMField {
 
+  //REVIEW For all Property<number | null>, what is the semantics of null?
+
   // This Property represents the average distance (horizontal displacement) of landed projectiles.
   public readonly meanDistanceProperty: Property<number | null>;
 

js/sampling/model/SamplingModel.ts

@@ -93,6 +93,7 @@ export default class SamplingModel extends PDLModel<SamplingField> {
       samplingLaunchModeProperty.value = launchMode;
     } );
 
+    //REVIEW Why is this not a NumberProperty?
     this.sampleSizeProperty = new Property<number>( 2, {
       validValues: SAMPLE_SIZES,
       tandem: options.tandem.createTandem( 'sampleSizeProperty' ),