Some review changes, see #29

js/building/model/Group.js

@@ -52,6 +52,8 @@ define( require => {
 
       // Keep our hover target up-to-date
       // REVIEW: Does this need an unlink?
+      // REVIEW*: I don't believe so. Since we "own" the Property, any reference to this is going to also reference the
+      // REVIEW*: Property (and vice versa).
       this.hoveringTargetProperty.lazyLink( ( newTarget, oldTarget ) => {
         oldTarget && oldTarget.hoveringGroups.remove( this );
         newTarget && newTarget.hoveringGroups.push( this );

js/building/model/NumberGroup.js

@@ -154,8 +154,7 @@ define( require => {
      */
     updateAllowedSpots() {
       if ( this.isMixedNumber ) {
-        // REVIEW: 'let' instead of 'var'?
-        var range = this.activeNumberRangeProperty.value;
+        const range = this.activeNumberRangeProperty.value;
 
         this.numeratorSpot.showNotAllowedProperty.value = range === null ? false : !this.canPlaceNumberInSpot( range.min, this.numeratorSpot );
         this.denominatorSpot.showNotAllowedProperty.value = range === null ? false : !this.canPlaceNumberInSpot( range.max, this.denominatorSpot );

js/building/model/NumberSpot.js

@@ -8,11 +8,10 @@
 define( require => {
   'use strict';
 
-  //REVIEW: `const` instead of `var`?
   // modules
-  var BooleanProperty = require( 'AXON/BooleanProperty' );
-  var fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' );
-  var Property = require( 'AXON/Property' );
+  const BooleanProperty = require( 'AXON/BooleanProperty' );
+  const fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' );
+  const Property = require( 'AXON/Property' );
 
   class NumberSpot {
     /**

js/building/model/NumberStack.js

@@ -22,6 +22,7 @@ define( require => {
      */
     // REVIEW: Could you respond in #29 about this notation/syntax "isMutable = true" in the constructor arguements?
     // REVIEW: If 'isMutable' is always true why is it a parameter and not a const?
+    // REVIEW*: It's a default value for a parameter (that it gets if not provided). BuildingLabModel provides false.
     constructor( number, layoutQuantity, isMutable = true ) {
       super( BuildingType.NUMBER, layoutQuantity, isMutable );
 

js/building/model/ShapeContainer.js

@@ -20,8 +20,7 @@ define( require => {
   const Util = require( 'DOT/Util' );
   const Vector2 = require( 'DOT/Vector2' );
 
-  // REVIEW: Please add doc about how this is used.
-  const scratchVector = new Vector2();
+  const scratchVector = new Vector2(); // Used as a shared Vector2 so that we can avoid allocating vectors dynamically.
 
   class ShapeContainer {
     /**

js/building/model/ShapeGroup.js

@@ -107,8 +107,7 @@ define( require => {
      * @returns {boolean}
      */
     hasAnyPieces() {
-      // REVIEW: Are we still using 'var' in for loops?
-      for ( var i = 0; i < this.shapeContainers.length; i++ ) {
+      for ( let i = 0; i < this.shapeContainers.length; i++ ) {
         if ( this.shapeContainers.get( i ).shapePieces.length ) {
           return true;
         }
@@ -121,8 +120,7 @@ define( require => {
      * @public
      */
     increaseContainerCount() {
-      // REVIEW: 'let' instead of 'var'?
-      var offset = new Vector2( this.shapeContainers.length * ( FractionsCommonConstants.SHAPE_SIZE + FractionsCommonConstants.SHAPE_CONTAINER_PADDING ), 0 );
+      const offset = new Vector2( this.shapeContainers.length * ( FractionsCommonConstants.SHAPE_SIZE + FractionsCommonConstants.SHAPE_CONTAINER_PADDING ), 0 );
       this.shapeContainers.push( new ShapeContainer( this, this.partitionDenominatorProperty, this.representation, this.changedEmitter, offset ) );
     }
 

js/building/model/ShapePiece.js

@@ -22,7 +22,9 @@ define( require => {
   const NumberProperty = require( 'AXON/NumberProperty' );
   const Property = require( 'AXON/Property' );
   const Vector2 = require( 'DOT/Vector2' );
-// REVIEW: Added doc about how this is used would be helpful
+
+  // globals
+  // Used for unique identifiers for every ShapePiece (so we can efficiently store a map from piece ID to other objects.
   let globalID = 0;
 
   class ShapePiece {
@@ -88,6 +90,7 @@ define( require => {
       this.trueTargetRotation = 0;
 
       // REVIEW: Does this property need an unlink?
+      // REVIEW*: Shouldn't need it, since they have the same lifetimes.
       this.isUserControlledProperty.link( isUserControlled => {
         if ( isUserControlled ) {
           this.shadowProperty.value = 1;
@@ -96,16 +99,15 @@ define( require => {
 
       // Handle rotational animation towards a target (if any)
       // REVIEW: Does this need a dispose?
+      // REVIEW*: Shouldn't need it, since they have the same lifetimes.
       Property.multilink( [ this.isUserControlledProperty, this.targetRotationProperty ], ( isUserControlled, targetRotation ) => {
         if ( isUserControlled ) {
-          // REVIEW: 'let' instead of 'var'
-          var currentRotation = this.rotationProperty.value;
+          const currentRotation = this.rotationProperty.value;
           this.trueTargetRotation = Animator.modifiedEndAngle( currentRotation, this.targetRotationProperty.value );
 
-          // REVIEW: 'let' instead of 'var'
-          // REVIEW: Documentation for these variables would be helpful, being they are only used below.
-          var damping = 1;
-          var force = 50;
+          // Constants tweaked to give the damped harmonic a pleasing behavior.
+          const damping = 1;
+          const force = 50;
           this.dampedHarmonicTimeElapsed = 0;
           this.dampedHarmonic = new DampedHarmonic( 1, Math.sqrt( 4 * force ) * damping, force, currentRotation - this.trueTargetRotation, this.angularVelocityProperty.value );
         }
@@ -162,12 +164,11 @@ define( require => {
         return Vector2.ZERO;
       }
       else {
-        // REVIEW: 'let' instead of 'var'
-        var positiveAngle = fraction.value * 2 * Math.PI;
+        const positiveAngle = fraction.value * 2 * Math.PI;
 
         // Compute the centroid for a circular sector
-        var radius = FractionsCommonConstants.SHAPE_SIZE / 2;
-        var distanceFromCenter = 4 / 3 * radius * Math.sin( positiveAngle / 2 ) / positiveAngle;
+        const radius = FractionsCommonConstants.SHAPE_SIZE / 2;
+        const distanceFromCenter = 4 / 3 * radius * Math.sin( positiveAngle / 2 ) / positiveAngle;
         return Vector2.createPolar( distanceFromCenter, -positiveAngle / 2 );
       }
     }

js/building/view/NumberGroupNode.js

@@ -153,6 +153,7 @@ define( require => {
 
         // Keep the group in the drag bounds (when they change)
         // REVIEW: Does this need an unlink?
+        // REVIEW*: Shouldn't need it, these have the same lifetime.
         this.dragBoundsProperty.lazyLink( dragBounds => {
           numberGroup.positionProperty.value = dragBounds.closestPointTo( numberGroup.positionProperty.value );
         } );

js/building/view/NumberPieceNode.js

@@ -102,6 +102,7 @@ define( require => {
       } );
 
       // REVIEW: Does this need an unlink?
+      // REVIEW*: Shouldn't need it, the two objects have the same lifetime.
       this.dragListener.isUserControlledProperty.link( controlled => {
         numberPiece.isUserControlledProperty.value = controlled;
       } );

js/building/view/ShapePieceNode.js

@@ -147,6 +147,7 @@ define( require => {
         }
       } );
       // REVIEW: Does this need an unlink?
+      // REVIEW*: Shouldn't need it, the two objects have the same lifetime.
       this.dragListener.isUserControlledProperty.link( controlled => {
         shapePiece.isUserControlledProperty.value = controlled;
       } );

js/common/FractionsCommonConstants.js

@@ -8,16 +8,15 @@
 define( require => {
   'use strict';
 
-  // REVEIW: 'let' instead of 'var'
   // modules
-  var fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' );
-  var PhetFont = require( 'SCENERY_PHET/PhetFont' );
+  const fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' );
+  const PhetFont = require( 'SCENERY_PHET/PhetFont' );
 
-  //REVIEW: Missing "// constants" notation
-  var WHOLE_FRACTIONAL_SIZE_RATIO = 2;
-  var SHAPE_RADIUS = 50;
+  // constants
+  const WHOLE_FRACTIONAL_SIZE_RATIO = 2;
+  const SHAPE_RADIUS = 50;
 
-  var FractionsCommonConstants = {
+  const FractionsCommonConstants = {
     // {number}
     PANEL_MARGIN: 10,