Added REVIEW comments #29

phetsims · Dec 21, 2018 · 3e0ddac · 3e0ddac
1 parent 95ae2ea
commit 3e0ddac
Show file tree

Hide file tree

Showing 5 changed files with 8 additions and 1 deletion.
diff --git a/js/building/model/BuildingModel.js b/js/building/model/BuildingModel.js
@@ -68,6 +68,7 @@ define( require => {
       // @public {Property.<Group|null>} - We'll only show controls for this group
       this.selectedGroupProperty = new Property( null );
 
+      // REVIEW: Is this an unused variable?
       // @public {EnumerationMap.<Array.<Stack>>} - The stacks for pieces
       this.stacksMap = new EnumerationMap( BuildingType, type => ( {
         [ BuildingType.SHAPE ]: this.shapeStacks,
@@ -232,6 +233,7 @@ define( require => {
       } );
     }
 
+    // REVIEW: Incomplete JSDoc for parameters
     /**
      * Places a ShapePiece into a ShapeContainer.
      * @public
@@ -389,6 +391,7 @@ define( require => {
       throw new Error( 'Could not find a piece to remove' );
     }
 
+    // REVIEW: JSDoc parameter mismatch
     /**
      * Removes the last piece from a NumberGroup (animating it back to its home stack).
      * @public
@@ -604,6 +607,7 @@ define( require => {
 
         // Don't compute the closest for ALL pieces, that would hurt performance.
         if ( shapePiece.representation === BuildingRepresentation.PIE && shapePiece.isUserControlledProperty.value ) {
+          // REVIEW: 'let' instead of 'var'
           var closestContainer = this.closestDroppableShapeContainer( shapePiece, Number.POSITIVE_INFINITY );
           if ( closestContainer ) {
             shapePiece.orientTowardsContainer( closestContainer, dt );

diff --git a/js/building/model/NumberGroup.js b/js/building/model/NumberGroup.js
@@ -55,6 +55,7 @@ define( require => {
      */
     constructor( isMixedNumber, options ) {
       options = _.extend( {
+
         // {Property.<Range|null>}
         activeNumberRangeProperty: new Property( null )
       }, options );

diff --git a/js/building/model/NumberPiece.js b/js/building/model/NumberPiece.js
@@ -24,7 +24,6 @@ define( require => {
   const NUMBER_SINGLE_DIGIT_WIDTH = 54;
   const NUMBER_DOUBLE_DIGIT_WIDTH = 80;
 
-
   class NumberPiece {
     /**
      * @param {number} number

diff --git a/js/building/model/ShapeGroup.js b/js/building/model/ShapeGroup.js
@@ -107,6 +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++ ) {
         if ( this.shapeContainers.get( i ).shapePieces.length ) {
           return true;
@@ -120,6 +121,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 );
       this.shapeContainers.push( new ShapeContainer( this, this.partitionDenominatorProperty, this.representation, this.changedEmitter, offset ) );
     }

diff --git a/js/game/model/FractionChallenge.js b/js/game/model/FractionChallenge.js
@@ -393,6 +393,7 @@ define( require => {
       const group = target.groupProperty.value;
 
       if ( group ) {
+
         // If the group hasn't fully completed its animation, then force it to complete early.
         group.animator.endAnimation();