From 3d772b73537390d1c80edbc780005dfcb560f457 Mon Sep 17 00:00:00 2001 From: jbphet Date: Wed, 7 Jun 2017 16:43:42 -0600 Subject: [PATCH] addressed several REVIEW comments, see #88 --- js/common/EESharedConstants.js | 14 ++++ js/common/enum/CoinTermTypeID.js | 3 + js/common/model/CoinTerm.js | 2 +- js/common/model/CoinTermFactory.js | 8 ++ js/game/view/EEGameScreenView.js | 7 -- js/game/view/EERewardNode.js | 12 +-- js/game/view/ExpressionDescriptionNode.js | 94 +++++++++-------------- 7 files changed, 69 insertions(+), 71 deletions(-) diff --git a/js/common/EESharedConstants.js b/js/common/EESharedConstants.js index c12e0024..36a8e865 100644 --- a/js/common/EESharedConstants.js +++ b/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 }; diff --git a/js/common/enum/CoinTermTypeID.js b/js/common/enum/CoinTermTypeID.js index 42741a94..cc58dc87 100644 --- a/js/common/enum/CoinTermTypeID.js +++ b/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 ); } diff --git a/js/common/model/CoinTerm.js b/js/common/model/CoinTerm.js index 7951ab1a..3c2fe496 100644 --- a/js/common/model/CoinTerm.js +++ b/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.}, read only, tracks what this coin term is composed of and what it can be broken down into diff --git a/js/common/model/CoinTermFactory.js b/js/common/model/CoinTermFactory.js index 81a7a41c..22519ee5 100644 --- a/js/common/model/CoinTermFactory.js +++ b/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 diff --git a/js/game/view/EEGameScreenView.js b/js/game/view/EEGameScreenView.js index b4c0d66c..e11b772a 100644 --- a/js/game/view/EEGameScreenView.js +++ b/js/game/view/EEGameScreenView.js @@ -44,14 +44,12 @@ 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(); }, @@ -59,7 +57,6 @@ define( function( require ) { 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 } ) diff --git a/js/game/view/EERewardNode.js b/js/game/view/EERewardNode.js index 8c4415cb..2c3ba936 100644 --- a/js/game/view/EERewardNode.js +++ b/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 ) } ); diff --git a/js/game/view/ExpressionDescriptionNode.js b/js/game/view/ExpressionDescriptionNode.js index 76968f94..c4be402d 100644 --- a/js/game/view/ExpressionDescriptionNode.js +++ b/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' + '2', SUB_SUP_OPTIONS ); - nodeInfo.charsUsed = 3; + node = new RichText( 'x' + '2', SUB_SUP_OPTIONS ); + charsUsed = 3; } else if ( expressionString.indexOf( 'y^2', startIndex ) === startIndex ) { - nodeInfo.node = new RichText( 'y2', SUB_SUP_OPTIONS ); - nodeInfo.charsUsed = 3; + node = new RichText( 'y2', 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 ); } ); \ No newline at end of file