More cleanup and review notes, see #88

js/common/enum/AllowedRepresentationsEnum.js

@@ -11,6 +11,7 @@ define( function( require ) {
   // modules
   var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' );
 
+  //REVIEW: Rename to AllowedRepresentation? Other enumerations are not named
   var AllowedRepresentationsEnum = {
     COINS_ONLY: 'COINS_ONLY',
     VARIABLES_ONLY: 'VARIABLES_ONLY',

js/common/view/ExpressionExplorationScreenView.js

@@ -312,6 +312,7 @@ define( function( require ) {
     this.visibleBoundsProperty.link( function( visibleBounds ) {
 
       // update the positions of the floating controls
+      // REVIEW: use bounds.left/bounds.right
       totalValueAccordionBox.left = visibleBounds.minX + FLOATING_PANEL_INSET;
       variableValuesAccordionBox.left = visibleBounds.minX + FLOATING_PANEL_INSET;
       myCollectionAccordionBox.right = visibleBounds.maxX - FLOATING_PANEL_INSET;
@@ -321,6 +322,7 @@ define( function( require ) {
       if ( showSubtractionCheckbox ) {
         showSubtractionCheckbox.left = myCollectionAccordionBox.left;
       }
+      //REVIEW: bounds.right
       resetAllButton.right = visibleBounds.maxX - FLOATING_PANEL_INSET;
     } );
 

js/common/view/ExpressionHintNode.js

@@ -60,11 +60,12 @@ define( function( require ) {
           leftHalfWidth = anchorCTBounds.width + 2 * INSET;
           rightHalfWidth = movingCTBounds.width + 2 * INSET;
           leftHalfCenterX = expressionHint.anchorCoinTerm.positionProperty.get().x +
-                            ( anchorCTBounds.minX + anchorCTBounds.maxX ) / 2;
+                            ( anchorCTBounds.minX + anchorCTBounds.maxX ) / 2; //REVIEW: bounds.centerX?
         }
         else { // anchor coin term is on the right
           leftHalfWidth = movingCTBounds.width + 2 * INSET;
           rightHalfWidth = anchorCTBounds.width + 2 * INSET;
+          //REVIEW: use bounds.centerX
           leftHalfCenterX = expressionHint.anchorCoinTerm.positionProperty.get().x + ( anchorCTBounds.minX + anchorCTBounds.maxX ) / 2 - anchorCTBounds.width / 2 - INSET - movingCTBounds.width / 2 - INSET;
         }
 

js/common/view/ExpressionManipulationView.js

@@ -65,6 +65,7 @@ define( function( require ) {
     model.collectionAreas.forEach( function( collectionArea ) {
       var ejectButton = new UndoButton( {
         listener: function() { collectionArea.ejectCollectedItem(); },
+        //REVIEW: leftTop: collectionArea.bounds.leftTop
         left: collectionArea.bounds.minX,
         top: collectionArea.bounds.minY
       } );
@@ -104,6 +105,7 @@ define( function( require ) {
 
     // define a function that will update the shape of the barrier rectangle
     function updateBarrierRectangle() {
+      //REVIEW: barrierRectangleShape = Shape.bounds( barrierRectangleBounds )
       barrierRectangleShape = Shape.rect(
         barrierRectangleBounds.minX,
         barrierRectangleBounds.minY,
@@ -117,6 +119,7 @@ define( function( require ) {
         barrierRectangleShape.lineTo( barrierRectangleHoleBounds.minX, barrierRectangleHoleBounds.maxY );
         barrierRectangleShape.lineTo( barrierRectangleHoleBounds.maxX, barrierRectangleHoleBounds.maxY );
         barrierRectangleShape.lineTo( barrierRectangleHoleBounds.maxX, barrierRectangleHoleBounds.minY );
+        //REVIEW: Why the moveTo before the close? The close would now do nothing, and could be removed
         barrierRectangleShape.moveTo( barrierRectangleHoleBounds.minX, barrierRectangleHoleBounds.minY );
         barrierRectangleShape.close();
       }

js/game/model/EEGameLevelModel.js

@@ -3,6 +3,9 @@
 /**
  * model for a single level of the Expression Exchange game
  *
+ * REVIEW: Why the 'Model' suffix? Usually would just be EEGameLevel
+ * (thought this was the main model for the screen).
+ *
  * @author John Blanco
  */
 define( function( require ) {
@@ -25,7 +28,7 @@ define( function( require ) {
   var NUM_EXPRESSION_COLLECTION_AREAS = 3;
 
   /**
-   * @param {number} level
+   * @param {number} level REVIEW: levelNumber? This object is the level!
    * @param {AllowedRepresentationsEnum} allowedRepresentations
    * @param {Property.<boolean>} soundEnabledProperty
    * @constructor

js/game/model/EEGameModel.js

@@ -41,6 +41,7 @@ define( function( require ) {
 
     // @public, currently selected level, null indicates no level selected which means that the level selection screen
     // should be shown in the view
+    //REVIEW: Why is this not just a property of the EEGameLevelModel objects? Should simplify a lot of code
     this.currentLevelProperty = new Property( null );
 
     //------------------------------------------------------------------------
@@ -50,7 +51,7 @@ define( function( require ) {
     // shuffle the challenge descriptors before creating the levels
     EEChallengeDescriptors.shuffleChallenges();
 
-    // @public (read-only) - models for each of the game levels
+    // @public (read-only) - models for each of the game levels REVIEW: gameLevels is a better name?
     this.gameLevelModels = [];
     _.times( NUMBER_OF_LEVELS, function( level ) {
       self.gameLevelModels.push( new EEGameLevelModel(
@@ -100,6 +101,9 @@ define( function( require ) {
       },
 
       // @public
+      //REVIEW: docs?
+      //REVIEW: This is done right after creating the views for each level. Can we just pass it as part of the view
+      // construction?
       setCoinTermRetrievalBounds: function( bounds ) {
         this.gameLevelModels.forEach( function( gameLevelModel ) {
           gameLevelModel.setCoinTermRetrievalBounds( bounds );

js/game/view/EEGameLevelView.js

@@ -38,10 +38,10 @@ define( function( require ) {
    * @param {EEGameLevelModel} levelModel
    * @param {Bounds2} screenLayoutBounds
    * @param {Property.<Bounds2>} visibleBoundsProperty
-   * @param {function} nextLevelFunction - function called when user hits 'next' button
+   * @param {Function} nextLevelFunction - function called when user hits 'next' button REVIEW callback param docs
    * @param {Property.<boolean>} allLevelsCompletedProperty - property that indicates when all levels are successfully
-   * completed
-   * @param {function} returnToLevelSelectionFunction
+   *                                                          completed
+   * @param {Function} returnToLevelSelectionFunction REVIEW: callback param docs
    * @constructor
    */
   function EEGameLevelView( levelModel,
@@ -52,16 +52,19 @@ define( function( require ) {
                             returnToLevelSelectionFunction ) {
 
     var self = this;
+
     Node.call( this );
 
-    // @public {boolean} - a property that is externally set to true when this node is in the view port and available
-    // for interaction with the user.  This is done in the view rather than in the model because the model has no
-    // awareness of the slide in/out animations, and clocking the reward node during those animations caused
+    // @public {Property.<boolean>} - a property that is externally set to true when this node is in the view port and
+    // available for interaction with the user.  This is done in the view rather than in the model because the model has
+    // no awareness of the slide in/out animations, and clocking the reward node during those animations caused
     // performance issues, and this property can be used to only clock when this is in the viewport.
     this.inViewportProperty = new Property( false );
 
     // add an invisible background rectangle so that bounds are correct
+    //REVIEW: Shouldn't need a reference to this with proper layering?
     var background = new Rectangle( screenLayoutBounds, {
+      //REVIEW: if for debugging, why still here?
       stroke: 'rgba( 0, 0, 200, 0.01 )' // increase opacity to make the outline visible if desired (for debugging)
     } );
     this.addChild( background );
@@ -76,6 +79,8 @@ define( function( require ) {
 
     // add the back button
     var backButton = new BackButton( {
+      //REVIEW: easier to read with screenLayoutBounds.left and screenLayoutBounds.top
+      //REVIEW: Or leftTop: screenLayoutBounds.eroded( 30 ).leftTop
       left: screenLayoutBounds.minX + 30,
       top: screenLayoutBounds.minY + 30,
       listener: returnToLevelSelectionFunction
@@ -114,6 +119,7 @@ define( function( require ) {
         { centerX: title.centerX, bottom: screenLayoutBounds.bottom - 40 }
       );
       self.addChild( coinTermCreatorBox );
+      //REVIEW: Seems like an anti-pattern to moveToBack here. Use layers instead?
       coinTermCreatorBox.moveToBack(); // needs to be behind coin term and other layers...
       background.moveToBack(); // ...except for the background
 
@@ -122,11 +128,13 @@ define( function( require ) {
     } );
 
     // add the check box that allows expressions with negative values to be simplified
+    //REVIEW: Third collection area seems easy to break in future refactoring. Pick last collection area instead?
     var boundsOfLowestCollectionArea = levelModel.collectionAreas[ 2 ].bounds;
     var showSubtractionCheckbox = new CheckBox(
       new ShowSubtractionIcon(),
       levelModel.simplifyNegativesProperty,
       {
+        //REVIEW: for readability, use bounds.left and bounds.bottom?
         left: boundsOfLowestCollectionArea.minX,
         top: boundsOfLowestCollectionArea.maxY + 20,
         maxWidth: boundsOfLowestCollectionArea.minX
@@ -161,6 +169,7 @@ define( function( require ) {
     this.addChild( expressionManipulationView );
 
     // hook up the audio player for playing a correct answer
+    //REVIEW: Presumably this one is used, the one in EEGameScreenView is not?
     var gameAudioPlayer = new GameAudioPlayer( levelModel.soundEnabledProperty );
     levelModel.scoreProperty.link( function( newScore, oldScore ) {
 
@@ -171,11 +180,12 @@ define( function( require ) {
     } );
 
     // control the visibility of the reward node
+    //REVIEW: Can we create this on startup, so the extra logic isn't needed?
     function createRewardNode() {
       if ( !self.rewardNode ) {
         self.rewardNode = new EERewardNode();
         background.addChild( self.rewardNode );
-        self.rewardNode.moveToBack();
+        self.rewardNode.moveToBack(); //REVIEW: Why immediately move to back? why not insert it at the correct place?
       }
     }
 
@@ -211,10 +221,11 @@ define( function( require ) {
 
   return inherit( Node, EEGameLevelView, {
 
-
     // @public
+    //REVIEW: docs?
     step: function( dt ) {
       if ( this.inViewportProperty.get() && this.rewardNode && this.rewardNode.visible ) {
+        //REVIEW: Generally preferred to cap DT at the top level. Is the cap only needed for RewardNode?
         this.rewardNode.step( Math.min( dt, 1 ) );
       }
     },

js/game/view/EEGameScreenView.js

@@ -29,25 +29,34 @@ define( function( require ) {
   function EEGameScreenView( gameModel ) {
 
     var self = this;
+    //REVIEW: visibility and type docs
     this.gameModel = gameModel;
     ScreenView.call( this, { layoutBounds: EESharedConstants.LAYOUT_BOUNDS } );
 
     // hook up the audio player to the sound settings
+    //REVIEW: visibility docs
+    //REVIEW: Wait, this isn't used? EEGameLevelView uses a different instance of GameAudioPlayer?
     this.gameAudioPlayer = new GameAudioPlayer( gameModel.soundEnabledProperty );
 
     // consolidate the level scores into an array for the level selection node
+    //REVIEW: just pass in the levels themselves, this is unnecessary
     var levelScoreProperties = [];
     gameModel.gameLevelModels.forEach( function( gameLevelModel ) {
       levelScoreProperties.push( gameLevelModel.scoreProperty );
     } );
 
     // create the icons used on the level selection buttons
+    //REVIEW: just pass in the levels themselves, this is unnecessary
     var levelSelectionButtonIcons = [];
     _.times( EEGameModel.NUMBER_OF_LEVELS, function( level ) {
       levelSelectionButtonIcons.push( EEGameLevelIconFactory.createIcon( level ) );
     } );
 
     // add the node that allows the user to choose a game level to play
+    //REVIEW: Heavily prefer passing in the level models directly
+    //REVIEW: Then you don't need to SEPARATE out the functions, icons and scores, and then combine them in the other type.
+    //REVIEW: Name change to LevelSelectionNode might be clearer here as a type?
+    //REVIEW: type/visibility docs
     this.levelSelectionNode = new StartGameLevelNode(
       function( level ) { gameModel.selectLevel( level ); },
       function() { gameModel.reset(); },
@@ -75,6 +84,7 @@ define( function( require ) {
     var slideDistance = this.layoutBounds.width * 1.25;
 
     // helper function for moving to next game level
+    //REVIEW: This looks like a function that should belong on the game model itself?
     function goToNextLevel() {
       if ( gameModel.currentLevelProperty.get() < EEGameModel.NUMBER_OF_LEVELS - 1 ) {
         gameModel.currentLevelProperty.set( gameModel.currentLevelProperty.get() + 1 );
@@ -85,12 +95,14 @@ define( function( require ) {
     }
 
     // helper function for returning to level selection mode
+    //REVIEW: Just use gameModel.returnToLevelSelection.bind( gameModel )
     function returnToLevelSelection() {
       gameModel.returnToLevelSelection();
     }
 
     // create the game level views and add them to the main game play node
     this.gameLevelViews = [];
+    //REVIEW: a map() would work better instead of having to push? Just addChild inside it?
     gameModel.gameLevelModels.forEach( function( levelModel ) {
       var gameLevelView = new EEGameLevelView(
         levelModel,
@@ -106,18 +118,22 @@ define( function( require ) {
     } );
 
     // set the bounds for retrieving coin terms when expressions or composite coin terms are broken up
+    //REVIEW: This is done right after creating the views for each level. Can we just pass it as part of the view
+    // construction?
     gameModel.setCoinTermRetrievalBounds( this.layoutBounds );
 
     // hook up the animations for moving between level selection and game play
     gameModel.currentLevelProperty.lazyLink( function( newLevel, oldLevel ) {
 
+      //REVIEW: If currentLevelProperty is changed to point to EEGameLevelModels (renamed EEGameLevel), this isn't needed.
       var incomingViewNode = newLevel === null ? self.levelSelectionNode : self.gameLevelViews[ newLevel ];
       var outgoingViewNode = oldLevel === null ? self.levelSelectionNode : self.gameLevelViews[ oldLevel ];
       var outgoingNodeDestinationX;
       var incomingNodeStartX;
 
       if ( newLevel === null ) {
 
+        //REVIEW: See make-a-ten's SlidingScreen. Can that be used as an abstraction for this?
         // level selection screen is coming in, which is a left-to-right motion
         incomingNodeStartX = self.layoutBounds.minX - slideDistance;
         outgoingNodeDestinationX = self.layoutBounds.minX + slideDistance;
@@ -135,6 +151,7 @@ define( function( require ) {
       } );
 
       // move out the old node
+      //REVIEW: See make-a-ten's SlidingScreen. Can that be used as an abstraction for this? Doesn't use TWEEN
       new TWEEN.Tween( { x: nodeInViewport.x } )
         .to( { x: outgoingNodeDestinationX }, SCREEN_CHANGE_TIME )
         .easing( TWEEN.Easing.Cubic.InOut )
@@ -149,6 +166,7 @@ define( function( require ) {
         } );
 
       // move in the new node
+      //REVIEW: See make-a-ten's SlidingScreen. Can that be used as an abstraction for this? Doesn't use TWEEN
       incomingViewNode.x = incomingNodeStartX;
       incomingViewNode.visible = true;
       new TWEEN.Tween( { x: incomingViewNode.x } )
@@ -169,6 +187,7 @@ define( function( require ) {
   return inherit( ScreenView, EEGameScreenView, {
 
     // @public
+    //REVIEW: docs
     step: function( dt ) {
       var currentLevelNumber = this.gameModel.currentLevelProperty.get();
       if ( currentLevelNumber !== null ) {

js/game/view/StartGameLevelNode.js

@@ -5,6 +5,8 @@
  * REVIEW: Lots of duplicated code here with other files of the same name. Should be consolidated into vegas, or
  * simplified in each use case.
  *
+ * REVIEW: Created as a 'levelSelectionNode' variable, can we change name to LevelSelectionNode?
+ *
  * @author John Blanco
  */
 define( function( require ) {