More review notes, see #253

js/common/view/GravityAndDampingControlNode.js

@@ -149,6 +149,7 @@ define( function( require ) {
         align: 'left',
         children: [
           new Text( gravityString, { font: new PhetFont( { size: 14, weight: 'bold' } ) } ),
+          //REVIEW: Similarly why would HStrut be used here? Usually we can accomplish layouts without?
           new HStrut( TITLE_OFFSET + 3 )
         ]
       } );
@@ -166,6 +167,7 @@ define( function( require ) {
       var gravitySlider = new VBox( {
         align: 'left',
         spacing: 2,
+        //REVIEW: Usually HStruts wouldn't be needed for something like this. Why is it used here?
         children: [ gravityHSliderTitle, new HBox( { children: [ new HStrut( 15 ), gravityHSlider ] } ) ]
       } );
     }
@@ -177,6 +179,7 @@ define( function( require ) {
     var questionTextNode = new VBox( {
 
       children: [
+        //REVIEW: Can we apply padding elsewhere instead of using struts?
         new VStrut( questionTextOffset ),
         new Text( whatIsTheValueOfGravityString, {
           font: MassesAndSpringsConstants.TITLE_FONT,
@@ -218,6 +221,8 @@ define( function( require ) {
         align: 'left',
         children: [
           new Text( dampingString, { font: new PhetFont( { size: 14, weight: 'bold' } ) } ),
+          //REVIEW: Lots of struts used for horizontal alignment, each with different offsets. Can we talk about layout
+          //REVIEW: and what needs to be accomplished here?
           new HStrut( TITLE_OFFSET - 7 )
         ]
       } );
@@ -246,6 +251,7 @@ define( function( require ) {
             font: MassesAndSpringsConstants.TITLE_FONT,
             maxWidth: this.maxWidth
           } ),
+          //REVIEW: Similar strut issue here
           new HStrut( TITLE_OFFSET - 35 )
         ]
       } );

js/common/view/LineVisibilityNode.js

@@ -54,6 +54,7 @@ define( function( require ) {
      */
     var createLine = function( color, tandem ) {
       return new Line( 0, 0, LINE_LENGTH, 0, {
+        //REVIEW: It's only a slight shortcut, but you can omit all 4 parameters at the start, and just specify x2: LINE_LENGTH in the options
         stroke: color,
         lineDash: [ 6, 2.5 ],
         lineWidth: 2.0,
@@ -73,13 +74,14 @@ define( function( require ) {
     } );
 
     if ( options.massEquilibrium ) {
-
       equilibriumText = new Text( massEquilibriumString, {
         font: MassesAndSpringsConstants.TITLE_FONT,
         tandem: tandem.createTandem( 'equilibriumPositionString' )
       } );
     }
 
+    //REVIEW: I'm curious why HBoxes are getting tandems here?
+
     var indicatorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ {
       content: new HBox( {
         children: [ new Text( naturalLengthString, {
@@ -123,6 +125,7 @@ define( function( require ) {
     );
     var lineVBox = new VBox( {
       children: [
+      //REVIEW: Use spacing here?
         blueLine,
         new VStrut( 24 ),
         greenLine,
@@ -132,6 +135,7 @@ define( function( require ) {
     } );
     var controlBox = new HBox( {
       children: [
+        //REVIEW: More struts, can we just use padding or other structuring instead of these?
         indicatorVisibilityControlsVBox,
         new HStrut( 10 ),
         lineVBox,

js/common/view/MassNode.js

@@ -51,7 +51,7 @@ define( function( require ) {
     Node.call( this, { cursor: 'pointer' } );
     var self = this;
 
-    // @public
+    // @public REVIEW: Type doc, since mass in the sim is either {number} or {Mass}
     this.mass = mass;
 
     var hookHeight = modelViewTransform2.modelToViewDeltaY( -mass.hookHeight );
@@ -82,6 +82,7 @@ define( function( require ) {
 
       // TODO (PERFORMANCE): If this is ever an issue (changing this every frame), try to create one object with the gradient, and then transform/scale it
       // into place.
+      //REVIEW: Is this a performance issue? If not, the TODO should be removed
       rect.fill = new LinearGradient( -rect.width / 2, 0, rect.width / 2, 0 )
         .addColorStop( 0, Color.toColor( mass.color ).colorUtilsBrighter( 0.3 ) )
         .addColorStop( 0.2, Color.toColor( mass.color ).colorUtilsBrighter( 0.8 ) )
@@ -90,10 +91,12 @@ define( function( require ) {
       // We are constraining the draggable bounds on our massNodes except when the mass is attached to a spring.
       var minY = mass.userControlledProperty.value ?
                  modelBoundsProperty.value.minY :
+                 //REVIEW: Another case with the 0.02 constant, should be factored out
                  MassesAndSpringsConstants.FLOOR_Y + 0.02 + mass.heightProperty.value;
 
       if ( mass.positionProperty.value.y < minY && !mass.springProperty.value ) {
         mass.positionProperty.set( new Vector2( mass.positionProperty.value.x, minY ) );
+        //REVIEW: Can this commented-out code be removed?
         // model.adjustDraggedMassPosition( self.mass, dragBoundsProperty.value );
       }
     } );
@@ -123,6 +126,7 @@ define( function( require ) {
     } );
     this.addChild( hookNode );
 
+    //REVIEW: I just wanted to point out that it was funny seeing `mass: mass.mass`. That's all. Remove this comment!
     var labelString = mass.mysteryLabel ? questionMarkString : StringUtils.fillIn( massValueString, { mass: mass.mass * 1000 } );
     var label = new Text( labelString, {
       font: new PhetFont( { size: 12, weight: 'bold' } ),
@@ -191,6 +195,7 @@ define( function( require ) {
     } );
 
     modelBoundsProperty.link( function( modelDragBounds ) {
+      //REVIEW: That is reaching private state of the drag handler. Use setDragBounds instead.
       self.movableDragHandler._dragBounds = modelDragBounds;
     } );
 
@@ -202,6 +207,7 @@ define( function( require ) {
     } );
 
     //Arrows created for vectors associated with mass nodes
+    //REVIEW: JSDoc
     this.velocityArrow = new VectorArrow( MassesAndSpringsConstants.VELOCITY_ARROW_COLOR );
     this.accelerationArrow = new VectorArrow( MassesAndSpringsConstants.ACCELERATION_ARROW_COLOR );
     this.gravityForceArrow = new ForceVectorArrow( MassesAndSpringsConstants.GRAVITY_ARROW_COLOR );
@@ -231,7 +237,7 @@ define( function( require ) {
      * @param {Property.<boolean>} arrowVisibilityProperty
      * @param {Node} arrowNode
      *
-     * @private
+     * @private REVIEW: Don't need visibility docs on a local variable
      */
     var updateArrowVisibility = function( arrowVisibilityProperty, arrowNode ) {
       Property.multilink( [ mass.springProperty, arrowVisibilityProperty, mass.userControlledProperty ],
@@ -276,10 +282,11 @@ define( function( require ) {
     // Show/hide line at base of vectors
     Property.multilink( [ mass.springProperty, model.gravityVectorVisibilityProperty, model.springVectorVisibilityProperty, model.forcesModeProperty ],
       function( spring, gravityForceVisible, springForceVisible, forcesVisible ) {
-        forceNullLine.visible = !!spring && (gravityForceVisible || springForceVisible || forcesVisible === MassesAndSpringsConstants.NET_FORCE_STRING);
+        forceNullLine.visible = !!spring && ( gravityForceVisible || springForceVisible || forcesVisible === MassesAndSpringsConstants.NET_FORCE_STRING );
       } );
 
     // TODO: Lots of similar code for setting arrow tail/tip. Ideally refactor to a function that can set tail/tip on all arrows (based on magnitude/etc.)
+    //REVIEW: Handle this TODO?
 
     //Links for handling the length of the vectors in response to the system.
     var scalingFactor = 3;

js/common/view/MassValueControlPanel.js

@@ -56,7 +56,7 @@ define( function( require ) {
       trackSize: new Dimension2( 125, 0.1 ),
       thumbSize: new Dimension2( 13, 24 ),
       thumbFillEnabled: '#00C4DF',
-      thumbFillHighlighted: '#71EDFF',
+      thumbFillHighlighted: '#71EDFF', //REVIEW: Should we look at factoring out some of these colors?
       stroke: null,
       sliderIndent: 7,
       majorTicks: [
@@ -80,6 +80,9 @@ define( function( require ) {
       delta: 1
     } );
 
+    //REVIEW: Is it safe to change whether a mass is adjustable after it may have been passed to things?
+    //REVIEW: MassNode checks this, is there an order dependency where it has to be created before/after this node?
+    //REVIEW: Should this be an assertion that it is adjustable?
     mass.adjustable = true;
 
     Panel.call( this, numberControl, {

js/common/view/MovableLineNode.js

@@ -25,7 +25,7 @@ define( function( require ) {
   /**
    * @param {Vector2} initialPosition - of the center of line
    * @param {number} length - in view coordinates
-   * @param {boolean} visibleProperty
+   * @param {boolean} visibleProperty REVIEW: a Property?
    * @param {Bounds2} dragBounds - limits draggable bounds of the line
    * @param {Tandem} tandem
    * @constructor
@@ -46,7 +46,7 @@ define( function( require ) {
       buttonRadius: 5,
       buttonTouchAreaDilation: 10,
       cursor: 'pointer'
-      }
+      } //REVIEW: Format with `} );` on the same line
     );
     this.addChild( laserPointerNode );
 
@@ -76,6 +76,7 @@ define( function( require ) {
     //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here.
     // @private {read-write} position of line in screen coordinates
     this.positionProperty.link( function( position ) {
+      //REVIEW: Could use .minusXY( length / 2, 0 ) to avoid creating an extra Vector2
       self.translation = position.minus( new Vector2( length / 2, 0 ) );
     } );
 

js/common/view/OneSpringScreenView.js

@@ -1,7 +1,7 @@
 // Copyright 2016-2018, University of Colorado Boulder
 
 /**
- * Common ScreenView for  using one mass.
+ * Common ScreenView for using one mass.
  *
  * @author Matt Pennington (PhET Interactive Simulations)
  * @author Denzell Barnett (PhET Interactive Simulations)
@@ -42,6 +42,7 @@ define( function( require ) {
    * @constructor
    */
   function OneSpringScreenView( model, tandem ) {
+    //REVIEW: JSDOc
     this.model = model; // Make model available
     SpringScreenView.call( this, model, tandem );
     var self = this;
@@ -72,6 +73,7 @@ define( function( require ) {
       } );
 
     // Initializes equilibrium line for the spring
+    //REVIEW: JSDoc
     this.springEquilibriumLineNode = new ReferenceLineNode(
       this.modelViewTransform,
       model.springs[ 0 ],
@@ -92,14 +94,18 @@ define( function( require ) {
       }
     );
 
+    //REVIEW: Is this something that HBox could be used for?
     var springStopperButtonNode = this.createStopperButton( this.model.springs[ 0 ], tandem );
     springStopperButtonNode.left = springHangerNode.right + this.spacing;
     this.model.springs[ 0 ].buttonEnabledProperty.link(
       function( buttonEnabled ) {
         springStopperButtonNode.enabled = buttonEnabled;
       } );
 
+    //REVIEW: JSDoc
     this.energyGraphNode = new EnergyGraphNode( model, tandem );
+    //REVIEW: Could inline topLeft: this.visibleBoundsProperty.get().topLeft.plus( new Vector2( this.spacing, this.spacing ) )
+    //REVIEW: Fine with this too
     this.energyGraphNode.top = this.visibleBoundsProperty.get().top + this.spacing;
     this.energyGraphNode.left = this.visibleBoundsProperty.get().left + this.spacing;
 
@@ -135,6 +141,7 @@ define( function( require ) {
       model, this, tandem.createTandem( 'gravityAndDampingControlNode' ),
       {
         right: this.rightPanelAlignment,
+        //REVIEW: Wait, what does minWidth do? It's not a Panel, right?
         minWidth: 1,
         maxWidth: MassesAndSpringsConstants.PANEL_MAX_WIDTH,
         dampingVisible: true
@@ -205,6 +212,8 @@ define( function( require ) {
     // Adjust the floating panels to the visibleBounds of the screen.
     this.visibleBoundsProperty.link( function( visibleBounds ) {
 
+      //REVIEW: Lots of layout here. Can we use things like AlignBox/HBox/VBox to simplify? Might be worth collaboration.
+      //REVIEW: How much of this can be shared with TwoSpringScreenView (and moved to SpringScreenView?)
       //Update the bounds of view elements
       springHangerNode.top = self.spacing;
       movableLineNode.centerX = springHangerNode.centerX;

js/common/view/OscillatingSpringNode.js

@@ -24,7 +24,7 @@ define( function( require ) {
   };
 
   /**
-   * @param {spring} spring model object
+   * @param {spring} spring model object // REVIEW: {Spring}, and probably no further docs needed
    * @param {ModelViewTransform2} modelViewTransform2
    * @param {Tandem} tandem
    * @param {Object} [options]
@@ -50,11 +50,13 @@ define( function( require ) {
     }, options );
 
     ParametricSpringNode.call( this, options );
+
+    //REVIEW: JSDoc
     this.spring = spring;
     this.translation = modelViewTransform2.modelToViewPosition(
       new Vector2( spring.positionProperty.get().x,
         spring.positionProperty.get().y - length ) );
-    this.modelViewTransform2 = modelViewTransform2;
+    this.modelViewTransform2 = modelViewTransform2; //REVIEW: JSDoc
 
     function updateViewLength() {
 
@@ -68,6 +70,7 @@ define( function( require ) {
       //The wrong side of the PSN is static, so we have to put the spring in reverse and update the length AND position.
       //Spring is rotated to be rotated so XScale relates to Y-direction in view
       //TODO There is possibly a better solution by setting the phase and deltaPhase.
+      //REVIEW: Handle the TODO?
       self.xScaleProperty.set( xScale );
       self.y = modelViewTransform2.modelToViewY( spring.positionProperty.get().y - spring.lengthProperty.get() );
     }
@@ -91,6 +94,7 @@ define( function( require ) {
   massesAndSprings.register( 'OscillatingSpringNode', OscillatingSpringNode );
 
   return inherit( ParametricSpringNode, OscillatingSpringNode, {
+    //REVIEW: Indentation should be  here
       /**
        * @public
        */

js/common/view/ReferenceLineNode.js

@@ -24,7 +24,7 @@ define( function( require ) {
   /**
    * @param {ModelViewTransform2} modelViewTransform2
    * @param {Spring} spring - spring model object
-   * @param {Property} property - determines which property is being referenced
+   * @param {Property} property - determines which property is being referenced REVIEW: Property of what? If it can be anything, Property.<*>
    * @param {Property.<boolean>} visibleProperty
    * @param {Object} [options]
    *
@@ -88,6 +88,8 @@ define( function( require ) {
 
     visibleProperty.linkAttribute( self, 'visible' );
 
+    //REVIEW: Lots of similarities between this and MovableLineNode. Could be considered to be factored out.
+
   }
 
   massesAndSprings.register( 'ReferenceLineNode', ReferenceLineNode );

js/common/view/SpringControlPanel.js

@@ -25,7 +25,7 @@ define( function( require ) {
    * @param {Property.<number>} springProperty - Property to be adjusted by hSlider
    * @param {Range} propertyRange - range of values for length
    * @param {string} title - string used to title the panel
-   * @param {array.<Text>} labels - formatted as: [ minLabel, maxLabel]
+   * @param {Array.<Text>} labels - formatted as: [ minLabel, maxLabel]
    * @param {Tandem} tandem
    * @param {Object} options
    * @constructor

js/common/view/SpringHangerNode.js

@@ -21,8 +21,9 @@ define( function( require ) {
   // constants
   var SPRING_HANGER_FONT = new PhetFont( { size: 16, weight: 'bold' } );
   var SPRING_HANGER_FILL = 'rgb( 180, 180, 180 )';
+
   /**
-   * @param {Array} springs
+   * @param {Array} springs REVIEW: {Array.<Spring>}?
    * @param {ModelViewTransform2} modelViewTransform2
    * @param {Tandem} tandem
    * @param {Object} options
@@ -63,6 +64,7 @@ define( function( require ) {
         tandem: tandem.createTandem( '2' ),
         centerX: springsSeparation
       } ) );
+      //REVIEW: springHangerLabelNode.center = this.center;  -- works equivalently?
       springHangerLabelNode.centerX = this.width / 2;
       springHangerLabelNode.centerY = this.height / 2;
       this.addChild( springHangerLabelNode );

js/common/view/SpringScreenView.js

@@ -46,13 +46,15 @@ define( function( require ) {
     // @public {MassesAndSpringsModel}
     this.model = model;
 
+    //REVIEW: Is this another case where the 0.02 constant needs to be factored out?
     var viewOrigin = new Vector2( 0, this.visibleBoundsProperty.get().height * 0.98 );
 
     // @public {ModelViewTransform2}
     this.modelViewTransform = ModelViewTransform2.createSinglePointScaleInvertedYMapping( Vector2.ZERO, viewOrigin, 397 );
 
     // @protected {Array.<OscillatingSpringNode>} Used to reference the created springs in the view.
     this.springNodes = [];
+    //REVIEW: Could do `this.springNodes = model.springs.map( ... )` with a function that returns it. Then separate pushes not needed
     model.springs.forEach( function( spring ) {
       var springNode = new OscillatingSpringNode(
         spring,

js/common/view/TwoSpringScreenView.js

@@ -150,6 +150,8 @@ define( function( require ) {
     this.visibleBoundsProperty.link( function( visibleBounds ) {
 
       //Update the bounds of view elements
+      //REVIEW: Lots of layout here. Can we use things like AlignBox/HBox/VBox to simplify? Might be worth collaboration.
+      //REVIEW: How much of this can be shared with OneSpringScreenView (and moved to SpringScreenView?)
       self.springHangerNode.top = model.springs[ 0 ].positionProperty.value.y + self.spacing;
       self.gravityAndDampingControlNode.right = self.panelRightSpacing;
       self.toolboxPanel.right = self.panelRightSpacing;