More review comments, see #88

js/game/model/EEChallengeDescriptors.js

@@ -562,7 +562,7 @@ define( function( require ) {
      * get a challenge descriptor for the specified level
      * @param {number} level
      * @param {number} challengeNumber
-     * @returns {Object}
+     * @returns {Object} REVIEW: Definitely need documentation on this. Why not create an EEChallengeDescriptor type?
      * @public
      */
     getChallengeDescriptor: function( level, challengeNumber ) {
@@ -571,13 +571,15 @@ define( function( require ) {
 
     /**
      * randomize the challenge sets
+     * @public
      */
     shuffleChallenges: function() {
       for ( var i = 0; i < challengeSets.length; i++ ) {
         challengeSets[ i ] = phet.joist.random.shuffle( challengeSets[ i ] );
       }
     },
 
+    //REVIEW: Usually type doc/visibility?
     CHALLENGES_PER_LEVEL: 5
   };
 

js/game/model/EECollectionArea.js

@@ -28,25 +28,27 @@ define( function( require ) {
    */
   function EECollectionArea( x, y, viewMode ) {
 
-    // @public, read-only {Expression||CoinTerm} - expression or coin term that has been collected, null if nothing
+    // @public, read-only {Expression|CoinTerm} - expression or coin term that has been collected, null if nothing
     this.collectedItemProperty = new Property( null );
 
     // @public, read-write - description of the expression that this capture area can hold
+    //REVIEW: Is this {Property.<ExpressionDescription}?
     this.expressionDescriptionProperty = new Property( null );
 
-    // @public, read-only - bounds in model space of this capture area
+    // @public {Bounds2}, read-only - bounds in model space of this capture area
+    //REVIEW: EESharedConstants.COLLECTION_AREA_SIZE.toBounds( x, y )
     this.bounds = new Bounds2(
       x,
       y,
       x + EESharedConstants.COLLECTION_AREA_SIZE.width,
       y + EESharedConstants.COLLECTION_AREA_SIZE.height
     );
 
-    // @public (read-only) - view mode (coins or variables)
+    // @public {ViewMode} (read-only) - view mode (coins or variables)
     this.viewMode = viewMode;
 
-    // @public - used by the view to turn on/off a "halo" for the collection area, generally used when the user holds
-    // something over the collection area
+    // @public {Property.<boolean>} - used by the view to turn on/off a "halo" for the collection area, generally used
+    // when the user holds something over the collection area
     this.haloActiveProperty = new Property( false );
   }
 
@@ -169,6 +171,7 @@ define( function( require ) {
      * get a reference to this collection area's model bounds, the results should not be changed
      * @returns {Bounds2}
      * @public
+     * REVIEW: this.bounds is public, what's the purpose of this extra method? Is it overridden anywhere?
      */
     getBounds: function() {
       return this.bounds;

js/game/view/EECollectionAreaNode.js

@@ -17,14 +17,15 @@ define( function( require ) {
   var CORNER_RADIUS = 4;
 
   /**
-   * @param collectionArea
+   * @param {EECollectionArea} collectionArea
    * @constructor
    */
   function EECollectionAreaNode( collectionArea ) {
     var self = this;
     Node.call( this );
 
     // create the 'halo' that will turn on as a hint that the user can drop something into the collection area
+    //REVIEW: Rectangle.bounds( collectionArea.bounds, { cornerRadius: CORNER_RADIUS, ... } ) if we don't translate by the bounds minX/minY
     var halo = new Rectangle(
       0,
       0,
@@ -43,6 +44,7 @@ define( function( require ) {
     collectionArea.haloActiveProperty.linkAttribute( halo, 'visible' );
 
     // create the basic rectangular background
+    //REVIEW: Rectangle.bounds( collectionArea.bounds, { cornerRadius: CORNER_RADIUS, ... } ) if we don't translate by the bounds minX/minY
     var collectionAreaRectangle = new Rectangle(
       0,
       0,
@@ -64,10 +66,13 @@ define( function( require ) {
       // remove the previous expression description node, if present
       if ( expressionDescriptionNode ) {
         self.removeChild( expressionDescriptionNode );
+        //REVIEW: Usually with this pattern, the expressionDescriptionNode would be set to null? Otherwise if the
+        // description goes to null and back, things error?
       }
 
       // add the description node for the new expression
       if ( expressionDescription ) {
+        //REVIEW: If we don't translate by the bounds minX/minY (as suggested above), just use things relative to the bounds instead
         expressionDescriptionNode = new ExpressionDescriptionNode(
           expressionDescription,
           collectionArea.viewMode,
@@ -77,6 +82,7 @@ define( function( require ) {
       }
     } );
 
+    //REVIEW: Recommend removing this, as it makes the Rectangle constructors a lot more verbose (and is an extra line)
     this.setTranslation( collectionArea.bounds.minX, collectionArea.bounds.minY );
   }
 

js/game/view/EEGameIconNode.js

@@ -33,9 +33,11 @@ define( function( require ) {
   function EEGameIconNode() {
 
     // create the background
+    //REVIEW: Rectangle.dimension( ICON_SIZE, { fill: BACKGROUND_COLOR } )
     Rectangle.call( this, 0, 0, ICON_SIZE.width, ICON_SIZE.height, { fill: BACKGROUND_COLOR } );
 
     // created a rounded rectangle that looks like a card
+    //REVIEW: Recommend cornerRadius (duplicated) specified once in options object
     var cardBackground = new Rectangle( 0, 0, ICON_SIZE.width / 2, ICON_SIZE.height / 2, 20, 20, {
       x: ICON_SIZE.width * 0.1,
       y: ICON_SIZE.height * 0.1,
@@ -50,6 +52,7 @@ define( function( require ) {
     var coinEquation = new Node();
     coinEquation.addChild( new Text( '2', { font: TEXT_FONT } ) );
     var coinRadius = ICON_SIZE.width * 0.04; // empirically determined
+    //REVIEW: Can an HBox be used here? looks like anti-pattern with incrementing left based on extisting width.
     coinEquation.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, {
       left: coinEquation.width,
       centerY: coinEquation.centerY
@@ -68,11 +71,13 @@ define( function( require ) {
     } ) );
 
     // add the coin equation to the card
+    //REVIEW: coinEquation.center = cardBackground.localBounds.center?
     coinEquation.centerX = cardBackground.width / 2;
     coinEquation.centerY = cardBackground.height / 2;
     cardBackground.addChild( coinEquation );
 
     // create and add coins next to the card
+    //REVIEW: Has some values that could be factored out (and named for improved readability)
     this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, {
       left: cardBackground.right + COIN_SPACING,
       centerY: cardBackground.top + cardBackground.height * 0.3

js/game/view/EEGameLevelIconFactory.js

@@ -30,12 +30,15 @@ define( function( require ) {
   var CARD_ICON_HEIGHT = 40;
 
   // helper function for creating coin-image-based icons
+  //REVIEW Type docs generally helpful when reading this type of function
   function createCoinIcon( coinTermTypeID, value ) {
 
     var rootNode = new Node();
     var imageNode;
 
     // create the coin image node
+    //REVIEW: These images are also used in similar switch statements in CoinNodeFactory. This could be potentially
+    // factored out into CoinNodeFactory?
     switch( coinTermTypeID ) {
       case CoinTermTypeID.X:
         imageNode = new Image( coinXTimesYFrontImage );
@@ -64,10 +67,14 @@ define( function( require ) {
       centerY: imageNode.height / 2
     } ) );
 
+    //REVIEW: Easier to
+    // return new Node( { children: [ imageNode, the_text_thing ] } ) rather than having an additional declaration and return.
+
     return rootNode;
   }
 
   // helper function for creating the icons that look like stacks of cards
+  //REVIEW Type docs generally helpful when reading this type of function
   function createCardStackIcon( numberOnStack, numberOfAdditionalCards ) {
     var rootNode = new Node();
     var cardWidth = ICON_WIDTH - numberOfAdditionalCards * CARD_STAGGER_OFFSET;
@@ -93,6 +100,7 @@ define( function( require ) {
     } ) );
 
     // add the cards to the root node
+    //REVIEW: cards.reverse() returns the reversed array, so that can be chained with the forEach directly
     cards.reverse();
     cards.forEach( function( card ) {
       rootNode.addChild( card );
@@ -117,6 +125,8 @@ define( function( require ) {
 
       var icon;
 
+      //REVIEW: If helpful, I've found it nice to have different objects for each level that handle creation of icons.
+      //REVIEW: No problem to leave as-is if this is the most convenient.
       switch( gameLevel ) {
 
         case 0: