diff --git a/js/common/model/CoinTerm.js b/js/common/model/CoinTerm.js index 6d11fe92..9f860908 100644 --- a/js/common/model/CoinTerm.js +++ b/js/common/model/CoinTerm.js @@ -59,25 +59,26 @@ define( function( require ) { // properties //------------------------------------------------------------------------ - // @public (read only), set using methods below + // @public {Property.} (read only), set using methods below this.positionProperty = new Property( options.initialPosition ); - // @public (read only), set using methods below + // @public {Property.} (read only), set using methods below this.destinationProperty = new Property( options.initialPosition ); - // @public, indicate whether user is currently dragging this coin + // @public {Property.}, indicate whether user is currently dragging this coin this.userControlledProperty = new Property( false ); - // @public + // @public {Property.} this.combineHaloActiveProperty = new Property( false ); - // @public, supports showing subtraction in expressions + // @public {Property.}, supports showing subtraction in expressions this.showMinusSignWhenNegativeProperty = new Property( true ); - // @public, indicates whether this is in a collection box (for game) + // @public {Property., indicates whether this is in a collection box (for game) this.collectedProperty = new Property( false ); - // @public (read only), tracks the current in-progress animation, if any + // @public {Property.} (read only), tracks the current in-progress animation, if any + //REVIEW: Here's an example where type documentation really helps. No idea (in initial read through) what type it can hold. this.inProgressAnimationProperty = new Property( null ); // @public (read-only) - total number of coins/terms combined into this one, can be negative @@ -118,6 +119,7 @@ define( function( require ) { this.termValueTextProperty = termValueTextProperty; // @public, read only, tracks what this coin term is composed of and what it can be broken down into + //REVIEW: Type documentation would definitely help here also. this.composition = []; if ( Math.abs( options.initialCount ) > 1 && options.decomposable ) { _.times( Math.abs( options.initialCount ), function() { @@ -291,6 +293,7 @@ define( function( require ) { } }, + //REVIEW: doc returnToOrigin: function() { this.travelToDestination( this.initialPosition ); }, @@ -298,6 +301,7 @@ define( function( require ) { /** * set both the position and destination in such a way that no animation is initiated * @param position + * REVIEW: @param type? * @public */ setPositionAndDestination: function( position ) { @@ -425,6 +429,7 @@ define( function( require ) { getViewBounds: function() { var position = this.positionProperty.get(); var relativeViewBounds = this.relativeViewBoundsProperty.get(); + //REVIEW: relativeViewBounds.shifted( position.x, position.y ) return new Bounds2( position.x + relativeViewBounds.minX, position.y + relativeViewBounds.minY, diff --git a/js/common/model/CoinTermFactory.js b/js/common/model/CoinTermFactory.js index 747999b9..53409c9d 100644 --- a/js/common/model/CoinTermFactory.js +++ b/js/common/model/CoinTermFactory.js @@ -25,6 +25,7 @@ define( function( require ) { var CONSTANT_ONE_TEXT_VALUE_PROPERTY = new Property( '1' ); /** + * REVIEW: Needs @param * {number} xValueProperty * {number} yValueProperty * {number} zValueProperty @@ -40,6 +41,7 @@ define( function( require ) { this.zValueProperty = zValueProperty; // @private - string representations of the variables + //REVIEW: Property doesn't allow missing parameter? this.xValueStringProperty = new Property(); this.yValueStringProperty = new Property(); this.zValueStringProperty = new Property(); diff --git a/js/common/view/AbstractCoinTermNode.js b/js/common/view/AbstractCoinTermNode.js index 9cc5fc12..9a0e2dde 100644 --- a/js/common/view/AbstractCoinTermNode.js +++ b/js/common/view/AbstractCoinTermNode.js @@ -31,7 +31,10 @@ define( function( require ) { * @constructor */ function AbstractCoinTermNode( coinTerm, options ) { + //REVIEW: Would highly recommend changing this to set more properties on the object and used methods. + // A 240+ line constructor with 12 functions that would be methods typically would be cleaner if broken up. + //REVIEW: Why the extra empty object in the extend? options = _.extend( {}, { addDragHandler: true, dragBounds: Bounds2.EVERYTHING, @@ -41,19 +44,23 @@ define( function( require ) { var self = this; Node.call( this, { pickable: true, cursor: 'pointer' } ); - // @public (read only) + // @public {CoinTerm} (read only) this.coinTerm = coinTerm; + // @protected {Rectangle} // Add the card-like background, initially tiny, will be set in subclasses by function that updates the // representation. + //REVIEW: It's a personal preference, but I prefer radius, in the options when available, e.g.: + // { cornerRadius: BACKGROUND_CORNER_ROUNDING } this.cardLikeBackground = new Rectangle( -1, -1, 2, 2, BACKGROUND_CORNER_ROUNDING, BACKGROUND_CORNER_ROUNDING, { fill: EESharedConstants.CARD_BACKGROUND_COLOR, stroke: 'black', - lineWidth: 1, + lineWidth: 1, //REVIEW: lineWidth 1 is the default, should not be specified visible: false } ); this.addChild( this.cardLikeBackground ); + // @protected {Node} // Add a root node so that the bounds can be easily monitored for changes in size without getting triggered by // changes in position. this.coinAndTextRootNode = new Node(); @@ -77,6 +84,8 @@ define( function( require ) { this.addChild( breakApartButton ); // adjust the touch area of the break apart button to make it easier to use on touch devices + //REVIEW: breakApartButton.localBounds.dilatedX( breakApartButton.width / 2 ) + // .withOffsets( 0, breakApartButton.height, 0, 0 ); var breakApartButtonTouchArea = breakApartButton.localBounds.copy(); breakApartButtonTouchArea.minX = breakApartButtonTouchArea.minX - breakApartButton.width / 2; breakApartButtonTouchArea.maxX = breakApartButtonTouchArea.maxX + breakApartButton.width / 2; @@ -112,6 +121,7 @@ define( function( require ) { // keep the button showing if the user is over it function handleOverBreakApartButtonChanged( overButton ) { + //REVIEW: Invert if statements to check for whether it is user controlled first (deduplicates) if ( overButton ) { if ( !coinTerm.userControlledProperty.get() ) { assert && assert( !!hideButtonTimer, 'should not be over button without hide timer running' ); @@ -143,6 +153,7 @@ define( function( require ) { // move this node as the model representation moves function handlePositionChanged( position ) { // the intent here is to position the center of the coin at the position, NOT the center of the node + //REVIEW: self.translation = position; self.x = position.x; self.y = position.y; } diff --git a/js/common/view/BreakApartButton.js b/js/common/view/BreakApartButton.js index 29a49c7c..24b6e2ac 100644 --- a/js/common/view/BreakApartButton.js +++ b/js/common/view/BreakApartButton.js @@ -22,7 +22,7 @@ define( function( require ) { /** * @constructor - * {Object} options + * @param {Object} [options] */ function BreakApartButton( options ) { @@ -34,6 +34,8 @@ define( function( require ) { assert && assert( options.mode === 'normal' || options.mode === 'inverted', 'invalid mode option' ); // the following options can't be overridden + //REVIEW: If they can't be overridden, do _.extend( options, { ... these ... } ) so the declaration is cleaner. + // It will mutate the options object, overriding as necessary. Presumably include the content one below also. options.xMargin = MARGIN; options.yMargin = MARGIN; options.baseColor = options.mode === 'normal' ? YELLOW : BLACK; diff --git a/js/common/view/CoinNodeFactory.js b/js/common/view/CoinNodeFactory.js index 9affe8d5..f51047b0 100644 --- a/js/common/view/CoinNodeFactory.js +++ b/js/common/view/CoinNodeFactory.js @@ -39,10 +39,12 @@ define( function( require ) { var outerCircle = new Circle( outerCircleRadius, { fill: outerCircleColor, + //REVIEW: at least 4 places with the same lineWidth and 0.25-darker color. Possibility for refactoring? stroke: outerCircleColor.colorUtilsDarker( 0.25 ), lineWidth: 0.5 } ); + //REVIEW: Type documentation may help make clear that this is optional? if ( innerCircleRadius ) { outerCircle.addChild( new Circle( innerCircleRadius, { fill: innerCircleColor, @@ -60,9 +62,14 @@ define( function( require ) { var outerShape = new Shape(); var vector = new Vector2( 0, outerMaxRadius ); vector.rotate( -Math.PI * 0.055 ); + //REVIEW: outerShape.moveToPoint( Vector2.createPolar( outerMaxRadius, Math.PI * 0.445 ) ) + // or if minimizing GC, outerShape.moveToPoint( vector ); outerShape.moveTo( vector.x, vector.y ); + //REVIEW: only 5 times is necessary? _.times( 6, function() { + //REVIEW: for(i): outerShape.moveToPoint( Vector2.createPolar( outerMaxRadius, Math.PI * 0.445 + i * Math.PI / 3 ) ) + // or if minimizing GC, remove the closure (bad for GC) and outerShape.lineToPoint( vector ) vector.rotate( Math.PI / 3 ); outerShape.lineTo( vector.x, vector.y ); } ); @@ -111,6 +118,9 @@ define( function( require ) { var imageMap = {}; imageMap[ Side.FRONT ][ CoinTermTypeID.X ] = coinXFrontImage; imageMap[ Side.BACK ][ CoinTermTypeID.X ] = coinXBackImage; + + This has the advantage of also returning shared nodes, so that they are reused instead of being recreated for each + instance of a coin. */ switch( coinTermTypeID ) { @@ -170,12 +180,13 @@ define( function( require ) { * @returns {Node} * @param {CoinTermTypeID} coinTermTypeID * @param {number} radius - * @param {Object} options + * @param {Object} options REVIEW: never used? Can be removed? */ createIconNode: function( coinTermTypeID, radius, options ) { var iconNode = null; + //REVIEW: See recommended pattern above instead of a switch? And sharing instances may be helpful for memory. switch( coinTermTypeID ) { case CoinTermTypeID.X: @@ -218,7 +229,7 @@ define( function( require ) { radius, new Color( 221, 219, 219 ), radius * 0.7, - new Color( 206, 180, 44 ) + new Color( 206, 180, 44 ) //REVIEW: duplicated color. Should that be refactored out? ); break; diff --git a/js/common/view/CoinTermCreatorBox.js b/js/common/view/CoinTermCreatorBox.js index 5a5014e4..4a9ed3fe 100644 --- a/js/common/view/CoinTermCreatorBox.js +++ b/js/common/view/CoinTermCreatorBox.js @@ -35,13 +35,14 @@ define( function( require ) { staggeredCreatorNodes: false }, options ); - // @public, read only - a flag that indicates if creator nodes that create coin terms with negative initial values - // are present + // @public {boolean}, read only - a flag that indicates if creator nodes that create coin terms with negative + // initial values are present this.negativeTermsPresent = _.some( creatorNodes, function( creatorNode ) { return ( creatorNode.createdCoinTermInitialCount < 0 ); } ); - // @public, read only - list of the coin term types present in this creator box + // @public {Array.}, read only - list of the coin term types present in this creator box + //REVIEW: _.uniq( _.map( creatorNodes, 'typeID' ) ) this.coinTermTypeList = _.uniq( _.map( creatorNodes, function( creatorNode ) { return creatorNode.typeID; } @@ -49,6 +50,7 @@ define( function( require ) { // add the panel or carousel that will contain the various coin terms that the user can create if ( creatorNodes.length > options.itemsPerCarouselPage ) { + // @private {Node} this.coinTermCreatorBox = new Carousel( creatorNodes, { itemsPerPage: options.itemsPerCarouselPage, spacing: options.itemSpacing, @@ -62,6 +64,7 @@ define( function( require ) { spacing: options.itemSpacing, resize: false } ); + // @private {Node} this.coinTermCreatorBox = new Panel( coinTermCreatorHBox, { cornerRadius: options.cornerRadius, xMargin: 65, // empirically determined to be similar in appearance to carousels diff --git a/js/common/view/CoinTermCreatorBoxFactory.js b/js/common/view/CoinTermCreatorBoxFactory.js index 4f8aacc0..a523318d 100644 --- a/js/common/view/CoinTermCreatorBoxFactory.js +++ b/js/common/view/CoinTermCreatorBoxFactory.js @@ -54,6 +54,7 @@ define( function( require ) { ]; // helper function for making coin term creator nodes for the explore screens, which use a non-staggered format + //REVIEW: recommend normal function docs function makeExploreScreenCreatorNode( typeID, createdCoinTermInitialCount, model, view ) { // Create a property that will control number of coin terms shown in this creator node. For the explore screens, @@ -85,12 +86,15 @@ define( function( require ) { } // helper function for making creator nodes for the game screen, which uses a staggered format + //REVIEW: recommend normal function docs function makeGameScreenCreatorNode( typeID, createdCoinTermInitialCount, numInstancesAllowed, model, view ) { // Create a property that will control number of coin terms shown in this creator node. For the game screen, // multiple creator nodes are shown and a staggered arrangement. var numberToShowProperty = new Property( numInstancesAllowed ); var instanceCount = model.getCoinTermCountProperty( typeID, createdCoinTermInitialCount, true ); + + //REVIEW: Potential memory leak here, since it seems like this is called whenever there is a new challenge? instanceCount.link( function( count ) { numberToShowProperty.set( numInstancesAllowed - count ); } ); diff --git a/js/common/view/VariableCoinTermNode.js b/js/common/view/VariableCoinTermNode.js index 2923c969..36727425 100644 --- a/js/common/view/VariableCoinTermNode.js +++ b/js/common/view/VariableCoinTermNode.js @@ -129,6 +129,7 @@ define( function( require ) { // TODO: this was written with no thought given to performance, may need to optimize + //REVIEW: How is this different than scaling the entire node? This variable is used a lot to scale multiple things. var scale = coinTerm.scaleProperty.get(); // for convenience // control front coin image visibility diff --git a/js/game/view/EEGameScreenView.js b/js/game/view/EEGameScreenView.js index 795ee5bb..497ff44c 100644 --- a/js/game/view/EEGameScreenView.js +++ b/js/game/view/EEGameScreenView.js @@ -55,6 +55,7 @@ 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, diff --git a/js/game/view/ExpressionDescriptionNode.js b/js/game/view/ExpressionDescriptionNode.js index 809c59d8..a5c34686 100644 --- a/js/game/view/ExpressionDescriptionNode.js +++ b/js/game/view/ExpressionDescriptionNode.js @@ -57,6 +57,7 @@ define( function( require ) { } // 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, diff --git a/js/game/view/NextLevelNode.js b/js/game/view/NextLevelNode.js index abfafdf2..75e5810e 100644 --- a/js/game/view/NextLevelNode.js +++ b/js/game/view/NextLevelNode.js @@ -19,7 +19,8 @@ define( function( require ) { var nextString = require( 'string!EXPRESSION_EXCHANGE/next' ); /** - * @param {function} listener - function that gets called when 'next' button is pressed + * @param {Function} listener - function that gets called when 'next' button is pressed + * @param {Object} [options] * @constructor */ function NextLevelNode( listener, options ) { @@ -29,6 +30,7 @@ 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, diff --git a/js/game/view/StartGameLevelNode.js b/js/game/view/StartGameLevelNode.js index 628270f5..b2bf01fd 100644 --- a/js/game/view/StartGameLevelNode.js +++ b/js/game/view/StartGameLevelNode.js @@ -2,6 +2,8 @@ /** * 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. * * @author John Blanco */ @@ -27,14 +29,15 @@ define( function( require ) { var CONTROL_BUTTON_TOUCH_AREA_DILATION = 4; /** - * @param {Function} startLevelFunction - Function used to initiate a game - * level, will be called with a zero-based index value. + * @param {Function} startLevelFunction - Function used to initiate a game level, will be called with a zero-based + * index value. * @param {Function} resetFunction - Function to reset game and scores. - * @param {Property} soundEnabledProperty - * @param {Array} iconNodes - Set of iconNodes to use on the buttons, sizes - * should be the same, length of array must match number of levels. - * @param {Array} scores - Current scores, used to decide which stars to - * illuminate on the level start buttons, length must match number of levels. + * @param {Property.} soundEnabledProperty + * @param {Array.} iconNodes - Set of iconNodes to use on the buttons, sizes should be the same, length of array + * must match number of levels. + * @param {Array.>} 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? * @param {Object} [options] - See code below for options and default values. * @constructor */ @@ -42,21 +45,23 @@ define( function( require ) { Node.call( this ); + //REVIEW: Options not really ever needed, should just be inlined (there is only one constructor call!) options = _.extend( { // defaults - numLevels: 4, + numLevels: 4, // REVIEW: Move actual value (EEGameModel.NUMBER_OF_LEVELS) from EEGameScreenView to here. titleString: chooseYourLevelString, maxTitleWidth: 500, - numStarsOnButtons: 5, - perfectScore: 10, + 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. buttonBackgroundColor: '#EDA891', - numButtonRows: 1, // For layout - controlsInset: 12, - size: EESharedConstants.LAYOUT_BOUNDS + 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 }, 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' ); } @@ -70,6 +75,7 @@ define( function( require ) { 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( @@ -82,6 +88,7 @@ define( function( require ) { baseColor: options.buttonBackgroundColor } ); + //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 ] ); } @@ -102,6 +109,7 @@ 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. @@ -124,6 +132,5 @@ define( function( require ) { expressionExchange.register( 'StartGameLevelNode', StartGameLevelNode ); - // Inherit from Node. return inherit( Node, StartGameLevelNode ); } ); diff --git a/js/game/view/UndoButton.js b/js/game/view/UndoButton.js index dd554be3..ac02a045 100644 --- a/js/game/view/UndoButton.js +++ b/js/game/view/UndoButton.js @@ -22,20 +22,17 @@ define( function( require ) { /** * @constructor - * {Object} options + * @param {Object} options */ function UndoButton( options ) { - options = _.extend( - { - xMargin: MARGIN, - yMargin: MARGIN, - baseColor: new Color( 'yellow' ), - cursor: 'pointer', - arrowFill: 'black' - }, - options - ); + options = _.extend( { + xMargin: MARGIN, + yMargin: MARGIN, + baseColor: new Color( 'yellow' ), + cursor: 'pointer', + arrowFill: 'black' + }, options ); // create the shape for the undo arrow var undoArrowShape = new Shape() @@ -43,14 +40,15 @@ define( function( require ) { .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 ) + //.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 ) + .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 } );