More review notes, see #253

js/common/MassesAndSpringsConstants.js

@@ -41,7 +41,7 @@ define( function( require ) {
     GRAVITY_RANGE_PROPERTY: new Property( new RangeWithValue( 0, 30, 9.8 ) ),
 
     // {Property.<Range>} range of gravitational acceleration associated with each planet
-    //REVIEW: No property? Additionally, if you expose only a Range, no reason to use RangeWithValue instead.
+    //REVIEW: No Property? Additionally, if you expose only a Range, no reason to use RangeWithValue instead.
     SPRING_CONSTANT_RANGE: new RangeWithValue( 3, 12, 6 ),
 
     // Constants for vectors

js/common/model/Mass.js

@@ -42,7 +42,7 @@ define( function( require ) {
    * @param {number} xPosition - starting x-coordinate of the mass object, offset from the first spring position
    * @param {boolean} isLabeled: determines if the mass is labeled in the view
    * @param {string} color: color of shown mass
-   * @param {Property.<number>} gravityProperty - the gravity property from the model
+   * @param {Property.<number>} gravityProperty - the gravity property from the model REVIEW: Use capitalized Property to talk about Properties
    * @param {Tandem} tandem
    * @param {Object} [options]
    * @constructor
@@ -61,7 +61,7 @@ define( function( require ) {
       zeroReferencePoint: 0 // Height of the mass when it is resting on the shelf REVIEW: units?
     }, options );
 
-    // @public Non-property attributes
+    // @public Non-property attributes REVIEW: These are properties, use capitalized 'Property' to talk about the type.
     this.isLabeled = isLabeled;
     this.adjustable = options.adjustable;
     this.mysteryLabel = options.mysteryLabel;
@@ -196,7 +196,7 @@ define( function( require ) {
         return springForce - massValue * gravity;
       } );
 
-    // Link that sets the acceleration property of the mass
+    // Link that sets the acceleration property of the mass REVIEW: Use capitalized Property to talk about Properties
     this.netForceProperty.link( function( netForce ) {
       self.accelerationProperty.set( netForce / self.mass );
     } );
@@ -291,6 +291,7 @@ define( function( require ) {
     // As the total energy changes we can derive the thermal energy as being the energy lost from the system
     this.totalEnergyProperty.link( function( newTotalEnergy, oldTotalEnergy ) {
       if ( self.userControlledProperty.get() ) {
+        //REVIEW: Style guide wants one space after //
         //If a user is dragging the mass we remove the thermal energy.
         self.initialTotalEnergyProperty.set( newTotalEnergy );
       }

js/common/model/MassesAndSpringsModel.js

@@ -49,6 +49,7 @@ define( function( require ) {
       tandem: tandem.createTandem( 'playingProperty' )
     } );
 
+    //REVIEW: Style guide wants one space after //
     //@public {Property.<boolean>} determines whether the sim is playing sound
     //REVIEW: Looks like nothing is listening to this? Can it be removed?
     this.isSoundEnabledProperty = new BooleanProperty( true, {
@@ -127,7 +128,7 @@ define( function( require ) {
       phetioType: PropertyIO( BodyIO )
     } );
 
-    // Visibility properties of vectors associated with each mass
+    // Visibility properties of vectors associated with each mass REVIEW: Use capitalized Property to talk about Properties
     // @public {Property.<boolean>} determines the visibility of the velocity vector
     this.velocityVectorVisibilityProperty = new BooleanProperty( false, {
       tandem: tandem.createTandem( 'velocityVectorVisibilityProperty' )
@@ -206,6 +207,7 @@ define( function( require ) {
       );
       this.springs.push( spring );
 
+      //REVIEW: Use capitalized Property to talk about Properties
       // Links are used to set damping property of each spring to the damping property of the system
       //REVIEW: Additionally, why not pass this reference in directly, so direct listeners can be added?
       this.dampingProperty.link( function( newDamping ) {
@@ -337,7 +339,7 @@ define( function( require ) {
 
       // Update mass position if unattached
       else {
-
+        //REVIEW: Style guide wants one space after //
         //Attempt to attach
         //REVIEW: Presumably springs are too far apart to have a mass attach to multiple springs? If so might doc.
         this.springs.forEach( function( spring ) {

js/common/model/Spring.js

@@ -39,7 +39,7 @@ define( function( require ) {
    * @param {Vector2} position - coordinates of the top center of the spring
    * @param {number} initialNaturalRestingLength - initial resting length of unweighted spring in m
    * @param {number} defaultDampingCoefficient N.s/m - viscous damping coefficient of the system
-   * @param {Property.<number>} gravityProperty - the gravity property from the model
+   * @param {Property.<number>} gravityProperty - the gravity property from the model REVIEW: Use capitalized Property to talk about Properties
    * @param {Tandem} tandem
    * @param {Object} [options]
    * REVIEW: Options are never used? (Correct me if wrong)
@@ -229,12 +229,12 @@ define( function( require ) {
     // @public {Property.<boolean>} Responsible for the visibility of the period trace.
     //REVIEW: BooleanProperty?
     //REVIEW: Why is this not reset?
-    //REVIEW: Wait, is this property never set a value?
+    //REVIEW: Wait, is this Property never set a value?
     this.periodTraceVisibilityProperty = new Property( false );
 
     //REVIEW: NumberProperty?
     // @public {Property.<number>} y position of the equilibrium position centered on mass's center of mass
-    //REVIEW: Can we try to turn this into a derived property, with a 0 or null if there is no mass attached?
+    //REVIEW: Can we try to turn this into a derived Property, with a 0 or null if there is no mass attached?
     this.massEquilibriumYPositionProperty = new Property( 0,
       {
         tandem: tandem.createTandem( 'equilibriumYPositionProperty' ),
@@ -354,6 +354,7 @@ define( function( require ) {
     /**
      * Updates thickness of spring and sets its thickness property to calculated value. This is not handled internally
      * by the spring because the intro model determines the conditions for updating thickness.
+     * REVIEW: Use capitalized Property to talk about Properties
      * @public
      *
      * REVIEW: Type comes before the name in the type docs
@@ -372,6 +373,7 @@ define( function( require ) {
     /**
      * Updates springConstant of spring and sets its spring constant property to calculated value. This is not handled
      * internally by the spring because the intro model determines the conditions for updating spring constant.
+     * REVIEW: Use capitalized Property to talk about Properties
      * @public
      *
      * @param length {number} current natural resting length of spring
@@ -534,6 +536,7 @@ define( function( require ) {
         // Critically damped case
         else {
           //TODO::  if needed decouple these objects
+          //REVIEW: JO will handle
           var omega = Math.sqrt( k / m );
           var phi = Math.exp( dt * omega );
 

js/common/view/EnergyGraphNode.js

@@ -77,6 +77,7 @@ define( function( require ) {
       //REVIEW: Specify read-write as (read-write) AFTER the type docs. Add type docs here. Adn visibility
       // {read-write} Responsible for the zoom level in the bar graph. Is adjusted by the zoom buttons and used for the
       // scaling property of the barNodes.
+      //REVIEW: Use capitalized Property to talk about Properties
       //REVIEW: JSDoc?
       //REVIEW: NumberProperty?
       this.zoomLevelProperty = new Property( 3 );

js/common/view/GravityAndDampingControlNode.js

@@ -74,10 +74,10 @@ define( function( require ) {
       } );
     } );
 
-    // @public REVIEW: type docs, and why is this set as a property instead of a local var?
+    // @public REVIEW: type docs, and why is this set as a Property instead of a local var?
     this.gravityProperty = model.gravityProperty;
 
-    // @public REVIEW: type docs, and why is this set as a property instead of a local var?
+    // @public REVIEW: type docs, and why is this set as a Property instead of a local var?
     this.bodyProperty = model.bodyProperty;
 
     //REVIEW: Does not need visibility/read-only docs, as it's a local variable
@@ -104,6 +104,7 @@ define( function( require ) {
       stroke: null,
       sliderIndent: 7,
       constrainValue: function( value ) {
+        //REVIEW: Use Util.roundSymmetric, see https://github.com/phetsims/dot/issues/35#issuecomment-113587879
         value = Math.round( value * 100 / 3 ) * 3;
         return value / 100;
       }
@@ -202,6 +203,7 @@ define( function( require ) {
         thumbFillHighlighted: '#71EDFF',
         align: 'center',
         constrainValue: function( value ) {
+          //REVIEW: Use Util.roundSymmetric, see https://github.com/phetsims/dot/issues/35#issuecomment-113587879
           value = Math.round( value * 100 / 5.75 ) * 5.75;
           return value / 100;
         },

js/common/view/MassNode.js

@@ -204,6 +204,7 @@ define( function( require ) {
       cursor: 'pointer'
     } );
 
+    //REVIEW: Style guide wants one space after //
     //Arrows created for vectors associated with mass nodes
     //REVIEW: JSDoc
     this.velocityArrow = new VectorArrow( MassesAndSpringsConstants.VELOCITY_ARROW_COLOR );
@@ -229,6 +230,7 @@ define( function( require ) {
 
     // TODO: It looks like the below functions could be refactored into a single multilink across 7+ properties that update the below 6 visibilities
     // TODO: OR Move code into VectorArrow (or whatever the supertype for all of the arrows would be) so that you can update visibility and tail/tip using code there.
+    //REVIEW: Handle TODOs?
 
     /**
      * Show/hide the velocity and acceleration arrows when appropriate
@@ -286,6 +288,7 @@ define( function( require ) {
     // 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?
 
+    //REVIEW: Style guide wants one space after //
     //Links for handling the length of the vectors in response to the system.
     var scalingFactor = 3;
     Property.multilink( [ mass.verticalVelocityProperty, model.velocityVectorVisibilityProperty, model.accelerationVectorVisibilityProperty ], function( velocity, visible, accelerationVisible ) {

js/common/view/MovableLineNode.js

@@ -36,6 +36,7 @@ define( function( require ) {
 
     // Creates laser pointer tip for reference line
     // Laser should never have a button in this sim, but a property is needed for the LaserPointerNode to work
+    //REVIEW: Use capitalized Property to talk about Properties
     var laserEnabledProperty = new Property( false, { validValues: [ false ] } );
     var laserPointerNode = new LaserPointerNode( laserEnabledProperty, {
       bodySize: new Dimension2( 12, 14 ),

js/common/view/OneSpringScreenView.js

@@ -215,6 +215,7 @@ define( function( require ) {
 
       //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?)
+      //REVIEW: Style guide wants one space after //
       //Update the bounds of view elements
       springHangerNode.top = self.spacing;
       movableLineNode.centerX = springHangerNode.centerX;

js/common/view/OscillatingSpringNode.js

@@ -67,6 +67,7 @@ define( function( require ) {
       - ( options.leftEndLength + options.rightEndLength) );
       var xScale = coilStretch / ( self.loopsProperty.get() * self.radiusProperty.get() );
 
+      //REVIEW: Style guide wants one space after //
       //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.
@@ -84,6 +85,7 @@ define( function( require ) {
       updateViewLength();
     } );
 
+    //REVIEW: Style guide wants one space after //
     //ParametricSpringNode width update
     //SpringConstant determines lineWidth
     spring.thicknessProperty.link( function( thickness ) {

js/common/view/ReferenceLineNode.js

@@ -25,6 +25,7 @@ define( function( require ) {
    * @param {ModelViewTransform2} modelViewTransform2
    * @param {Spring} spring - spring model object
    * @param {Property} property - determines which property is being referenced REVIEW: Property of what? If it can be anything, Property.<*>
+   * REVIEW: Use capitalized Property to talk about Properties
    * @param {Property.<boolean>} visibleProperty
    * @param {Object} [options]
    *

js/common/view/SpringScreenView.js

@@ -187,7 +187,7 @@ define( function( require ) {
     //REVIEW: on the screenview and override/extend as necessary, as you are unintentionally depending on this listener
     //REVIEW: being added before the other visibleBoundsProperty listeners are added (for layout)
     this.visibleBoundsProperty.link( function( visibleBounds ) {
-
+      //REVIEW: Style guide wants one space after //
       //Update the bounds of view elements
       self.panelRightSpacing = visibleBounds.right - self.spacing;
       soundToggleButton.right = self.panelRightSpacing;

js/common/view/TwoSpringScreenView.js

@@ -65,7 +65,7 @@ define( function( require ) {
     this.secondSpringStopperButtonNode = this.createStopperButton( rightSpring, tandem );
     this.secondSpringStopperButtonNode.left = this.springHangerNode.right + this.spacing;
 
-    //REVIEW: Not sure why these two properties are linked together. This looks like it could be handled with two
+    //REVIEW: Not sure why these two Properties are linked together. This looks like it could be handled with two
     //REVIEW: separate link statements?
     Property.multilink( [ leftSpring.buttonEnabledProperty, rightSpring.buttonEnabledProperty ],
       function( leftButtonEnabled, rightButtonEnabled ) {
@@ -150,6 +150,7 @@ define( function( require ) {
     this.addChild( this.firstSpringStopperButtonNode );
     this.addChild( this.secondSpringStopperButtonNode );
 
+    //REVIEW: Style guide wants one space after //
     //Reference lines from indicator visibility box
     this.addChild( firstNaturalLengthLineNode );
     this.addChild( secondNaturalLengthLineNode );
@@ -161,7 +162,7 @@ define( function( require ) {
 
     // Adjust the floating panels to the visibleBounds of the screen.
     this.visibleBoundsProperty.link( function( visibleBounds ) {
-
+      //REVIEW: Style guide wants one space after //
       //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?)

js/energy/model/EnergyModel.js

@@ -40,7 +40,7 @@ define( function( require ) {
     reset: function() {
       MassesAndSpringsModel.prototype.reset.call( this );
 
-      //REVIEW: The more "proper" way is to pass an initial value in to the supertype that is set as the property's
+      //REVIEW: The more "proper" way is to pass an initial value in to the supertype that is set as the Property's
       //REVIEW: actual initial value. Then it can be reset as normal.
       this.dampingProperty.set( 0.2 );
     }

js/intro/model/IntroModel.js

@@ -86,6 +86,7 @@ define( function( require ) {
         // Manages stashing and applying parameters to each scene
         self.resetScene( true );
 
+        //REVIEW: Style guide wants one space after //
         //reset the spring stop buttons
         self.spring1.buttonEnabledProperty.reset();
         self.spring2.buttonEnabledProperty.reset();
@@ -102,6 +103,7 @@ define( function( require ) {
         // Manages stashing and applying parameters to each scene
         self.resetScene( true );
 
+        //REVIEW: Style guide wants one space after //
         //reset the spring stop buttons
         self.spring1.buttonEnabledProperty.reset();
         self.spring2.buttonEnabledProperty.reset();
@@ -115,6 +117,7 @@ define( function( require ) {
     // Manages logic for updating spring thickness and spring constant
     self.spring1.naturalRestingLengthProperty.link( function( naturalRestingLength ) {
 
+      //REVIEW: Style guide wants one space after //
       //Functions used to determine the inverse relationship between the length and springConstant/thickness
       //  SpringConstant = constant --> As length increases, spring thickness decreases (and vice versa)
       //  Thickness = constant --> As length increases, spring constant decreases  (and vice versa)
@@ -199,6 +202,7 @@ define( function( require ) {
       } );
 
       // We are resetting the springs' displacement property to recalculate an appropriate length (derived property)
+      //REVIEW: Use capitalized Property to talk about Properties
       this.springs.forEach( function( spring ) {
         if ( spring.massAttachedProperty.get() ) {
           spring.massAttachedProperty.reset();

js/intro/view/IntroScreenView.js

@@ -74,6 +74,7 @@ define( function( require ) {
         visible: false,
         centerTick: true,
         constrainValue: function( value ) {
+          //REVIEW: Use Util.roundSymmetric, see https://github.com/phetsims/dot/issues/35#issuecomment-113587879
           value = Math.round( value * 100 / 5 ) * 5;
           return value / 100;
         }
@@ -164,6 +165,7 @@ define( function( require ) {
     this.addChild( firstSpringEquilibriumLineNode );
     this.addChild( secondSpringEquilibriumLineNode );
 
+    //REVIEW: Style guide wants one space after //
     //We do this to prevent overlap with the massNodes.
     firstSpringEquilibriumLineNode.moveToBack();
     secondSpringEquilibriumLineNode.moveToBack();

js/vectors/view/VectorVisibilityControlNode.js

@@ -141,6 +141,7 @@ define( function( require ) {
       }
     } );
 
+    //REVIEW: Style guide wants one space after //
     //Contains all checkboxes and radio buttons for vector visibility
     var vectorVisibilityControlsVBox;
 

js/vectors/view/VectorsScreenView.js

@@ -102,10 +102,12 @@ define( function( require ) {
       }
     );
 
+    //REVIEW: Style guide wants one space after //
     //Reference lines from indicator visibility box
     this.addChild( firstSpringEquilibriumLineNode );
     this.addChild( secondSpringEquilibriumLineNode );
 
+    //REVIEW: Style guide wants one space after //
     //We do this to prevent overlap with the massNodes.
     firstSpringEquilibriumLineNode.moveToBack();
     secondSpringEquilibriumLineNode.moveToBack();