Review items in MAS with quick scan, see phetsims/masses-and-springs-…

…basics#48
phetsims · Jan 17, 2019 · 5776ef4 · 5776ef4
1 parent ef405b3
commit 5776ef4
Show file tree

Hide file tree

Showing 29 changed files with 80 additions and 38 deletions.
diff --git a/js/common/enum/ForcesModeChoice.js b/js/common/enum/ForcesModeChoice.js
@@ -12,5 +12,6 @@ define( function( require ) {
   var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' );
   var Enumeration = require( 'PHET_CORE/Enumeration' );
 
+  // REVIEW: The recent decision was to move enumerations out of enum/ folders. Can you put it in model/?
   return massesAndSprings.register( 'ForcesModeChoice', new Enumeration( [ 'FORCES', 'NET_FORCES' ] ) );
 } );
diff --git a/js/common/enum/SimSpeedChoice.js b/js/common/enum/SimSpeedChoice.js
@@ -12,5 +12,6 @@ define( function( require ) {
   var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' );
   var Enumeration = require( 'PHET_CORE/Enumeration' );
 
+  // REVIEW: The recent decision was to move enumerations out of enum/ folders. Can you put it in model/?
   return massesAndSprings.register( 'SimSpeedChoice', new Enumeration( [ 'NORMAL', 'SLOW' ] ) );
 } );
diff --git a/js/common/model/MassesAndSpringsModel.js b/js/common/model/MassesAndSpringsModel.js
@@ -46,9 +46,11 @@ define( function( require ) {
       basicsVersion: false
     }, options );
 
+    // REVIEW: Don't store the options object, but just whether it is a basicsVersion or not. (this.basicsVersion = ...)
     this.options = options;
 
     if ( options.basicsVersion ) {
+      // REVIEW: Shouldn't have a console.log for this.
       console.log( 'basics version' );
     }
 

diff --git a/js/common/model/Spring.js b/js/common/model/Spring.js
@@ -19,6 +19,7 @@ define( function( require ) {
   var inherit = require( 'PHET_CORE/inherit' );
   var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' );
   var MassesAndSpringsConstants = require( 'MASSES_AND_SPRINGS/common/MassesAndSpringsConstants' );
+  // REVIEW: Can we remove commented-out lines?
   // var Mass = require( 'MASSES_AND_SPRINGS/common/model/Mass' );
   // var MassIO = require( 'MASSES_AND_SPRINGS/common/model/MassIO' );
   // var NullableIO = require( 'TANDEM/types/NullableIO' );
@@ -218,9 +219,11 @@ define( function( require ) {
         this.naturalRestingLengthProperty
       ],
       function( springConstant, gravity, mass, naturalRestingLength ) {
+        // REVIEW: Is it possible to combine this with the multilink that sets massEquilibriumYPositionProperty?
+        // REVIEW: The springExtensionValue is computed in both.
         if ( mass ) {
           var springExtensionValue =
-            ( mass.massProperty.value * self.gravityProperty.value) / self.springConstantProperty.value;
+            ( mass.massProperty.value * self.gravityProperty.value ) / self.springConstantProperty.value;
           self.equilibriumYPositionProperty.set(
             self.positionProperty.get().y - naturalRestingLength - springExtensionValue );
         }
@@ -262,7 +265,7 @@ define( function( require ) {
 
           // springExtension = mg/k
           var springExtensionValue =
-            ( mass.massProperty.value * self.gravityProperty.value) / self.springConstantProperty.value;
+            ( mass.massProperty.value * self.gravityProperty.value ) / self.springConstantProperty.value;
           self.massEquilibriumYPositionProperty.set(
             self.positionProperty.get().y - naturalRestingLength - springExtensionValue - mass.heightProperty.value / 2
           );
@@ -340,7 +343,7 @@ define( function( require ) {
      * @param {Object} springState - Sets the springs's properties with previously stored properties. See getSpringState
      *
      * @public
-     * @returns {Object}
+     * @returns {Object} REVIEW: I don't see a return value here
      */
     setSpringState: function( springState ) {
       this.displacementProperty.set( springState.displacement );
@@ -401,7 +404,8 @@ define( function( require ) {
       this.buttonEnabledProperty.set( false );
     },
 
-    /** Updates the displacement Property of the spring.
+    /**
+     * Updates the displacement Property of the spring.
      * @param {number} yPosition
      * @param {boolean} factorNaturalLength
      *

diff --git a/js/common/view/DraggableRulerNode.js b/js/common/view/DraggableRulerNode.js
@@ -67,6 +67,7 @@ define( function( require ) {
     }, { tandem: tandem.createTandem( 'ruler' ) } );
 
     // @private {Vector2} (read-only) position of ruler node in screen coordinates
+    // REVIEW: JSDoc looks incorrect
     this.positionProperty = new Property( initialPosition, {
       tandem: tandem.createTandem( 'positionProperty' ),
       phetioType: PropertyIO( Vector2IO )

diff --git a/js/common/view/DraggableTimerNode.js b/js/common/view/DraggableTimerNode.js
@@ -47,11 +47,13 @@ define( function( require ) {
 
 
     // @private {Vector2} (read-only) position of ruler node in screen coordinates
+    // REVIEW: Seems like incorrect type JSDoc
     this.positionProperty = new Property( initialPosition, {
       tandem: tandem.createTandem( 'positionProperty' ),
       phetioType: PropertyIO( Vector2IO )
     } );
     this.positionProperty.linkAttribute( this, 'translation' );
+    // REVIEW: Also seems like a lot of code duplicated with DraggableRulerNode. Is there anything that can be deduplicated?
 
     // @private {MovableDragHandler} (read-only) handles timer node drag events
     this.timerNodeMovableDragHandler = new MovableDragHandler( this.positionProperty, {

diff --git a/js/common/view/EnergyGraphNode.js b/js/common/view/EnergyGraphNode.js
@@ -58,7 +58,6 @@ define( function( require ) {
   var totalEnergyString = require( 'string!MASSES_AND_SPRINGS/totalEnergy' );
 
   /**
-   *
    * @param {MassesAndSpringsModel} model
    * @param {Tandem} tandem
    * @constructor
@@ -92,7 +91,7 @@ define( function( require ) {
       self.zoomLevelProperty.value += 1;
     } );
 
-    // {DerivedProperty.<number>} Responsible for adjusting the scaling of the barNode heights.
+    // {Property.<number>} Responsible for adjusting the scaling of the barNode heights.
     var scaleFactorProperty = new DerivedProperty( [ this.zoomLevelProperty ], function( zoomLevel ) {
       return Math.pow( 2, zoomLevel ) * 20;
     } );
@@ -294,6 +293,7 @@ define( function( require ) {
     reset: function() {
       this.zoomLevelProperty.reset();
     },
+
     /**
      * @public
      */

diff --git a/js/common/view/ForceVectorArrow.js b/js/common/view/ForceVectorArrow.js
@@ -29,5 +29,6 @@ define( function( require ) {
   }
 
   massesAndSprings.register( 'ForceVectorArrow', ForceVectorArrow );
+
   return inherit( ArrowNode, ForceVectorArrow );
 } );
diff --git a/js/common/view/MassNode.js b/js/common/view/MassNode.js
@@ -63,6 +63,7 @@ define( function( require ) {
       hookHeight = modelViewTransform2.modelToViewDeltaY( -MassesAndSpringsConstants.HOOK_HEIGHT * 0.34 );
     }
 
+    // REVIEW: JSDoc?
     this.rect = new Rectangle( {
       stroke: 'black',
       boundsMethod: 'unstroked',
@@ -102,6 +103,7 @@ define( function( require ) {
       }
     } );
 
+    // REVIEW: Why JSDoc here?
     // @public Sets the gradient on the massNode.
     this.rect.fill = new LinearGradient( -this.rect.width / 2, 0, this.rect.width / 2, 0 )
       .addColorStop( 0, Color.toColor( mass.color ).colorUtilsBrighter( 0.1 ) )
@@ -113,6 +115,7 @@ define( function( require ) {
     hookShape.arc( 0, 0, radius, Math.PI, ( 0.5 * Math.PI ) );
     hookShape.lineTo( 0, hookHeight / 2 );
 
+    // REVIEW: Type doc?
     // @public Used for hook on massNode.
     this.hookNode = new Path( hookShape, {
       stroke: 'black',
@@ -255,7 +258,7 @@ define( function( require ) {
      * @param {Property.<boolean>} arrowVisibilityProperty
      * @param {Node} arrowNode
      *
-     * @private
+     * @private REVIEW: No visibility for a local variable
      */
     var updateForceVisibility = function( arrowVisibilityProperty, arrowNode ) {
       Property.multilink( [ mass.springProperty, arrowVisibilityProperty, model.forcesModeProperty ],
@@ -330,7 +333,7 @@ define( function( require ) {
       function( spring, gravity ) {
         var gravitationalAcceleration = mass.mass * gravity;
         position = mass.positionProperty.get();
-        xOffset = (forcesOrientation) * 45;
+        xOffset = forcesOrientation * 45;
         y2 = ARROW_SIZE_DEFAULT * gravitationalAcceleration;
         updateArrow( gravityForceArrow, position, xOffset, y2 );
       } );
@@ -339,7 +342,7 @@ define( function( require ) {
     Property.multilink( [ mass.springForceProperty ],
       function( springForce ) {
         position = mass.positionProperty.get();
-        xOffset = (forcesOrientation) * 45;
+        xOffset = forcesOrientation * 45;
         y2 = -ARROW_SIZE_DEFAULT * springForce;
         updateArrow( springForceArrow, position, xOffset, y2 );
       } );
@@ -354,7 +357,7 @@ define( function( require ) {
     ], function( netForce, accelerationVisible, netAcceleration, velocityVisible ) {
       position = mass.positionProperty.get();
       if ( Math.abs( netForce ) > 1E-6 ) {
-        xOffset = (forcesOrientation) * 45;
+        xOffset = forcesOrientation * 45;
         y2 = -ARROW_SIZE_DEFAULT * netForce;
         updateArrow( netForceArrow, position, xOffset, y2 );
       }
@@ -374,12 +377,13 @@ define( function( require ) {
     // When the mass's position changes update the forces baseline marker
     mass.positionProperty.link( function( position ) {
       forceNullLine.setLine(
-        self.rect.centerX + (forcesOrientation) * 40, position.y + self.rect.centerY,
-        self.rect.centerX + (forcesOrientation) * 50, position.y + self.rect.centerY
+        self.rect.centerX + forcesOrientation * 40, position.y + self.rect.centerY,
+        self.rect.centerX + forcesOrientation * 50, position.y + self.rect.centerY
       );
     } );
   }
 
   massesAndSprings.register( 'MassNode', MassNode );
+
   return inherit( Node, MassNode );
 } );
diff --git a/js/common/view/MassValueControlPanel.js b/js/common/view/MassValueControlPanel.js
@@ -48,7 +48,7 @@ define( function( require ) {
       stroke: 'gray',
       yMargin: 6,
       xMargin: 6,
-      basicsVersion: false,
+      basicsVersion: false, // REVIEW: Where is this used?
       tandem: tandem
     }, options );
 
@@ -65,7 +65,7 @@ define( function( require ) {
       }
     } );
 
-    // @public {Property.<Dimension2>}
+    // @public {Property.<Dimension2>} REVIEW: No JSDoc on local variable?
     var trackSizeProperty = new Property( options.basics ? new Dimension2( 132, 0.1 ) : new Dimension2( 125, 0.1 ) );
 
     var numberControl = new MutableOptionsNode( NumberControl, [ massString, massInGramsProperty, range ], {
@@ -116,5 +116,6 @@ define( function( require ) {
   }
 
   massesAndSprings.register( 'MassValueControlPanel', MassValueControlPanel );
+
   return inherit( Panel, MassValueControlPanel );
 } );
diff --git a/js/common/view/MovableLineNode.js b/js/common/view/MovableLineNode.js
@@ -85,6 +85,7 @@ define( function( require ) {
     initialPosition.setX( dragBounds.minX );
 
     // @private {Vector2} (read-write) position of line in screen coordinates
+    // REVIEW: Incorrect JSDoc
     this.positionProperty = new Property( initialPosition, {
       tandem: tandem.createTandem( 'positionProperty' ),
       phetioType: PropertyIO( Vector2IO )
@@ -113,5 +114,4 @@ define( function( require ) {
       this.positionProperty.reset();
     }
   } );
-
 } );
diff --git a/js/common/view/OneSpringScreenView.js b/js/common/view/OneSpringScreenView.js
@@ -46,7 +46,7 @@ define( function( require ) {
     SpringScreenView.call( this, model, tandem, options );
     var self = this;
 
-    // @public {Number} centerX of the spring in view coordinates
+    // @public {number} centerX of the spring in view coordinates
     this.springCenter = self.modelViewTransform.modelToViewX( model.firstSpring.positionProperty.value.x );
 
     // Spring Constant Control Panel
@@ -55,7 +55,7 @@ define( function( require ) {
       new Text( largeString, { font: MassesAndSpringsConstants.LABEL_FONT, maxWidth: 60 } )
     ];
 
-    // @public {DerivedProperty.<boolean>} Equilibrium of mass is dependent on the mass being attached and the visibility of the equilibrium line.
+    // @public {Property.<boolean>} Equilibrium of mass is dependent on the mass being attached and the visibility of the equilibrium line.
     this.equilibriumVisibilityProperty = new DerivedProperty(
       [ model.equilibriumPositionVisibleProperty, model.firstSpring.massAttachedProperty ],
       function( equilibriumPositionVisible, massAttached ) {
@@ -121,6 +121,7 @@ define( function( require ) {
         self.springStopperButtonNode.enabled = buttonEnabled;
       } );
 
+    // REVIEW: Turn this into model.basicsVersion, and properly document it.
     if ( !model.options.basicsVersion ) {
       // @public {EnergyGraphNode} energy graph that displays energy values for the spring system.
       this.energyGraphNode = new EnergyGraphNode( model, tandem );
@@ -130,7 +131,7 @@ define( function( require ) {
     // Property that determines the zero height in the view.
     var zeroHeightProperty = new Property( this.modelViewTransform.modelToViewY( MassesAndSpringsConstants.FLOOR_Y ) );
 
-    // @public {MovableLineNode} Initializes movable line
+    // @public {MovableLineNode} Initializes movable line REVIEW: No JSDoc on local variable
     var xBoundsLimit = this.springCenter + this.spacing * 1.1;
     this.movableLineNode = new MovableLineNode(
       this.springHangerNode.center.plus( new Vector2( 45, 200 ) ),
@@ -176,6 +177,8 @@ define( function( require ) {
       this.resetAllButton.addListener( function() {
         self.model.reset();
         self.movableLineNode.reset();
+        // REVIEW: Generally prefer `self.energyGraphNode && self.energyGraphNode.reset` if it is optional, instead of
+        // REVIEW: repeating the same constraints.
         if ( !model.options.basicsVersion ) {
           self.energyGraphNode.reset();
         }
@@ -269,6 +272,7 @@ define( function( require ) {
       MassesAndSpringsModel.prototype.reset.call( this );
       this.energyGraphNode.reset();
     },
+
     /**
      * Responsible for updating the energy bar graph
      *

diff --git a/js/common/view/OscillatingSpringNode.js b/js/common/view/OscillatingSpringNode.js
@@ -53,6 +53,7 @@ define( function( require ) {
 
     // @public {Spring} (read-only)
     this.spring = spring;
+
     this.translation = modelViewTransform2.modelToViewPosition(
       new Vector2( spring.positionProperty.get().x,
         spring.positionProperty.get().y - length ) );

diff --git a/js/common/view/ReferenceLineNode.js b/js/common/view/ReferenceLineNode.js
@@ -58,6 +58,7 @@ define( function( require ) {
     var yPos = modelViewTransform2.modelToViewY( lengthFunction( spring.naturalRestingLengthProperty.value ) );
 
     // @private {Vector2} (read-write) position of line in screen coordinates.
+    // REVIEW: Incorrect JSDoc
     this.positionProperty = new Property( new Vector2( xPos, yPos ), {
       phetioType: PropertyIO( Vector2IO )
     } );
@@ -94,7 +95,7 @@ define( function( require ) {
       // Adjust the position of the label
       if ( options.label ) {
         options.label.centerY = 0;
-        options.label.x = (LINE_LENGTH + 10);
+        options.label.x = LINE_LENGTH + 10;
       }
     } );
 

diff --git a/js/common/view/Shelf.js b/js/common/view/Shelf.js
@@ -17,6 +17,7 @@ define( function( require ) {
    * @param {Tandem} tandem
    * @param {Object} [options]
    * @constructor
+   * REVIEW: ShelfNode might be a better name? `Shelf` seems like the model name
    */
   function Shelf( tandem, options ) {
     options = _.extend( {

diff --git a/js/common/view/SpringControlPanel.js b/js/common/view/SpringControlPanel.js
@@ -34,8 +34,8 @@ define( function( require ) {
       titleFont: MassesAndSpringsConstants.TITLE_FONT,
       xMargin: 5,
       yMargin: 5,
-      spacing:3,
-      minWidth:165,
+      spacing: 3,
+      minWidth: 165,
       align: 'center',
       centerTick: false,
       cornerRadius: MassesAndSpringsConstants.PANEL_CORNER_RADIUS,
@@ -44,6 +44,7 @@ define( function( require ) {
       minorTickMarksVisible: true,
       sliderTrackSize: new Dimension2( 120, 0.1 ),
       tickLabelSpacing: 6,
+      // REVIEW: Just pass `constrainValue: Util.roundSymmetric`?
       constrainValue: function( value ) {
         return Util.roundSymmetric( value );
       }
@@ -94,12 +95,9 @@ define( function( require ) {
         hSlider
       ]
     } ), options );
-
   }
 
   massesAndSprings.register( 'SpringControlPanel', SpringControlPanel );
 
   return inherit( Panel, SpringControlPanel );
-
-} )
-;
+} );
diff --git a/js/common/view/SpringHangerNode.js b/js/common/view/SpringHangerNode.js
@@ -24,7 +24,7 @@ define( function( require ) {
 
   /**
    * @param {Array.<Spring>} springs
-   * @param {ModelViewTransform2} modelViewTransform2
+   * @param {ModelViewTransform2} modelViewTransform2 REVIEW: Usually just name modelViewTransform or mvt
    * @param {Tandem} tandem
    * @param {Object} options
    * @constructor
@@ -41,8 +41,8 @@ define( function( require ) {
       var springHangerNodeWidth = springsSeparation * 1.4;
 
       // X coordinate of middle of springs
-      var middleOfSprings = modelViewTransform2.modelToViewX( (springs[ 0 ].positionProperty.get().x +
-                                                               springs[ 1 ].positionProperty.get().x) / 2 );
+      var middleOfSprings = modelViewTransform2.modelToViewX( ( springs[ 0 ].positionProperty.get().x +
+                                                                springs[ 1 ].positionProperty.get().x ) / 2 );
 
       // derived from x positions of springs.
       Rectangle.call( this, 0, 0, springHangerNodeWidth, 20, 8, 8, {
-Original file line number
+Diff line change
@@ Expand Up / @@ -29,5 +29,6 @@ define( function( require ) { @@
       }
       massesAndSprings.register( 'ForceVectorArrow', ForceVectorArrow );
       return inherit( ArrowNode, ForceVectorArrow );
     } );