first pass through coding convention in model, #259

Signed-off-by: Chris Malley <[email protected]>

js/common/model/IntensitySample.js

@@ -56,6 +56,8 @@ define( require => {
 
       const averagedIntensities = [];
       averagedIntensities.length = intensities.length;
+
+      //REVIEW explain why it's OK that the first and last values in averagedIntensities are not averages
       averagedIntensities[ 0 ] = intensities[ 0 ];
       averagedIntensities[ averagedIntensities.length - 1 ] = intensities[ averagedIntensities.length - 1 ];
 

js/common/model/Lattice.js

@@ -68,7 +68,8 @@ define( require => {
       this.height = height;
 
       // @public {number} - Determines how far we have animated between the "last" and "current" matrices, so that we
-      // can use getInterpolatedValue to update the view at 60fps even though the model is running at a slower rate
+      // can use getInterpolatedValue to update the view at 60fps even though the model is running at a slower rate.
+      // See EventTimer.getRatio for more about this value.
       this.interpolationRatio = 0;
 
       // @public (read-only) {Bounds2} - a Bounds2 representing the visible (non-damping) region of the lattice.
@@ -83,6 +84,7 @@ define( require => {
       return new Bounds2( 0, 0, this.width, this.height );
     }
 
+    //REVIEW missing visibility annotation
     /**
      * Returns true if the visible bounds contains the lattice coordinate
      * @param {number} i - integer for the horizontal coordinate
@@ -275,7 +277,7 @@ define( require => {
       // Numerical computation of absorbing boundary conditions, under the assumption that the wave is perpendicular
       // to the edge, see https://www.phy.ornl.gov/csep/sw/node22.html.  This assumption does not hold everywhere, but
       // it is a helpful approximation.
-      // Note there is a fortran error on the top boundary and in the equations, replace:
+      // Note there is a Fortran error on the top boundary and in the equations, replace:
       // u2 => matrix1.get
       // u1 => matrix2.get
       // cb => WAVE_SPEED

js/common/model/LightScene.js

@@ -21,6 +21,7 @@ define( require => {
     constructor( config ) {
       super( config );
 
+      //REVIEW (read-only) ?
       // @public {IntensitySample} reads out the intensity on the right hand side of the lattice
       this.intensitySample = new IntensitySample( this.lattice );
     }

js/common/model/Scene.js

@@ -121,6 +121,7 @@ define( require => {
         units: this.timeUnits
       } );
 
+      //REVIEW add value validation?
       // @public {Property.<Number>} - distance between the sources in the units of the scene, or 0 if there is only one
       // source initialized to match the initial slit separation,
       // see https://github.com/phetsims/wave-interference/issues/87
@@ -325,6 +326,7 @@ define( require => {
       }
     }
 
+    //REVIEW @override seems incorrect, this class has no super
     /**
      * Set the incoming source values, in this case it is a point source near the left side of the lattice (outside of
      * the damping region).
@@ -524,6 +526,7 @@ define( require => {
       this.phase = -this.timeProperty.value * angularFrequency;
     }
 
+    //REVIEW should the PhET convention for ES5 getters be used here? That is, define and call getWavelength.
     /**
      * Returns the wavelength in the units of the scene
      * @returns {number}
@@ -533,6 +536,7 @@ define( require => {
       return this.waveSpeed / this.frequencyProperty.get();
     }
 
+    //REVIEW Why are you returning a Rectangle instead of a Bounds2 here? Is something depending on Rectangle features?
     /**
      * Returns a Bounds2 for the visible part of the wave area, in the coordinates of the scene.
      * @returns {Bounds2} the lattice model bounds, in the coordinates of this scene.
@@ -542,6 +546,7 @@ define( require => {
       return new Rectangle( 0, 0, this.waveAreaWidth, this.waveAreaWidth );
     }
 
+    //REVIEW missing doc and visibility annotation
     startPulse() {
       assert && assert( !this.pulseFiringProperty.value, 'Cannot fire a pulse while a pulse is already being fired' );
       this.resetPhase();
@@ -572,6 +577,7 @@ define( require => {
       this.barrierTypeProperty && this.barrierTypeProperty.reset();
     }
 
+    //REVIEW missing visibility annotation
     /**
      * Move forward in time by the specified amount
      * @param {number} dt - amount of time to move forward, in the units of the scene

js/common/model/SceneType.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW this enum is redundant, use WaterScene, SoundScene, and LightScene constructors
 /**
  * Enum that identifies the type of Scene
  *

js/common/model/SoundParticle.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW I don't understand the "When selected" bit.
 /**
  * When selected, shows discrete, moving particles for the sound scene.
  *
@@ -13,7 +14,9 @@ define( require => {
   const waveInterference = require( 'WAVE_INTERFERENCE/waveInterference' );
 
   // constants
+  //REVIEW RANDOMNESS value needs more explanation - range, effect of increase/decrease,...
   const RANDOMNESS = 14.75; // The random motion of the particles
+  //REVIEW FRICTION_SCALE needs more explanation, since 1 = no friction is not what I would have expected
   const FRICTION_SCALE = 0.732; // Scaling factor for friction (1.0 = no friction)
   const RESTORATION_FORCE_SCALE = 0.5; // Additional scaling for the home force
 
@@ -27,6 +30,7 @@ define( require => {
      */
     constructor( i, j, x, y ) {
 
+      //REVIEW I don't see assignments to i,j,x,y anywhere.  Should these be read-only?
       // @public {number} - horizontal lattice coordinate of the particle
       this.i = i;
 
@@ -46,6 +50,7 @@ define( require => {
       this.vy = 0;
     }
 
+    //REVIEW missing visibility annotation
     /**
      * Applies a force toward the given point with the given strength;
      * @param {number} fx - sum of applied forces in the x direction

js/common/model/SoundScene.js

@@ -35,6 +35,7 @@ define( require => {
       // on the Slits screen, see https://github.com/phetsims/wave-interference/issues/109
       this.showSoundParticles = showSoundParticles;
 
+      //REVIEW {Property.<SoundViewType>}?
       // @public {Property.<string>} - indicates the selected view for sound
       this.viewSelectionProperty = new Property( SoundViewType.WAVES, {
         validValues: SoundViewType.VALUES
@@ -60,9 +61,11 @@ define( require => {
       }
     }
 
+    //REVIEW missing visibility annotation
     /**
      * Move forward in time by the specified amount, updating velocity and position of the SoundParticle instances
      * @param {number} dt - amount of time to move forward, in the units of the scene
+     * @override
      */
     step( dt ) {
 

js/common/model/SoundViewType.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW I see no "SourceScene".  Do you mean "SoundScene"?
 /**
  * For the SourceScene, selects whether to display waves, particles or both.
  *

js/common/model/WaterDrop.js

@@ -42,6 +42,7 @@ define( require => {
       // should no longer be visible.  But the modeled time that it affects the lattice is the same.
       this.absorbed = false;
 
+      //REVIEW read-only?
       // @public {function} - called when absorbed
       this.onAbsorption = onAbsorption;
 
@@ -53,6 +54,7 @@ define( require => {
       this.sign = sign;
     }
 
+    //REVIEW missing doc and visibility annotation
     step( dt ) {
 
       this.y -= dt * WATER_DROP_SPEED;

js/common/model/WaterScene.js

@@ -25,20 +25,24 @@ define( require => {
     constructor( config ) {
       super( config );
 
+      //REVIEW add value validation? range: this.frequencyProperty.range ?
       // @public - In the water Scene, the user specifies the desired frequency and amplitude, and that
       // gets propagated to the lattice via the water drops
       this.desiredFrequencyProperty = new NumberProperty( this.frequencyProperty.initialValue );
 
+      //REVIEW add value valdation?
       // @public - In the water Scene, the user specifies the desired source separation.  This is the position of
       // the faucets.  The sourceSeparationProperty indicates the sources of oscillation once the water has struck.
       this.desiredSourceSeparationProperty = new NumberProperty( this.sourceSeparationProperty.value );
 
+      //REVIEW add value validation?
       // @public - the amplitude the user has selected
       this.desiredAmplitudeProperty = new NumberProperty( config.initialAmplitude );
 
       // @public (read-only) {WaterDrop[]} drops of water that are falling from the hose to the lattice.
       this.waterDrops = [];
 
+      //REVIEW what is "the phase"? units, range and semantics of values,...
       // @private {number|null} - sets the phase for both drops.  Null means no drops have been emitted.
       this.lastDropTime = null;
 
@@ -89,7 +93,7 @@ define( require => {
           amplitude,
           buttonPressed,
           sourceSeparation,
-          100,
+          100, //REVIEW magic number? param is y - distance to fall before the particles meets the plane of the lattice
           sign,
           () => {
 
@@ -114,9 +118,11 @@ define( require => {
       }
     }
 
+    //REVIEW missing visibility annotation
     /**
      * Move forward in time by the specified amount, updating velocity and position of the SoundParticle instances
      * @param {number} dt - amount of time to move forward, in the units of the scene
+     * @override
      */
     step( dt ) {
 
@@ -204,6 +210,5 @@ define( require => {
     }
   }
 
-
   return waveInterference.register( 'WaterScene', WaterScene );
 } );

js/common/model/WaveSpatialType.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW A nit about doc for most (all?) enums. You say "Determines whether..." but an enum determines nothing, it is a set of values with no responsibilities. A {Property.<WaveSpatialType>} would "determine".
 /**
  * Determines whether the wave is a point source or plane wave.
  *

js/diffraction/model/DiffractionModel.js

@@ -1,7 +1,7 @@
 // Copyright 2017-2018, University of Colorado Boulder
 
 /**
- * The Diffraction model is implemented in DiffractionScreenView
+ * Model for the Diffraction screen.
  *
  * @author Sam Reid (PhET Interactive Simulations)
  */
@@ -21,19 +21,23 @@ define( require => {
       // @public {Property.<boolean>} whether the laser is emitting light
       this.onProperty = new BooleanProperty( true );
 
+      //REVIEW add value validation. These presumably have ranges and must be > 0.
       // @public {Property.<number>} dimensions of the square aperture
       this.squareWidthProperty = new NumberProperty( 16 );
       this.squareHeightProperty = new NumberProperty( 16 );
 
+      //REVIEW add value validation
       // @public {Property.<number>} dimensions of the elliptical aperture
       this.sigmaXProperty = new NumberProperty( 10 );
       this.sigmaYProperty = new NumberProperty( 10 );
       this.gaussianMagnitudeProperty = new NumberProperty( 400 );
 
+      //REVIEW add value validation
       // @public {Property.<number>} characteristics of the grating
       this.numberOfLinesProperty = new NumberProperty( 10 );
       this.angleProperty = new NumberProperty( 0 );
 
+      //REVIEW {Property.<ApertureType>}
       // @public {Property.<string>} selected scene
       this.sceneProperty = new Property( ApertureType.CIRCLE );
     }

js/interference/model/InterferenceScreenModel.js

@@ -16,6 +16,8 @@ define( require => {
     constructor() {
       super( {
         numberOfSources: 2,
+
+        //REVIEW There's an extended comment about the default value for initialAmplitude over in WaveScreenModel, and it's not 10. Why did you choose this value here?
         initialAmplitude: 10
       } );
     }

js/slits/model/SlitsScreenModel.js

@@ -17,6 +17,8 @@ define( require => {
 
     constructor() {
       super( {
+
+        //REVIEW There's an extended comment about the default value for initialAmplitude over in WaveScreenModel, and it's not 10. Why did you choose this value here?
         initialAmplitude: 10,
         waveSpatialType: WaveSpatialType.PLANE,
 

js/waves/model/WavesScreenModel.js

@@ -53,7 +53,7 @@ define( require => {
   const waterWaveGeneratorString = require( 'string!WAVE_INTERFERENCE/waterWaveGenerator' );
 
   // Tuned so that iPad2 has enough time to run model computations
-  const EVENT_RATE = 20;
+  const EVENT_RATE = 20; //REVIEW say more about this value, effect of increasing/decreasing
   const toFemto = WaveInterferenceUtils.toFemto;
 
   class WavesScreenModel {
@@ -68,6 +68,7 @@ define( require => {
         // This model supports one or two sources.  If the sources are initially separated, there are two sources
         numberOfSources: 1,
 
+        //REVIEW you mention 'optimize the view for the max', but what is the range? units?
         // Initial amplitude of the oscillator. We optimize the view for the max, but starting the value at the extreme
         // may prevent the user from exploring the range, so we start closer to the max but not at the max.  I chose 8
         // so it would match up directly with a tickmark (when it was at 7.5, it covered 2 tickmarks and looked odd)
@@ -78,6 +79,7 @@ define( require => {
 
         waveSpatialType: WaveSpatialType.POINT
       }, options );
+
       assert && assert(
         options.numberOfSources === 1 || options.numberOfSources === 2,
         'Model only supports 1 or 2 sources'
@@ -206,6 +208,7 @@ define( require => {
         }
       };
 
+      //REVIEW might be nice to say a few words about this
       // @private
       this.eventTimer = new EventTimer( eventTimerModel, timeElapsed =>
         this.advanceTime( 1 / EVENT_RATE, false )
@@ -366,6 +369,7 @@ define( require => {
       return this.waterScene.desiredFrequencyProperty;
     }
 
+    //REVIEW I don't recall this alternative to static constants coming up in our recent dev discussion. Just sayin'...
     /**
      * @returns {number}
      * @public

js/waves/view/WavesScreenView.js

@@ -214,6 +214,7 @@ define( require => {
         0, this.latticeNode.globalBounds.top - intensityGraphPanel.getChartGlobalBounds().top
       );
 
+      //REVIEW Property's value parameter is not optional, use Property(null) here?
       const measuringTapeProperty = new Property();
       model.sceneProperty.link( scene => {
         measuringTapeProperty.set( {