Addressed REVIEWS: Responding to REVIEW comments. Passed model.option…

…s for dampingProperty value. Refactored springConstantPatternString usages.

phetsims/masses-and-springs-basics#48

js/common/model/MassesAndSpringsModel.js

@@ -38,8 +38,12 @@ define( function( require ) {
    * @constructor
    *
    * @param {Tandem} tandem
+   * @param {Object} [options]
    */
-  function MassesAndSpringsModel( tandem ) {
+  function MassesAndSpringsModel( tandem, options ) {
+    options = _.extend( {
+      damping: 0
+    }, options );
 
     // Flag used to differentiate basics and non-basics version
     this.basicsVersion = true;
@@ -54,7 +58,7 @@ define( function( require ) {
     } );
 
     // @public {Property.<number>} coefficient of damping applied to the system
-    this.dampingProperty = new NumberProperty( 0, {
+    this.dampingProperty = new NumberProperty( options.damping, {
       units: 'newtons',
       tandem: tandem.createTandem( 'dampingProperty' )
     } );

js/common/view/DraggableTimerNode.js

@@ -52,9 +52,6 @@ define( function( require ) {
       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?
-    // *REVIEW: Similarities are the use of a position property and the start/end callbacks of the MovableDragHandler.
-    // *REVIEW: I'm don't mind the duplication here because it is readable.
 
     // @private {MovableDragHandler} (read-only) handles timer node drag events
     this.timerNodeMovableDragHandler = new MovableDragHandler( this.positionProperty, {

js/common/view/SpringScreenView.js

@@ -44,7 +44,7 @@ define( function( require ) {
   var Vector2 = require( 'DOT/Vector2' );
 
   // strings
-  var springConstantString = require( 'string!MASSES_AND_SPRINGS/springConstant' );
+  var springConstantPatternString = require( 'string!MASSES_AND_SPRINGS/springConstantPattern' );
   var springStrengthString = require( 'string!MASSES_AND_SPRINGS/springStrength' );
 
   /**
@@ -316,7 +316,7 @@ define( function( require ) {
 
       // Additional options for compatibility with Masses and Springs: Basics
       options = _.extend( {
-        string: this.model.basicsVersion ? springStrengthString : springConstantString,
+        string: this.model.basicsVersion ? springStrengthString : springConstantPatternString,
         sliderTrackSize: this.model.basicsVersion ? new Dimension2( 140, 0.1 ) : new Dimension2( 120, 0.1 ),
         yMargin: this.model.basicsVersion ? 7 : 5,
         spacing: this.model.basicsVersion ? 5 : 3,

js/energy/model/EnergyModel.js

@@ -14,20 +14,19 @@ define( function( require ) {
   var MassesAndSpringsColorProfile = require( 'MASSES_AND_SPRINGS/common/view/MassesAndSpringsColorProfile' );
   var MassesAndSpringsConstants = require( 'MASSES_AND_SPRINGS/common/MassesAndSpringsConstants' );
   var MassesAndSpringsModel = require( 'MASSES_AND_SPRINGS/common/model/MassesAndSpringsModel' );
-  var NumberProperty = require( 'AXON/NumberProperty' );
 
   /**
    * @param {Tandem} tandem
    *
    * @constructor
    */
   function EnergyModel( tandem ) {
-    MassesAndSpringsModel.call( this, tandem );
+    MassesAndSpringsModel.call( this, tandem, {
+      damping: 0.0575     // Energy screen should have spring damping
+    } );
     this.basicsVersion = false;
 
     // Energy screen should have spring damping
-    // REVIEW: This looks like we should pass in the initial value of damping, instead of overwriting the Property.
-    this.dampingProperty = new NumberProperty( 0.0575 );
 
     // Creation of masses and springs specific for this screen
     this.createSpring( MassesAndSpringsConstants.SPRING_X, tandem.createTandem( 'spring' ) );

js/intro/view/ConstantsControlPanel.js

@@ -15,7 +15,6 @@ define( function( require ) {
   var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' );
   var MassesAndSpringsConstants = require( 'MASSES_AND_SPRINGS/common/MassesAndSpringsConstants' );
   var Node = require( 'SCENERY/nodes/Node' );
-  var StringUtils = require( 'PHETCOMMON/util/StringUtils' );
   var Text = require( 'SCENERY/nodes/Text' );
   var VBox = require( 'SCENERY/nodes/VBox' );
 
@@ -58,13 +57,10 @@ define( function( require ) {
         tandem: tandem.createTandem( 'thicknessRadioButton' )
       } );
 
-    var constantText = new Text(
-      // REVIEW: Why the fill in with an empty string? Looks suspicious
-      // *REVIEW: SpringConstant is a string that is used elsewhere but has a placeholder.
-      // *REVIEW: Alternatively, I can create a springConstantString without a placeholder.
-      StringUtils.fillIn( springConstantString, { spring: '' } ),
-      _.extend( { font: TITLE_FONT, tandem: tandem.createTandem( 'constantText' ) },
-        constantsSelectionButtonOptions ) );
+    var constantText = new Text( springConstantString, {
+      font: TITLE_FONT,
+      tandem: tandem.createTandem( 'constantText' )
+    } );
     var springConstantRadioButton = new AquaRadioButton(
       selectedConstantProperty, constantEnumeration.SPRING_CONSTANT, constantText, {
         radius: MassesAndSpringsConstants.RADIO_BUTTON_RADIUS,

js/lab/model/LabModel.js

@@ -61,9 +61,7 @@ define( function( require ) {
       } );
     }
 
-    // Initialize masses for non-basics version
-    // REVIEW: This code seems to be running for both versions. Is that correct?
-    // *REVIEW: Both sims uses the same mass. I'm adjusting the massValue and density based on the sim. So this should run for both sims.
+    // Initialize masses
     massValue = this.basicsVersion ? 0.12 : 0.23;
     color = this.basicsVersion ? new Color( 9, 19, 174 ) : new Color( 0, 222, 224 );
     this.createMass( massValue, massXPosition + MASS_OFFSET * 1.5, new Property( color ), null, tandem.createTandem( 'mediumMysteryMass' ), {

js/lab/view/PeriodTraceNode.js

@@ -33,6 +33,8 @@ define( function( require ) {
 
     // @public {PeriodTrace} Model element for the period trace
     this.periodTrace = periodTrace;
+
+    // @public {Shape}
     this.shape = new Shape();
 
     // @private {number} The opacity of the trace (not using Node opacity for performance reasons)
@@ -93,8 +95,7 @@ define( function( require ) {
           var secondPeakYPosition = modelViewTransform.modelToViewY( massEquilibrium + this.periodTrace.secondPeakY );
           var currentYPosition = modelViewTransform.modelToViewY( massEquilibrium +
                                                                   spring.massEquilibriumDisplacementProperty.value );
-          // REVIEW: Why setting this here? JSDoc it also, and maybe declare in constructor IF NEEDED.
-          // *REVIEW: The shape needs to be reset here? Thoughts on best way to do this?
+
           this.shape = new Shape();
 
           var state = this.periodTrace.stateProperty.value; // 0 to 4

js/vectors/view/VectorVisibilityControlNode.js

@@ -72,45 +72,41 @@ define( function( require ) {
       tandem: tandem.createTandem( 'accelerationString' )
     } ), { group: alignGroup, xAlign: 'left' } );
 
+    // Handle options for checkbox group
     var vectorVisibilityCheckboxGroup;
+    var velocityCheckboxObject = {
+      node: new HBox( { children: [ velocityAlignBox, velocityArrow ], spacing: CONTENT_SPACING } ),
+      property: model.velocityVectorVisibilityProperty,
+      label: velocityString
+    };
+    var accelerationCheckboxObject = {
+      node: new HBox( { children: [ accelerationAlignBox, accelerationArrow ], spacing: CONTENT_SPACING } ),
+      property: model.accelerationVectorVisibilityProperty,
+      label: accelerationString
+    };
 
     if ( !model.basicsVersion ) {
-      vectorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ {
-        node: new HBox( { children: [ velocityAlignBox, velocityArrow ], spacing: CONTENT_SPACING } ),
-        property: model.velocityVectorVisibilityProperty,
-        label: velocityString
-      }, {
-        node: new HBox( { children: [ accelerationAlignBox, accelerationArrow ], spacing: CONTENT_SPACING } ),
-        property: model.accelerationVectorVisibilityProperty,
-        label: accelerationString
-      } ], {
+      vectorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ velocityCheckboxObject, accelerationCheckboxObject ], {
         checkboxOptions: {
           boxWidth: 16,
           spacing: 8
         },
         tandem: tandem.createTandem( 'vectorVisibilityCheckboxGroup' )
       } );
     }
+
     // Responsible for velocity and acceleration vectors checkboxes and period trace in basics version
     else {
-      // REVIEW: Looks like some duplication here, where there is just an added title. Can we reduce that by
-      // REVIEW: conditionally adding the node?
       vectorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ {
         node: new Text( periodTraceString, {
           font: MassesAndSpringsConstants.TITLE_FONT,
           maxWidth: MAX_WIDTH,
           tandem: tandem.createTandem( 'periodTraceString' )
         } ),
         property: model.firstSpring.periodTraceVisibilityProperty
-      }, {
-        node: new HBox( { children: [ velocityAlignBox, velocityArrow ], spacing: CONTENT_SPACING } ),
-        property: model.velocityVectorVisibilityProperty,
-        label: velocityString
-      }, {
-        node: new HBox( { children: [ accelerationAlignBox, accelerationArrow ], spacing: CONTENT_SPACING } ),
-        property: model.accelerationVectorVisibilityProperty,
-        label: accelerationString
-      }
+      },
+        velocityCheckboxObject,
+        accelerationCheckboxObject
       ], {
         checkboxOptions: {
           boxWidth: 16,
@@ -148,7 +144,7 @@ define( function( require ) {
       tandem: tandem.createTandem( 'forcesVisibilityCheckboxGroup' )
     } );
 
-    // responsible for forces aquaRadioButton
+    // Responsible for forces aquaRadioButton
     var forcesVisibilityRadioButton = new AquaRadioButton(
       model.forcesModeProperty,
       model.forcesModeChoice.FORCES,

masses-and-springs-strings_en.json

@@ -135,6 +135,9 @@
     "value": " Spring Strength {{spring}}"
   },
   "springConstant": {
+    "value": "Spring Constant"
+  },
+  "springConstantPattern": {
     "value": "Spring Constant {{spring}}"
   },
   "length": {