addressed several REVIEW comments, see #88

js/game/view/EEGameScreenView.js

@@ -17,7 +17,7 @@ define( function( require ) {
   var GameAudioPlayer = require( 'VEGAS/GameAudioPlayer' );
   var inherit = require( 'PHET_CORE/inherit' );
   var ScreenView = require( 'JOIST/ScreenView' );
-  var StartGameLevelNode = require( 'EXPRESSION_EXCHANGE/game/view/StartGameLevelNode' );
+  var LevelSelectionNode = require( 'EXPRESSION_EXCHANGE/game/view/LevelSelectionNode' );
 
   // constants
   var SCREEN_CHANGE_TIME = 1000; // milliseconds
@@ -50,18 +50,13 @@ define( function( require ) {
     } );
 
     // add the node that allows the user to choose a game level to play
-    var levelSelectionNode = new StartGameLevelNode(
+    var levelSelectionNode = new LevelSelectionNode(
       function( level ) { gameModel.selectLevel( level ); },
       function() { gameModel.reset(); },
       gameModel.soundEnabledProperty,
       levelSelectionButtonIcons,
       levelScoreProperties,
       {
-        numStarsOnButtons: EEGameModel.CHALLENGES_PER_LEVEL,
-        perfectScore: EEGameModel.MAX_SCORE_PER_LEVEL,
-        numLevels: EEGameModel.NUMBER_OF_LEVELS,
-        numButtonRows: 2,
-        controlsInset: 10,
         size: this.layoutBounds,
         centerX: this.layoutBounds.centerX
       }

js/game/view/StartGameLevelNode.js → js/game/view/LevelSelectionNode.js

@@ -2,10 +2,6 @@
 
 /**
  * A node that fills most of the screen and allows the user to select the game level that they wish to play.
- * 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
  */
@@ -14,6 +10,7 @@ define( function( require ) {
 
   // modules
   var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' );
+  var EEGameModel = require( 'EXPRESSION_EXCHANGE/game/model/EEGameModel' );
   var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' );
   var inherit = require( 'PHET_CORE/inherit' );
   var LevelSelectionButton = require( 'VEGAS/LevelSelectionButton' );
@@ -32,52 +29,50 @@ define( function( require ) {
 
   /**
    * @param {Function} startLevelFunction - Function used to initiate a game level, will be called with a zero-based
-   *                                        index value.
+   * index value.
    * @param {Function} resetFunction - Function to reset game and scores.
    * @param {Property.<boolean>} soundEnabledProperty
    * @param {Array.<Node>} iconNodes - Set of iconNodes to use on the buttons, sizes should be the same, length of array
-   *                                   must match number of levels.
+   * must match number of levels.
    * @param {Array.<Property.<number>>} scores - Current scores, used to decide which stars to illuminate on the level
-   *                                             start buttons, length must match number of levels.
-   *                                             REVIEW: added type doc here, but can you verify it's correct?
+   * start buttons, length must match number of levels.
    * @param {Object} [options] - See code below for options and default values.
    * @constructor
    */
-  function StartGameLevelNode( startLevelFunction, resetFunction, soundEnabledProperty, iconNodes, scores, options ) {
+  function LevelSelectionNode( startLevelFunction, resetFunction, soundEnabledProperty, iconNodes, scores, options ) {
 
     Node.call( this );
 
-    //REVIEW: Options not really ever needed, should just be inlined (there is only one constructor call!)
     options = _.extend( {
 
       // defaults
-      numLevels: 4, // REVIEW: Move actual value (EEGameModel.NUMBER_OF_LEVELS) from EEGameScreenView to here.
+      numLevels: EEGameModel.NUMBER_OF_LEVELS,
       titleString: chooseYourLevelString,
       maxTitleWidth: 500,
-      numStarsOnButtons: 5, // REVIEW: Move actual value (EEGameModel.CHALLENGES_PER_LEVEL) from EEGameScreenView to here.
-      perfectScore: 10, // REVIEW: Move actual value (EEGameModel.MAX_SCORE_PER_LEVEL) from EEGameScreenView to here.
+      numStarsOnButtons: EEGameModel.CHALLENGES_PER_LEVEL,
+      perfectScore: EEGameModel.MAX_SCORE_PER_LEVEL,
       buttonBackgroundColor: '#EDA891',
-      numButtonRows: 1, // For layout REVIEW: Move actual value (2) from EEGameScreenView to here. Confusing to have two values when only one is ever used
-      controlsInset: 12, // REVIEW: move from EEGameScreenView, actual value 10
-      size: EESharedConstants.LAYOUT_BOUNDS // REVIEW: Don't specify default here, only usage uses layoutBounds
+      numButtonRows: 2,
+      controlsInset: 10,
+      size: EESharedConstants.LAYOUT_BOUNDS,
+      buttonScale: 0.8
     }, options );
 
     // Verify parameters
-    //REVIEW: Looks like this should be an assertion instead?
-    if ( iconNodes.length !== options.numLevels || scores.length !== options.numLevels ) {
-      throw new Error( 'Number of game levels doesn\'t match length of provided arrays' );
-    }
+    assert(
+      iconNodes.length === options.numLevels && scores.length === options.numLevels,
+      'Number of game levels doesn\'t match length of provided arrays'
+    );
 
-    // Title
+    // title
     var title = new Text( options.titleString, { font: new PhetFont( 30 ), maxWidth: options.maxTitleWidth } );
     this.addChild( title );
 
-    // Add the buttons
+    // add the buttons
     function createLevelStartFunction( level ) {
       return function() { startLevelFunction( level ); };
     }
 
-    //REVIEW: Using a game Level object should help clean this code up?
     var buttons = new Array( options.numLevels );
     for ( var i = 0; i < options.numLevels; i++ ) {
       buttons[ i ] = new LevelSelectionButton(
@@ -87,11 +82,10 @@ define( function( require ) {
         scores[ i ],
         options.perfectScore,
         {
-          baseColor: options.buttonBackgroundColor
+          baseColor: options.buttonBackgroundColor,
+          scale: options.buttonScale
         }
       );
-      //REVIEW: Why is this scale done here, instead of { scale: 0.8 } in the above options? (and 0.8 instead of 0.80)
-      buttons[ i ].scale( 0.80 );
       this.addChild( buttons[ i ] );
     }
 
@@ -111,7 +105,6 @@ define( function( require ) {
     this.addChild( resetButton );
 
     // Layout
-    //REVIEW: Lots of copy-pasted code from other StartGameLevelNodes? Clean up, use layout boxes if possible
     var numColumns = options.numLevels / options.numButtonRows;
     var buttonSpacingX = buttons[ 0 ].width * 1.2; // Note: Assumes all buttons are the same size.
     var buttonSpacingY = buttons[ 0 ].height * 1.2;  // Note: Assumes all buttons are the same size.
@@ -132,7 +125,7 @@ define( function( require ) {
     soundToggleButton.bottom = options.size.height - options.controlsInset;
   }
 
-  expressionExchange.register( 'StartGameLevelNode', StartGameLevelNode );
+  expressionExchange.register( 'LevelSelectionNode', LevelSelectionNode );
 
-  return inherit( Node, StartGameLevelNode );
+  return inherit( Node, LevelSelectionNode );
 } );

js/game/view/NextLevelNode.js

@@ -30,7 +30,6 @@ define( function( require ) {
     var faceNode = new FaceNode( FACE_DIAMETER );
     this.addChild( faceNode );
 
-    //REVIEW: Replace the layout code with a VBox with spacing 10?
     var button = new RectangularPushButton( {
       content: new Text( nextString, { font: new PhetFont( 30 ) } ),
       centerX: faceNode.centerX,

js/game/view/UndoButton.js

@@ -34,21 +34,19 @@ define( function( require ) {
       arrowFill: 'black'
     }, options );
 
+    assert && assert( !options.content, 'content should not be specified for this button' );
+
     // create the shape for the undo arrow
     var undoArrowShape = new Shape()
       .moveTo( 0, 0 )
       .lineTo( 0, ICON_HEIGHT )
       .lineTo( ICON_HEIGHT, ICON_HEIGHT )
       .lineTo( ICON_HEIGHT * 0.7, ICON_HEIGHT * 0.7 )
-      //.lineTo( ICON_HEIGHT * 2, ICON_HEIGHT * 0.5 ) // REVIEW: can these commented out lines be removed or tagged with an issue?
       .quadraticCurveTo( ICON_HEIGHT * 1.25, -ICON_HEIGHT * 0.1, ICON_HEIGHT * 2, ICON_HEIGHT * 0.75 )
-      //.lineTo( ICON_HEIGHT * 0.25, ICON_HEIGHT * 0.25 )
       .quadraticCurveTo( ICON_HEIGHT * 1.25, -ICON_HEIGHT * 0.5, ICON_HEIGHT * 0.3, ICON_HEIGHT * 0.3 )
-      .lineTo( 0, 0 ) // REVIEW: this was the starting point, so the extra lineTo isn't necessary before the close.
       .close();
 
     // set up the content node
-    //REVIEW: CM would prefer an assertion to make sure this wasn't already specified in the options.
     options.content = new Path( undoArrowShape, {
       fill: options.arrowFill
     } );

js/negatives/view/EENegativesIconNode.js

@@ -11,6 +11,7 @@ define( function( require ) {
   // modules
   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' );
@@ -36,20 +37,15 @@ define( function( require ) {
     Rectangle.call( this, 0, 0, ICON_SIZE.width, ICON_SIZE.height, { fill: BACKGROUND_COLOR } );
 
     // create and add the equation node
-    //REVIEW: Relies on Text.left being 0. Seems like we can just use HBox here to simplify logic?
-    var equationNode = new Node();
-    equationNode.addChild( new Text( '3', { font: TEXT_FONT } ) );
-    equationNode.addChild( new RichText( 'x<sup>2</sup>', {
-      font: MATH_FONT,
-      supScale: 0.5,
-      left: equationNode.width
-    } ) );
-    equationNode.addChild( new Text( ' \u2212 ', { font: TEXT_FONT, left: equationNode.width } ) );
-    equationNode.addChild( new RichText( 'x<sup>2</sup>', {
-      font: MATH_FONT,
-      supScale: 0.5,
-      left: equationNode.width
-    } ) );
+    var equationNode = new HBox( {
+      children: [
+        new Text( '3', { font: TEXT_FONT } ),
+        new RichText( 'x<sup>2</sup>', { font: MATH_FONT, supScale: 0.5 } ),
+        new Text( ' \u2212 ', { font: TEXT_FONT } ),
+        new RichText( 'x<sup>2</sup>', { font: MATH_FONT, supScale: 0.5 } )
+      ],
+      align: 'bottom'
+    } );
     equationNode.centerX = ICON_SIZE.width / 2;
     equationNode.centerY = ICON_SIZE.height * 0.45;
     this.addChild( equationNode );