Address review comments, see #247

js/systems/model/EnergySystemElementCarousel.js

@@ -183,7 +183,6 @@ define( require => {
       } );
     }
 
-    //REVIEW #247 document
     /**
      * whether the current selected element is in its destination spot
      * @private

js/systems/model/EnergyUser.js

@@ -36,9 +36,8 @@ define( require => {
       this.incomingEnergyChunks = _.union( this.incomingEnergyChunks, energyChunks );
     }
 
-    //REVIEW #247 this was @protected in the superclass, is @public accurate?
     /**
-     * @public
+     * @protected
      * @override
      */
     clearEnergyChunks() {

js/systems/model/Fan.js

@@ -94,12 +94,10 @@ define( require => {
       this.targetVelocity = 0;
     }
 
-    //REVIEW #247 step appears nowhere in the class hierarchy, why is this an override?
     /**
      * @param {number} dt - time step, in seconds
      * @param {Energy} incomingEnergy
      * @public
-     * @override
      */
     step( dt, incomingEnergy ) {
       if ( !this.activeProperty.value ) {
@@ -181,8 +179,9 @@ define( require => {
       this.bladePositionProperty.set( newAngle );
     }
 
-    //REVIEW #247 document
     /**
+     * move electrical energy chunks through the fan's wire
+     *
      * @param  {number} dt - time step, in seconds
      * @private
      */
@@ -210,7 +209,7 @@ define( require => {
 
             // release the energy chunk as mechanical to blow away
             this.mechanicalEnergyChunkMovers.push( new EnergyChunkPathMover( mover.energyChunk,
-              this.createBlownEnergyChunkPath( mover.energyChunk.positionProperty.get() ),
+              createBlownEnergyChunkPath( mover.energyChunk.positionProperty.get() ),
               EFACConstants.ENERGY_CHUNK_VELOCITY ) );
           }
           else {
@@ -230,8 +229,9 @@ define( require => {
       } );
     }
 
-    //REVIEW #247 document
     /**
+     * move thermal energy chunks up and away from the fan
+     *
      * @param  {number} dt - time step, in seconds
      * @private
      */
@@ -250,6 +250,8 @@ define( require => {
     }
 
     /**
+     * move mechanical energy chunks out of the motor and away from the blades as wind
+     *
      * @param  {number} dt - time step, in seconds
      * @private
      */
@@ -267,33 +269,6 @@ define( require => {
       } );
     }
 
-    //REVIEW #247 function can be private, it has no dependencies on Fan
-    /**
-     * create a path for chunks to follow when blown out of the fan.
-     * @param  {Vector2} startingPoint
-     * @returns {Vector2[]}
-     * @private
-     */
-    createBlownEnergyChunkPath( startingPoint ) {
-      const path = [];
-      const numberOfDirectionChanges = 20; // empirically determined
-      const nominalTravelVector = new Vector2( BLOWN_ENERGY_CHUNK_TRAVEL_DISTANCE / numberOfDirectionChanges, 0 );
-
-      // The first point is straight right the starting point.  This is done because it makes the chunk
-      // move straight out of the fan center cone.
-      let currentPosition = startingPoint.plus( new Vector2( INSIDE_FAN_ENERGY_CHUNK_TRAVEL_DISTANCE, 0 ) );
-      path.push( currentPosition );
-
-      // add the remaining points in the path
-      for ( let i = 0; i < numberOfDirectionChanges - 1; i++ ) {
-        const movement = nominalTravelVector.rotated( ( phet.joist.random.nextDouble() - 0.5 ) * Math.PI / 4 );
-        currentPosition = currentPosition.plus( movement );
-        path.push( currentPosition );
-      }
-
-      return path;
-    }
-
     /**
      * deactivate this energy system element
      * @public
@@ -384,6 +359,32 @@ define( require => {
     }
   }
 
+  /**
+   * create a path for chunks to follow when blown out of the fan.
+   * @param  {Vector2} startingPoint
+   * @returns {Vector2[]}
+   * @private
+   */
+  const createBlownEnergyChunkPath = startingPoint => {
+    const path = [];
+    const numberOfDirectionChanges = 20; // empirically determined
+    const nominalTravelVector = new Vector2( BLOWN_ENERGY_CHUNK_TRAVEL_DISTANCE / numberOfDirectionChanges, 0 );
+
+    // The first point is straight right the starting point.  This is done because it makes the chunk
+    // move straight out of the fan center cone.
+    let currentPosition = startingPoint.plus( new Vector2( INSIDE_FAN_ENERGY_CHUNK_TRAVEL_DISTANCE, 0 ) );
+    path.push( currentPosition );
+
+    // add the remaining points in the path
+    for ( let i = 0; i < numberOfDirectionChanges - 1; i++ ) {
+      const movement = nominalTravelVector.rotated( ( phet.joist.random.nextDouble() - 0.5 ) * Math.PI / 4 );
+      currentPosition = currentPosition.plus( movement );
+      path.push( currentPosition );
+    }
+
+    return path;
+  };
+
   return energyFormsAndChanges.register( 'Fan', Fan );
 } );
 

js/systems/model/FaucetAndWater.js

@@ -105,8 +105,9 @@ define( require => {
       return new EnergyChunk( EnergyType.MECHANICAL, initialPosition, velocity, this.energyChunksVisibleProperty );
     }
 
-    //REVIEW #247 document
     /**
+     * if enough energy has been produced since the last energy chunk was emitted, release another one into the system
+     *
      * @private
      */
     addChunkIfEnoughEnergy() {
@@ -117,13 +118,11 @@ define( require => {
       }
     }
 
-    //REVIEW #247 why is this an override? no step in class hierarchy
     /**
      * step in time
      * @param  {number} dt time step, in seconds
      * @returns {Energy}
      * @public
-     * @override
      */
     step( dt ) {
 

js/systems/model/Generator.js

@@ -145,14 +145,12 @@ define( require => {
       }
     }
 
-    //REVIEW #247 step appears nowhere in the class hierarchy, why is this an override?
     /**
      * step this model element in time
      * @param {number} dt time step
      * @param {Energy} incomingEnergy
      * @returns {Energy}
      * @public
-     * @override
      */
     step( dt, incomingEnergy ) {
       if ( this.activeProperty.value ) {

js/systems/model/LightBulb.js

@@ -75,12 +75,10 @@ define( require => {
       this.goRightNextTime = true;
     }
 
-    //REVIEW #247 step appears nowhere in the class hierarchy, why is this an override?
     /**
      * @param  {number} dt - time step, in seconds
      * @param  {Energy} incomingEnergy
      * @public
-     * @override
      */
     step( dt, incomingEnergy ) {
       if ( this.activeProperty.value ) {
@@ -219,8 +217,8 @@ define( require => {
           if ( this.hasFilament ) {
             mover.energyChunk.energyTypeProperty.set( EnergyType.THERMAL );
             const path = this.createPathOnFilament( mover.energyChunk.positionProperty.value );
-            const speed = this.getTotalPathLength( mover.energyChunk.positionProperty.value, path ) /
-                          this.generateThermalChunkTimeOnFilament();
+            const speed = getTotalPathLength( mover.energyChunk.positionProperty.value, path ) /
+                          generateThermalChunkTimeOnFilament();
             this.filamentEnergyChunkMovers.push( new EnergyChunkPathMover( mover.energyChunk, path, speed ) );
           }
           else {
@@ -326,36 +324,6 @@ define( require => {
       return path;
     }
 
-    //REVIEW #247 can be a private function, has no dependencies on LightBulb
-    /**
-     * @returns {number} time
-     * @private
-     */
-    generateThermalChunkTimeOnFilament() {
-      return THERMAL_ENERGY_CHUNK_TIME_ON_FILAMENT.min +
-             phet.joist.random.nextDouble() * THERMAL_ENERGY_CHUNK_TIME_ON_FILAMENT.getLength();
-    }
-
-    //REVIEW #247 can be a private function, has no dependencies on LightBulb
-    /**
-     * @param {Vector2} startingLocation
-     * @param {Vector2[]} pathPoints
-     * @returns {number}
-     * @private
-     */
-    getTotalPathLength( startingLocation, pathPoints ) {
-      if ( pathPoints.length === 0 ) {
-        return 0;
-      }
-
-      let pathLength = startingLocation.distance( pathPoints[ 0 ] );
-      for ( let i = 0; i < pathPoints.length - 1; i++ ) {
-        pathLength += pathPoints[ i ].distance( pathPoints[ i + 1 ] );
-      }
-
-      return pathLength;
-    }
-
     /**
      * deactivate the light bulb
      * @public
@@ -379,6 +347,34 @@ define( require => {
     }
   }
 
+  /**
+   * @returns {number} time
+   * @private
+   */
+  const generateThermalChunkTimeOnFilament = () => {
+    return THERMAL_ENERGY_CHUNK_TIME_ON_FILAMENT.min +
+           phet.joist.random.nextDouble() * THERMAL_ENERGY_CHUNK_TIME_ON_FILAMENT.getLength();
+  };
+
+  /**
+   * @param {Vector2} startingLocation
+   * @param {Vector2[]} pathPoints
+   * @returns {number}
+   * @private
+   */
+  const getTotalPathLength = ( startingLocation, pathPoints ) => {
+    if ( pathPoints.length === 0 ) {
+      return 0;
+    }
+
+    let pathLength = startingLocation.distance( pathPoints[ 0 ] );
+    for ( let i = 0; i < pathPoints.length - 1; i++ ) {
+      pathLength += pathPoints[ i ].distance( pathPoints[ i + 1 ] );
+    }
+
+    return pathLength;
+  };
+
   return energyFormsAndChanges.register( 'LightBulb', LightBulb );
 } );
 

js/systems/model/SolarPanel.js

@@ -102,12 +102,10 @@ define( require => {
       } );
     }
 
-    //REVIEW #247 step appears nowhere in the class hierarchy, why is this an override?
     /**
      * @param  {number} dt - time step, in seconds
      * @param  {Energy} incomingEnergy - type, amount, direction of energy
      * @returns {Energy}
-     * @override
      * @public
      */
     step( dt, incomingEnergy ) {
@@ -402,10 +400,10 @@ define( require => {
       this.latestChunkArrivalTime = 0;
     }
 
-    //REVIEW #247 missing visibility annotation
     /**
      * get the shape of the area where light can be absorbed
      * @returns {Shape}
+     * @public
      */
     getAbsorptionShape() {
       return this.absorptionShape;

js/systems/model/SunEnergySource.js

@@ -99,11 +99,11 @@ define( require => {
       } );
     }
 
-    //REVIEW #247 missing visibility annotation
     /**
      * step in time
      * @param dt - time step, in seconds
      * @returns {Energy}
+     * @public
      */
     step( dt ) {
       let energyProduced = 0;
@@ -243,10 +243,10 @@ define( require => {
       this.outgoingEnergyChunks.length = 0;
     }
 
-    //REVIEW #247 missing visibility annotation
     /**
      * return a structure containing type, rate, and direction of emitted energy
      * @returns {Energy}
+     * @public
      */
     getEnergyOutputRate() {
       return new Energy(