code review updates, see #19

inverse-square-law-common-strings_en.json

@@ -12,7 +12,7 @@
     "value": "cm"
   },
   "pattern_0value_1units": {
-    "value": "{0} {1}"
+    "value": "{{value}} {{units}}"
   },
   "constantRadius": {
     "value": "Constant Radius"

js/ISLQueryParameters.js

@@ -1,4 +1,4 @@
-// Copyright 2016, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * Query parameters for inverse-square-law-common

js/model/ISLCModel.js

@@ -1,4 +1,4 @@
-// Copyright 2013-2015, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * Main model for a system of two objects that exert forces on each other.
@@ -20,9 +20,6 @@ define( function( require ) {
   // phet-io modules
   var BooleanIO = require( 'ifphetio!PHET_IO/types/BooleanIO' );
 
-  // constants
-  // var DISTANCE_DECIMAL_PRECISION = 3; // limit precision so small changes are not propogated to the force
-
   /**
    * @constructor
    * @param {number} forceConstant the appropriate force constant (e.g. G or k)
@@ -69,6 +66,7 @@ define( function( require ) {
     this.stepEmitter = new Emitter();
 
     // derived property that calculates the force based on changes to values and positions
+    // objects are never destroyed, so forceProperty does not require disposal
     this.forceProperty = new DerivedProperty(
      [
        this.object1.valueProperty,
@@ -87,12 +85,12 @@ define( function( require ) {
     // thus, there is no need ot dispose of the listeners below
     this.object1.radiusProperty.link( function( radius ) {
       self.object1.radiusLastChanged = true;
-      self.toggleRadiusLastChangedObject( self.object1 );
+      self.object2.radiusLastChanged = false;
     } );
 
     this.object2.radiusProperty.link( function( radius ) {
       self.object2.radiusLastChanged = true;
-      self.toggleRadiusLastChangedObject( self.object2 );
+      self.object1.radiusLastChanged = false;
     } );
   }
 
@@ -158,6 +156,12 @@ define( function( require ) {
       this.stepEmitter.emit();
     },
 
+    /**
+     * Helper function to for accessing and mapping force ranges in the inheriting sims' views
+     *
+     * @public
+     * @return {number} the smallest possible force magnitude
+     */
     getMinForce: function () {
       var maxDistance = Math.abs( this.rightObjectBoundary - this.leftObjectBoundary );
 
@@ -169,17 +173,40 @@ define( function( require ) {
       return Math.abs( this.calculateForce( minValue, minValue, maxDistance ) );
     },
 
+    /**
+     * Helper function to for accessing and mapping force ranges in the inheriting sims' views
+     *
+     * @public
+     * @return {number} the largest possible force magnitude
+     */
     getMaxForce: function () {
       var maxValue = this.object1.valueRange.max;
       return Math.abs( this.calculateForce( maxValue, maxValue, this.getMinDistance( maxValue ) ) );
     },
 
+    /**
+     * Get the minimum possible separation between the objects' centers given a defined value for each of their
+     * main properties. 
+     *
+     * @public
+     * @param  {number} value - the object's mass or charges
+     * @return {number} the distance between the objects' centers
+     */
     getMinDistance: function( value ) {
       // calculate radius for masses and charges at maximum mass/charge
       var minRadius = this.object1.calculateRadius( value );
       return ( 2 * minRadius ) + this.minSeparationBetweenObjects;
     },
 
+    /**
+     * Helper function to calculate the force within the model
+     *
+     * @public
+     * @param  {number} v1 - the first object's mass or charge
+     * @param  {number} v2 - the second object's mass or charge
+     * @param  {number} distance - the distance between the objects' centers
+     * @return {number} the force between the two objects
+     */
     calculateForce: function( v1, v2, distance ) {
       assert && assert( distance > 0, 'must have non zero distance between objects' );
       return this.forceConstant * v1 * v2 / ( distance * distance );
@@ -200,8 +227,9 @@ define( function( require ) {
 
     /**
      * Get the absolute maximum horizontal position for an object, relative to the object's center.
-     * 
-     * @param  {} object
+     *
+     * @private
+     * @param  {ISLCObject} object
      * @return {number}
      */
     getObjectMaxPosition: function( object ) {
@@ -224,7 +252,8 @@ define( function( require ) {
 
     /**
      * Get the absolute minimum horizontal position for an object.
-     * 
+     *
+     * @private
      * @param  {ISLCObject} object 
      * @return {number}
      */
@@ -267,19 +296,6 @@ define( function( require ) {
       return snappedPosition;
     },
 
-    /**
-     * Switches between which object's radius was most recently updated. Used in the logic for updating object positions.
-     * @param  {ISLObject} object
-     * @return {void}
-     */
-    toggleRadiusLastChangedObject: function( object ) {
-      if ( object === this.object1 ) {
-        this.object2.radiusLastChanged = !this.object1.radiusLastChanged;
-      } else if ( object === this.object2 ) {
-        this.object1.radiusLastChanged = !this.object2.radiusLastChanged;
-      }
-    },
-
     // @public
     reset: function() {
       this.showValuesProperty.reset();

js/model/ISLCObject.js

@@ -1,7 +1,7 @@
 // Copyright 2017, University of Colorado Boulder
 
 /**
- * Common type for items in inverse square law sims.
+ * Common type for model items in inverse square law sims.
  *
  * @author Jesse Greenberg (PhET Interactive Simulations)
  */

js/model/Mass.js

@@ -1,4 +1,4 @@
-// Copyright 2016, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * Model for one of the spherical draggable masses.

js/view/ISLCForceArrowNode.js

@@ -26,11 +26,16 @@ define( function( require ) {
   var forceDescriptionPatternTargetSourceValueString = require( 'string!INVERSE_SQUARE_LAW_COMMON/force-description-pattern-target_source_value' );
 
   // constants
-  // TODO: these will probably have to be changed for different sims?
   var ARROW_LENGTH = 8; // empirically determined
   var TEXT_OFFSET = 5; // empirically determined to make sure text does not go out of bounds
 
-  // TODO: JSDOC
+  /**
+   * @constructor
+   * @param {Range} arrowForceRange - the range in force magnitude
+   * @param {Bounds2} layoutBounds
+   * @param {Tandem} tandem
+   * @param {Object} options
+   */
   function ISLCForceArrowNode( arrowForceRange, layoutBounds, tandem, options ) {
 
     options = _.extend( {

js/view/ISLCLegendNode.js

@@ -1,4 +1,4 @@
-// Copyright 2013-2017, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * A legend graphic consisting of a double-ended arrow, two endpoint lines, and a label string.

js/view/ISLCObjectControl.js

@@ -1,4 +1,4 @@
-// Copyright 2013-2015, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * Arrow buttons, slider and text box for editing the object value amount.
@@ -52,7 +52,7 @@ define( function( require ) {
       valueMaxWidth: VALUE_MAX_WIDTH,
 
       // Don't fill in the {0}, it will be filled in by NumberControl
-      valuePattern: StringUtils.format( pattern0Value1UnitsString, '{0}', unitString ),
+      valuePattern: StringUtils.fillIn( pattern0Value1UnitsString, { value: '{0}', units: unitString } ),
       layoutFunction: NumberControl.createLayoutFunction3( { xSpacing: 10 } ),
       minorTickSpacing: 2,
       minorTickLength: 6,

js/view/ISLCObjectNode.js

@@ -204,6 +204,8 @@ define( function( require ) {
 
     var self = this;
 
+    // on reset, no objects are destroyed and properties are set to initial values
+    // no need to dispose of any of the below listeners
     objectModel.positionProperty.link( function( property ) {
 
       // position this node and its force arrow with label
@@ -306,6 +308,9 @@ define( function( require ) {
       this.redrawForce();
     },
 
+    /**
+     * Updates the radius, arrow length & direction, force readout, and the visible puller image.
+     */
     redrawForce: function() {
       this.objectCircle.setRadius( this.modelViewTransform.modelToViewDeltaX( this.objectModel.radiusProperty.get() ) );
       this.updateGradient( this.objectModel.baseColorProperty.get() );

js/view/ISLCPullerNode.js

@@ -1,9 +1,11 @@
-// Copyright 2013-2015, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
- * puller view for massObject
+ * The pullers attached to the mass and charge objects. The node maintains a list of puller images that vary in 
+ * perceived pull effort. They are made visible and invisible to correspond to a given force value.
  *
- * @author Anton Ulyanov (Mlearner)
+ * @author Michael Barlow
+ * @author Jesse Greenberg
  */
 define( function( require ) {
   'use strict';

js/view/ISLCRulerNode.js

@@ -1,9 +1,10 @@
-// Copyright 2013-2015, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * The draggable horizontal ruler.
  *
- * @author Anton Ulyanov (Mlearner)
+ * @author Michael Barlow
+ * @author Jesse Greenberg
  */
 define( function( require ) {
   'use strict';
@@ -68,6 +69,7 @@ define( function( require ) {
     );
     this.addChild( ruler );
 
+    // ruler node is never destroyed, no listener disposal necessary
     model.rulerPositionProperty.link( function( value ) {
       ruler.center = modelViewTransform.modelToViewPosition( value );
     } );

js/view/ISLCheckboxPanel.js

@@ -1,9 +1,10 @@
-// Copyright 2013-2015, University of Colorado Boulder
+// Copyright 2017, University of Colorado Boulder
 
 /**
  * control that allows the user to show or hide the force values
  *
- * @author Anton Ulyanov (Mlearner)
+ * @author Michael Barlow
+ * @author Jesse Greenberg
  */
 define( function( require ) {
   'use strict';