addressed several REVIEW comments, see #88

js/common/EESharedConstants.js

@@ -9,10 +9,21 @@ define( function( require ) {
 
   // modules
   var Bounds2 = require( 'DOT/Bounds2' );
+  var CoinTermTypeID = require( 'EXPRESSION_EXCHANGE/common/enum/CoinTermTypeID' );
   var Color = require( 'SCENERY/util/Color' );
   var Dimension2 = require( 'DOT/Dimension2' );
   var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' );
 
+  // create a map of coin term IDs to the text that is shown when in 'variables' mode
+  var COIN_TERM_TYPE_TO_TEXT_MAP = new Map();
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.X_SQUARED_TIMES_Y_SQUARED, 'x^2*y2' );
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.X_SQUARED, 'x^2' );
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.Y_SQUARED, 'y^2' );
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.X_TIMES_Y, 'xy' );
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.X, 'x' );
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.Y, 'y' );
+  COIN_TERM_TYPE_TO_TEXT_MAP.set( CoinTermTypeID.Z, 'z' );
+
   var EESharedConstants = {
 
     LAYOUT_BOUNDS: new Bounds2( 0, 0, 1024, 618 ), // at the time of this writing this is the same as the default
@@ -42,6 +53,9 @@ define( function( require ) {
     // size of the collection areas in the game, in view coordinates, empirically determined
     COLLECTION_AREA_SIZE: new Dimension2( 220, 90 ),
 
+    // map of coin term types to the term text
+    COIN_TERM_TYPE_TO_TEXT_MAP: COIN_TERM_TYPE_TO_TEXT_MAP,
+
     // misc
     RESET_BUTTON_RADIUS: 24
   };

js/common/enum/CoinTermTypeID.js

@@ -21,6 +21,9 @@ define( function( require ) {
     CONSTANT: 'CONSTANT'
   };
 
+  // make the values available in an array
+  CoinTermTypeID.VALUES = _.values( CoinTermTypeID );
+
   // verify that enum is immutable, without the runtime penalty in production code
   if ( assert ) { Object.freeze( CoinTermTypeID ); }
 

js/common/model/CoinTerm.js

@@ -116,7 +116,7 @@ define( function( require ) {
     // @public, read only, indicates that the value will never change, will be displayed differently in the view
     this.isConstant = typeID === CoinTermTypeID.CONSTANT;
 
-    // @public, listen only, a property with contains the text that should be shown when displaying term value
+    // @public, listen only, a property which contains the text that should be shown when displaying term value
     this.termValueTextProperty = termValueTextProperty;
 
     // @public {Array.<number>}, read only, tracks what this coin term is composed of and what it can be broken down into

js/common/model/CoinTermFactory.js

@@ -23,6 +23,14 @@ define( function( require ) {
   var CONSTANT_ONE_VALUE_PROPERTY = new Property( 1 );
   var CONSTANT_ONE_TEXT_VALUE_PROPERTY = new Property( '1' );
 
+  // map of coin term type IDs to the text that is shown in 'variable' mode
+  var TYPE_TO_TEXT_MAP = new Map();
+  TYPE_TO_TEXT_MAP.set( CoinTermTypeID.X, 'x' );
+  TYPE_TO_TEXT_MAP.set( CoinTermTypeID.Y, 'y' );
+  TYPE_TO_TEXT_MAP.set( CoinTermTypeID.Z, 'z' );
+  TYPE_TO_TEXT_MAP.set( CoinTermTypeID.Z, 'z' );
+
+
   /**
    * @param {number} xValueProperty
    * @param {number} yValueProperty

js/game/view/EEGameScreenView.js

@@ -44,22 +44,19 @@ define( function( require ) {
     } );
 
     // 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: type/visibility docs
     var levelSelectionNode = new StartGameLevelNode(
       function( level ) { gameModel.selectLevel( level ); },
       function() { gameModel.reset(); },
       gameModel.soundEnabledProperty,
       levelSelectionButtonIcons,
       levelScoreProperties,
       {
-        //REVIEW: Almost all of these should be inlined in StartGameLevelNode, and not specified here.
         numStarsOnButtons: EEGameModel.CHALLENGES_PER_LEVEL,
         perfectScore: EEGameModel.MAX_SCORE_PER_LEVEL,
         numLevels: EEGameModel.NUMBER_OF_LEVELS,
@@ -80,7 +77,6 @@ define( function( require ) {
 
     // 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.gameLevels.forEach( function( levelModel ) {
       var gameLevelView = new EEGameLevelView(
         gameModel,
@@ -104,7 +100,6 @@ define( function( require ) {
 
       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;
@@ -122,7 +117,6 @@ 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 )
@@ -137,7 +131,6 @@ 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 } )

js/game/view/EERewardNode.js

@@ -31,18 +31,20 @@ define( function( require ) {
    * @constructor
    */
   function EERewardNode() {
+
     var nodes = [];
+
+    // add nodes that look like smiley faces, stars, and variables
     nodes.push( new FaceNode( FACE_DIAMETER ) );
     nodes.push( new StarNode( { outerRadius: STAR_OUTER_RADIUS, innerRadius: STAR_INNER_RADIUS } ) );
     nodes.push( new Text( 'x', { font: VARIABLE_FONT } ) );
     nodes.push( new Text( 'y', { font: VARIABLE_FONT } ) );
     nodes.push( new Text( 'z', { font: VARIABLE_FONT } ) );
 
-    //REVIEW: Don't like this pattern, what if you add a function to the enumeration type?
-    //REVIEW: Recommend instead adding CoinTermTypeID.VALUES as an array with all of the enumeration values.
-    _.values( CoinTermTypeID ).forEach( function( coinTermTypeId ) {
-      if ( coinTermTypeId !== CoinTermTypeID.CONSTANT ) {
-        nodes.push( CoinNodeFactory.createImageNode( coinTermTypeId, COIN_RADIUS, true ) );
+    // add coin images
+    CoinTermTypeID.VALUES.forEach( function( coinTermTypeID ) {
+      if ( coinTermTypeID !== CoinTermTypeID.CONSTANT ) {
+        nodes.push( CoinNodeFactory.createImageNode( coinTermTypeID, COIN_RADIUS, true ) );
       }
     } );
     RewardNode.call( this, { nodes: RewardNode.createRandomNodes( nodes, NUMBER_OF_NODES ) } );

js/game/view/ExpressionDescriptionNode.js

@@ -4,7 +4,8 @@
  * a Scenery node that represents a visual description of an expression, used in the game to describe what the user
  * should attempt to construct
  *
- * REVIEW: This should use the ExpressionDescription's terms / termArray instead of re-parsing the string?
+ * Note the the expression description string is re-parsed in this object because the expression description is in the
+ * reduced form (no parens) and we need to handle subscripts and superscripts.
  */
 define( function( require ) {
   'use strict';
@@ -13,9 +14,9 @@ define( function( require ) {
   var CoinNodeFactory = require( 'EXPRESSION_EXCHANGE/common/view/CoinNodeFactory' );
   var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' );
   var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' );
+  var HBox = require( 'SCENERY/nodes/HBox' );
   var inherit = require( 'PHET_CORE/inherit' );
   var MathSymbolFont = require( 'SCENERY_PHET/MathSymbolFont' );
-  var Node = require( 'SCENERY/nodes/Node' );
   var PhetFont = require( 'SCENERY_PHET/PhetFont' );
   var RichText = require( 'SCENERY_PHET/RichText' );
   var Text = require( 'SCENERY/nodes/Text' );
@@ -24,11 +25,8 @@ define( function( require ) {
   // constants
   var COIN_ICON_RADIUS = 10;
   var COIN_EXPRESSION_FONT = new PhetFont( 22 );
-  var COEFFICIENT_TO_COIN_SPACING = 1;
-  var COIN_TO_PLUS_SIGN_SPACING = 5;
   var EXPRESSION_FONT_FOR_NON_VARIABLE = new PhetFont( 22 );
   var EXPRESSION_FONT_FOR_VARIABLES = new MathSymbolFont( 24 );
-  var ADDITIONAL_EXPRESSION_FRAGMENT_SPACING = 2; // empirically determined to look good on the most platforms
   var SUP_SCALE = 0.65; // empirically determined to look good on the most platforms
   var SUB_SUP_OPTIONS = { font: EXPRESSION_FONT_FOR_VARIABLES, supScale: SUP_SCALE };
 
@@ -39,62 +37,51 @@ define( function( require ) {
    * @constructor
    */
   function ExpressionDescriptionNode( expressionDescription, viewMode, options ) {
-    var self = this;
-    Node.call( this );
 
-    //REVIEW: Using HBoxes would potentially be cleaner (not specifying lefts/centerYs everywhere)?
-    var nextXPos = 0;
+    HBox.call( this, { align: 'bottom' } );
+    var self = this;
 
     if ( viewMode === ViewMode.COINS ) {
+
+      this.setAlign( 'center' );
+
       expressionDescription.terms.forEach( function( expressionTerm, index ) {
 
         // add coefficient if needed
         if ( expressionTerm.coefficient > 1 ) {
-          var coefficientNode = new Text( expressionTerm.coefficient, {
-            font: COIN_EXPRESSION_FONT,
-            left: nextXPos,
-            centerY: COIN_ICON_RADIUS
-          } );
+          var coefficientNode = new Text( expressionTerm.coefficient, { font: COIN_EXPRESSION_FONT } );
           self.addChild( coefficientNode );
-          nextXPos += coefficientNode.width + COEFFICIENT_TO_COIN_SPACING;
         }
 
         // add coin icon
-        //REVIEW: for memory usage, consider not creating fresh coins here, but instead use DAG?
         var coinIconNode = CoinNodeFactory.createIconNode(
           expressionTerm.coinTermTypeID,
-          COIN_ICON_RADIUS,
-          { left: nextXPos, centerY: COIN_ICON_RADIUS }
+          COIN_ICON_RADIUS
         );
         self.addChild( coinIconNode );
-        nextXPos += coinIconNode.width + COIN_TO_PLUS_SIGN_SPACING;
 
         // add plus symbol if not at end of expression
         if ( index < expressionDescription.terms.length - 1 ) {
-          var plusSign = new Text( '+', {
-            font: COIN_EXPRESSION_FONT,
-            left: nextXPos,
-            centerY: COIN_ICON_RADIUS
-          } );
+          var plusSign = new Text( ' + ', { font: COIN_EXPRESSION_FONT } );
           self.addChild( plusSign );
-          nextXPos += plusSign.width + COIN_TO_PLUS_SIGN_SPACING;
         }
 
       } );
     }
     else if ( viewMode === ViewMode.VARIABLES ) {
 
-      // go through the expression string, turning the various pieces into nodes
+      this.setAlign( 'bottom' );
+
+      // Go through the expression string, turning the various pieces into nodes.  The 'terms' field of the expression
+      // description can't be used here because it is the returned version of the expression.
       var expressionStringIndex = 0;
+
       while ( expressionStringIndex < expressionDescription.expressionString.length ) {
-        //REVIEW: Why are we parsing this string, instead of relying on the terms array?
         var expressionFragmentInfo = createExpressionFragment(
           expressionDescription.expressionString,
           expressionStringIndex
         );
-        expressionFragmentInfo.node.left = nextXPos;
-        this.addChild( expressionFragmentInfo.node );
-        nextXPos += expressionFragmentInfo.node.width + ADDITIONAL_EXPRESSION_FRAGMENT_SPACING;
+        self.addChild( expressionFragmentInfo.node );
         expressionStringIndex += expressionFragmentInfo.charsUsed;
       }
     }
@@ -109,66 +96,58 @@ define( function( require ) {
    * helper function that creates an object that consists of a node that represents a variable and the number of
    * characters from the expression string that the node represents
    */
-  //REVIEW: Why parsing strings, and not using a logical (object) format with equivalent toString methods?
   function createVariableExpressionFragment( expressionString, startIndex ) {
 
     // error checking
     var firstChar = expressionString.charAt( startIndex );
     assert && assert( firstChar === 'x' || firstChar === 'y' || firstChar === 'z', 'unexpected first char of string' );
 
-    // create the object that will be populated and returned
-    //REVIEW: Usually simpler to return inline new objects, e.g.
-    // return {
-    //   node: ...,
-    //   charsUsed: ...
-    // }
-    var nodeInfo = {
-      node: null,
-      charsUsed: 0
-    };
+    var node = null;
+    var charsUsed = 0;
 
-    //REVIEW: Just use the parsed ExpressionDescription instead of this?
     // identify the expression to be created based on a finite set of those supported
     if ( expressionString.indexOf( 'x^2', startIndex ) === startIndex ) {
-      nodeInfo.node = new RichText( 'x' + '<sup>2</sup>', SUB_SUP_OPTIONS );
-      nodeInfo.charsUsed = 3;
+      node = new RichText( 'x' + '<sup>2</sup>', SUB_SUP_OPTIONS );
+      charsUsed = 3;
     }
     else if ( expressionString.indexOf( 'y^2', startIndex ) === startIndex ) {
-      nodeInfo.node = new RichText( 'y<sup>2</sup>', SUB_SUP_OPTIONS );
-      nodeInfo.charsUsed = 3;
+      node = new RichText( 'y<sup>2</sup>', SUB_SUP_OPTIONS );
+      charsUsed = 3;
     }
     else if ( expressionString.indexOf( 'xy', startIndex ) === startIndex ) {
-      nodeInfo.node = new Text( 'xy', {
+      node = new Text( 'xy', {
         font: EXPRESSION_FONT_FOR_VARIABLES
       } );
-      nodeInfo.charsUsed = 2;
+      charsUsed = 2;
     }
     else if ( expressionString.indexOf( 'x', startIndex ) === startIndex ) {
-      nodeInfo.node = new Text( 'x', {
+      node = new Text( 'x', {
         font: EXPRESSION_FONT_FOR_VARIABLES
       } );
-      nodeInfo.charsUsed = 1;
+      charsUsed = 1;
     }
     else if ( expressionString.indexOf( 'y', startIndex ) === startIndex ) {
-      nodeInfo.node = new Text( 'y', {
+      node = new Text( 'y', {
         font: EXPRESSION_FONT_FOR_VARIABLES
       } );
-      nodeInfo.charsUsed = 1;
+      charsUsed = 1;
     }
     else if ( expressionString.indexOf( 'z', startIndex ) === startIndex ) {
-      nodeInfo.node = new Text( 'z', {
+      node = new Text( 'z', {
         font: EXPRESSION_FONT_FOR_VARIABLES
       } );
-      nodeInfo.charsUsed = 1;
+      charsUsed = 1;
     }
     else {
       assert && assert( false, 'unsupported expression' );
     }
 
-    return nodeInfo;
+    return {
+      node: node,
+      charsUsed: charsUsed
+    };
   }
 
-  //REVIEW: Why parsing strings, and not using a logical (object) format with equivalent toString methods?
   function createNonVariableExpressionFragment( expressionString, startIndex ) {
     var fragmentString = '';
     var index = startIndex;
@@ -197,7 +176,6 @@ define( function( require ) {
    */
   function createExpressionFragment( expressionString, index ) {
 
-    //REVIEW: Why are we parsing strings? Presumably there is an improved logical representation of this that would be easier?
     var expressionFragment;
     var nextChar = expressionString.charAt( index );
     if ( nextChar === 'x' || nextChar === 'y' || nextChar === 'z' ) {
@@ -211,5 +189,5 @@ define( function( require ) {
 
   expressionExchange.register( 'ExpressionDescriptionNode', ExpressionDescriptionNode );
 
-  return inherit( Node, ExpressionDescriptionNode );
+  return inherit( HBox, ExpressionDescriptionNode );
 } );