Assorted cleanup and review issues, see #88

js/common/model/CoinTerm.js

@@ -59,25 +59,26 @@ define( function( require ) {
     // properties
     //------------------------------------------------------------------------
 
-    // @public (read only), set using methods below
+    // @public {Property.<Vector2>} (read only), set using methods below
     this.positionProperty = new Property( options.initialPosition );
 
-    // @public (read only), set using methods below
+    // @public {Property.<Vector2>} (read only), set using methods below
     this.destinationProperty = new Property( options.initialPosition );
 
-    // @public, indicate whether user is currently dragging this coin
+    // @public {Property.<boolean>}, indicate whether user is currently dragging this coin
     this.userControlledProperty = new Property( false );
 
-    // @public
+    // @public {Property.<boolean>}
     this.combineHaloActiveProperty = new Property( false );
 
-    // @public, supports showing subtraction in expressions
+    // @public {Property.<boolean>}, supports showing subtraction in expressions
     this.showMinusSignWhenNegativeProperty = new Property( true );
 
-    // @public, indicates whether this is in a collection box (for game)
+    // @public {Property.<boolean>, 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.<????|null>} (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,13 +293,15 @@ define( function( require ) {
       }
     },
 
+    //REVIEW: doc
     returnToOrigin: function() {
       this.travelToDestination( this.initialPosition );
     },
 
     /**
      * 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,

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();

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;
     }

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;

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;
 

js/common/view/CoinTermCreatorBox.js

@@ -35,20 +35,22 @@ 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.<CoinTermTypeID>}, 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; }
     ) );
 
     // 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

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 );
     } );

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

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,

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,

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,