first pass through coding convention in view, #259

Signed-off-by: Chris Malley <cmalley@pixelzoom.com>

js/common/WaveInterferenceConstants.js

@@ -55,12 +55,14 @@ define( require => {
     CHART_LINE_JOIN: 'round',
 
     // Look of the emitter button across all 3 scenes
+    //REVIEW where is the color for the green emitter button in pulse mode?
     EMITTER_BUTTON_COLOR: 'red',
     EMITTER_BUTTON_RADIUS: 14,
     EMITTER_BUTTON_TOUCH_AREA_DILATION: 8,
 
     FEMTO: 1E-15,
 
+    //REVIEW no idea how I'd coordinate this, and WaterEmitterNode waterDrops does not exist
     // Cell that oscillates, specified as an offset from the origin of the lattice (includes damping region).  This
     // value must be coordinated with WaterEmitterNode's waterDrops[ index ].centerX
     POINT_SOURCE_HORIZONTAL_COORDINATE: 23,
@@ -79,11 +81,14 @@ define( require => {
     // maxWidth for slider ticks
     TICK_MAX_WIDTH: 30,
 
+    //REVIEW 'a smidge smaller than the reset of the texts' means a smidge smaller than DEFAULT_FONT, yes?  Then factor out DEFAULT_FONT_SIZE and make it so.
     // Use for the time and length scale texts above the wave area, looks best to be a smidge smaller than the
     // rest of the texts
     TIME_AND_LENGTH_SCALE_INDICATOR_FONT: new PhetFont( 14 )
   };
 
+  //REVIEW assert that WaveInterferenceConstants.LATTICE_DIMENSION is an odd integer
+
   waveInterference.register( 'WaveInterferenceConstants', WaveInterferenceConstants );
 
   return WaveInterferenceConstants;

js/common/WaveInterferenceUtils.js

@@ -23,7 +23,6 @@ define( require => {
 
   class WaveInterferenceUtils {
 
-
     /**
      * Gets a Shape representing the top of the water in water side view from left to right, also used for the chart.
      * @param {number[]} array - reused to avoid allocations
@@ -60,6 +59,8 @@ define( require => {
      * @public
      */
     static getWaterSideY( waveAreaBounds, waveValue ) {
+
+      //REVIEW explain the magic numbers here
       return Util.linear( 0, 5, waveAreaBounds.centerY, waveAreaBounds.centerY - 80, waveValue );
     }
 
@@ -97,6 +98,7 @@ define( require => {
       return value / WaveInterferenceConstants.FEMTO;
     }
 
+    //REVIEW unnecessary coupling, passing in the entire model when only model.waterScene.lattice.visibleBounds is needed
     /**
      * Gets the horizontal coordinate where water drops come out--aligned with the oscillation cell.
      * @param {WavesScreenModel} model

js/common/model/Scene.js

@@ -62,11 +62,11 @@ define( require => {
       // @private {number} - indicates the time when the pulse began, or 0 if there is no pulse.
       this.pulseStartTime = 0;
 
-      // @public {Property.<Boolean>} - whether the button for the first source is pressed.  This is also used for the
+      // @public whether the button for the first source is pressed.  This is also used for the
       // slits screen plane wave source.
       this.button1PressedProperty = new BooleanProperty( false );
 
-      // @public {Property.<Boolean>} - whether the button for the second source is pressed
+      // @public whether the button for the second source is pressed
       this.button2PressedProperty = new BooleanProperty( false );
 
       // @public (read-only) {string} - units for this scene

js/common/model/ViewType.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW ViewType is a pretty weak name. ViewpointEnum?
 /**
  * Selects whether the wave is shown from the top (default) or side view.  This value represents the user selection--
  * the view animates between the selections.

js/common/model/WaterScene.js

@@ -67,8 +67,8 @@ define( require => {
     /**
      * Fire another water drop if one is warranted for the specified faucet.  We already determined that the timing
      * is right (for the period), now we need to know if a drop should fire.
-     * @param {Property.<Boolean>} buttonProperty - indicates whether the corresponding button is pressed
-     * @param {Property.<Boolean>} oscillatingProperty - indicates whether the wave source is oscillating
+     * @param {BooleanProperty} buttonProperty - indicates whether the corresponding button is pressed
+     * @param {BooleanProperty} oscillatingProperty - indicates whether the wave source is oscillating
      * @param {number} sign - -1 for top faucet, +1 for bottom faucet
      * @private
      */

js/common/model/WaveTemporalType.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW a better name for this might be DisturbanceType.  A wave is a continuous disturbance. A pulse is a one-time disturbance.
 /**
  * Determines whether the incoming wave is continuous or pulse.
  *

js/common/view/DashedLineNode.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW This doc was confusing. It's not a 'dotted line', it's a dashed line with stroked dashes.
 /**
  * When the graph is selected, the dotted line is shown in the center of the WaveAreaNode and in the graph's center.
  *

js/common/view/EmitterNode.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW #272 Clarify 'emitter' in this context, or use a synonym, to prevent confusion with AXON/Emitter.
 /**
  * For each scene, shows one node for each emitter, each with its own on/off button.
  *

js/common/view/InputTypeIconNode.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW This is a visual representation of WaveTemporalType, so why is this class name totally different? why not WaveTermTypeNode?
 /**
  * Shows the icons for the radio buttons that choose between pulse and continuous waves.
  *
@@ -26,11 +27,13 @@ define( require => {
   class InputTypeIconNode extends Node {
 
     /**
-     * @param {WaveTemporalType} incidentWaveType
+     * @param {WaveTemporalType} incidentWaveType //REVIEW rename to waveTemporalType
      * @param {Object} [options]
      */
     constructor( incidentWaveType, options ) {
       super();
+
+      //REVIEW Implementation is unnecessarily complicated because you're trying to do both icons in one class. Separate into 2 classes?
       const minAngle = incidentWaveType === WaveTemporalType.PULSE ? Math.PI : 0;
       const minX = incidentWaveType === WaveTemporalType.PULSE ? MARGIN : 0;
       const maxX = incidentWaveType === WaveTemporalType.PULSE ? ( WIDTH - MARGIN ) : WIDTH;
@@ -56,6 +59,7 @@ define( require => {
       if ( incidentWaveType === WaveTemporalType.PULSE ) {
         shape.lineToRelative( MARGIN, 0 );
       }
+
       this.addChild( new Path( shape, {
         stroke: 'black',
         lineWidth: 2

js/common/view/IntensityGraphPanel.js

@@ -52,7 +52,7 @@ define( require => {
       } );
 
       /**
-       * Creates a line an the given y-coordinate.
+       * Creates a line on the given y-coordinate.
        * @param {number} index
        * @param {number} y
        * @returns {Line}

js/common/view/LatticeCanvasNode.js

@@ -95,6 +95,7 @@ define( require => {
      * Draws into the canvas.
      * @param {CanvasRenderingContext2D} context
      * @public
+     * @override
      */
     paintCanvas( context ) {
 

js/common/view/LengthScaleIndicatorNode.js

@@ -2,7 +2,7 @@
 
 /**
  * Appears above the lattice and shows the scale, like this:
- * |<------>| 500 nanometers
+ * |<------>| 500 nm
  *
  * @author Sam Reid (PhET Interactive Simulations)
  */

js/common/view/LightEmitterNode.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW #272 clarify emitter in this context
 /**
  * For the light scene, shows one laser pointer for each emitter, each with its own on/off button.
  *

js/common/view/LightScreenNode.js

@@ -52,7 +52,7 @@ define( require => {
       // @private
       this.intensitySample = intensitySample;
 
-      // @private
+      // @private {Color} required because we'll be operating on a Color
       this.baseColor = new Color( 'blue' );
 
       // Render into a sub-canvas which will be drawn into the rendering context at the right scale.
@@ -96,6 +96,8 @@ define( require => {
     /**
      * Draws into the canvas.
      * @param {CanvasRenderingContext2D} context
+     * @public
+     * @override
      */
     paintCanvas( context ) {
 

js/common/view/Perspective3DNode.js

@@ -28,7 +28,7 @@ define( require => {
     /**
      * @param {Bounds2} waveAreaBounds
      * @param {NumberProperty} rotationAmountProperty
-     * @param {DerivedProperty.<boolean>} isRotatingProperty
+     * @param {DerivedProperty.<boolean>} isRotatingProperty //REVIEW why does this need to be a DerivedProperty?
      */
     constructor( waveAreaBounds, rotationAmountProperty, isRotatingProperty ) {
 
@@ -108,7 +108,8 @@ define( require => {
     }
 
     /**
-     * @private - update the shapes and text when the rotationAmount has changed
+     * Update the shapes and text when the rotationAmount has changed
+     * @private
      */
     update() {
 

js/common/view/PulseContinuousRadioButtonGroup.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW since this is related to WaveTemporalType, rename to WaveTemporalTypeRadioButtonGroup ?
 /**
  * Shows the "pulse" vs "continuous" radio buttons.
  *

js/common/view/SoundEmitterNode.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW #272 clarify emitter in this context
 /**
  * For the sound scene, shows one speaker for each emitter, each with its own on/off button.
  *

js/common/view/SoundParticleLayer.js

@@ -15,6 +15,7 @@ define( require => {
   const waveInterference = require( 'WAVE_INTERFERENCE/waveInterference' );
 
   // constants
+  //REVIEW say more about RESOLUTION. Looks like same semantics as resolution option to Node.rasterized, but I had to work for it.
   const RESOLUTION = 2; // Render at increased resolution so particles don't appear pixellated on a large screen.
 
   class SoundParticleLayer extends CanvasNode {
@@ -45,6 +46,7 @@ define( require => {
         waveAreaNodeBounds
       );
 
+      //REVIEW looks like this could be factored out of constructor
       const toImage = color => new ShadedSphereNode( 10, {
         mainColor: color,
         stroke: 'black'
@@ -72,6 +74,8 @@ define( require => {
     /**
      * Draws into the canvas.
      * @param {CanvasRenderingContext2D} context
+     * @public
+     * @override
      */
     paintCanvas( context ) {
       context.transform( 1 / RESOLUTION, 0, 0, 1 / RESOLUTION, 0, 0 );

js/common/view/TimeControls.js

@@ -1,7 +1,7 @@
 // Copyright 2018, University of Colorado Boulder
 
 /**
- * Buttons for play/pause radio buttons for normal/slow
+ * Buttons for play/pause, and radio buttons for normal/slow
  *
  * @author Sam Reid (PhET Interactive Simulations)
  */

js/common/view/ToolboxPanel.js

@@ -64,7 +64,7 @@ define( require => {
 
       // Make sure the probes have enough breathing room so they don't get shoved into the WaveMeterNode icon
       // The true value is set when dragging
-      waveMeterNode.backgroundNode.translate( 100, 0 );
+      waveMeterNode.backgroundNode.translate( 100, 0 ); //REVIEW magic number?
 
       // The draggable icon, which has an overlay to make the buttons draggable instead of pressable
       // Temporarily show the node so it can be rasterized for an icon
@@ -92,6 +92,8 @@ define( require => {
             waveMeterIcon
           ]
         } ) ), {
+
+          // Panel options
           yMargin: 9.55,
           maxWidth: WaveInterferenceConstants.PANEL_MAX_WIDTH
         }
@@ -102,7 +104,7 @@ define( require => {
   /**
    * Initialize the icon for use in the toolbox.
    * @param {Node} node
-   * @param {Property.<Boolean>} inPlayAreaProperty
+   * @param {Property.<boolean>} inPlayAreaProperty
    * @param {Object} forwardingListener
    */
   const initializeIcon = ( node, inPlayAreaProperty, forwardingListener ) => {

js/common/view/ViewRadioButtonGroup.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW since this is related to ViewType, rename to ViewTypeRadioButtonGroup ?
 /**
  * Selects between Top View and Side View.
  *

js/common/view/WaterDropImage.js

@@ -24,6 +24,7 @@ define( require => {
     constructor( options ) {
       super( waterDropImage, options );
 
+      //REVIEW {WaterDrop|null} and presumably null means no associated WaterDrop instance?
       // @public {WaterDrop} - Link each Image to its corresponding WaterDrop, so that when the view goes underwater,
       // we can mark the corresponding model as absorbed.
       this.waterDrop = null;

js/common/view/WaterDropLayer.js

@@ -37,6 +37,7 @@ define( require => {
       const MAX_DROPS = 4;
       const waterDropNodes = _.times( MAX_DROPS, () => new WaterDropImage() );
 
+      //REVIEW assert( !options || !options.children ) or your children may immediately be replaced by this.mutate
       this.children = waterDropNodes;
       this.mutate( options );
 

js/common/view/WaterEmitterNode.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW #272 clarify emitter in this context
 /**
  * For the water scene, shows one hose for each emitter, each with its own on/off button. This implementation is trivial
  * and doesn't add state or methods, it simplifies readability at the call site, so we keep it as a convenience
@@ -33,8 +34,8 @@ define( require => {
     constructor( model, waveAreaNode, isPrimarySource ) {
 
       const faucetNode = new FaucetNode(
-        // This value is irrelevant because we use our own faucet water emitting model, but must be nonzero
-        // to prevent a divide by zero problem
+        // This value for maxFlowRate is irrelevant because we use our own faucet water emitting model,
+        // but must be nonzero to prevent a divide by zero problem
         1,
 
         // Flow rate is managed by this simulation and not depicted by the FaucetNode

js/common/view/WaterSideViewNode.js

@@ -27,6 +27,7 @@ define( require => {
      */
     constructor( waveAreaBounds, model ) {
 
+      //REVIEW vestigial comment and annotation?
       // @private - depicts the side face (when the user selects "side view")
       super( null, {
         lineJoin: WaveInterferenceConstants.CHART_LINE_JOIN,

js/common/view/WaveAreaGraphNode.js

@@ -43,6 +43,7 @@ define( require => {
 
       const graphWidth = WaveInterferenceConstants.WAVE_AREA_WIDTH;
 
+      //REVIEW nice to know that graphHeight and dy must be synchronized, but how?
       // Manually tuned so the graph is the appropriate height, must be synchronized with dy below.
       const graphHeight = WaveInterferenceConstants.WAVE_AREA_WIDTH * 0.3833333333333333;
 
@@ -171,6 +172,7 @@ define( require => {
         centerY: plotHeight / 2
       } ) );
 
+      //REVIEW describe what's going on here
       [ 1 / 4, 3 / 4 ].forEach( horizontalGridLineFraction => {
         this.addChild( new Line(
           0, horizontalGridLineFraction * plotHeight,

js/common/view/WaveInterferenceControlPanel.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW There are definitely similarities in this control panel for the 3 scenes. But the reuse of what's common in this implementation feels forced/complicated. I'd hate to have to maintain this.
 /**
  * Shows the main controls, including frequency/wavelength and amplitude.
  *
@@ -61,6 +62,7 @@ define( require => {
         maxWidth: WaveInterferenceConstants.PANEL_MAX_WIDTH
       }, options );
 
+      //REVIEW 'metric coordinate frame' is not one of the 3 coordinate frames described in implmentation notes. What is this?
       // Controls are in the metric coordinate frame
       const waterFrequencySlider = new WaveInterferenceSlider(
         model.getWaterFrequencySliderProperty(),
@@ -111,6 +113,8 @@ define( require => {
         );
       } );
 
+      //REVIEW worthy of factoring out into its own class file, SoundViewTypeRadioButtonGroup?
+      //REVIEW PhET-iO: rename const to soundViewTypeRadioButtonGroup, since it's setting SoundViewType
       const viewSelectionRadioButtonGroup = new VerticalAquaRadioButtonGroup( [ {
         node: new WaveInterferenceText( wavesString ),
         value: SoundViewType.WAVES,
@@ -163,6 +167,8 @@ define( require => {
       // Create faucet icon, and rasterize to clip out invisible parts (like the ShooterNode)
       const sceneIcons = new WaveInterferenceSceneIcons();
 
+      //REVIEW worthy of factoring out into its own class file, SceneRadioButtonGroup?
+      //REVIEW PhET-iO: rename const to sceneRadioButtonGroup
       const sceneRadioButtons = new RadioButtonGroup( model.sceneProperty, [
         { value: model.waterScene, node: sceneIcons.faucetIcon },
         { value: model.soundScene, node: sceneIcons.speakerIcon },
@@ -266,6 +272,7 @@ define( require => {
         ];
       } );
 
+      //REVIEW move assignments of point areas closer to definition of these checkboxes?
       graphCheckbox.mouseArea = graphCheckbox.localBounds.dilated( 2 ).withX( separator.right );
       graphCheckbox.touchArea = graphCheckbox.mouseArea;
 

js/common/view/WaveInterferenceSceneIcons.js

@@ -1,5 +1,6 @@
 // Copyright 2018, University of Colorado Boulder
 
+//REVIEW very odd to have a class for this, especially since the icons are not at all related. Factory pattern would be better.
 /**
  * Creates a set of uniformly-sized icons for each of the scenes.
  *