addressed several REVIEW comments, see #88

js/common/view/CoinTermIconNode.js

@@ -16,6 +16,7 @@ define( function( require ) {
   var MathSymbolFont = require( 'SCENERY_PHET/MathSymbolFont' );
   var Node = require( 'SCENERY/nodes/Node' );
   var PhetFont = require( 'SCENERY_PHET/PhetFont' );
+  var Property = require( 'AXON/Property' );
   var RichText = require( 'SCENERY_PHET/RichText' );
   var Text = require( 'SCENERY/nodes/Text' );
   var ViewMode = require( 'EXPRESSION_EXCHANGE/common/enum/ViewMode' );
@@ -32,7 +33,7 @@ define( function( require ) {
    * @param {Property.<ViewMode>} viewModeProperty - controls whether to show the coin or the term
    * @param {Property.<boolean>} showCoinValuesProperty - controls whether or not coin value is shown
    * @param {Property.<boolean>} showVariableValuesProperty - controls whether or not variable values are shown
-   * @param {Object} options //REVIEW: single usage doesn't use this, shouldn't be marked as required
+   * @param {Object} [options]
    * @constructor
    */
   function CoinTermIconNode( coinTerm, viewModeProperty, showCoinValuesProperty, showVariableValuesProperty, options ) {
@@ -56,14 +57,9 @@ define( function( require ) {
     this.addChild( coinValueText );
 
     // control the coin value text visibility
-    //REVIEW: One use, presumably inline
-    var coinValueVisibleProperty = new DerivedProperty(
-      [ viewModeProperty, showCoinValuesProperty ],
-      function( viewMode, showCoinValues ) {
-        return ( viewMode === ViewMode.COINS && showCoinValues );
-      }
-    );
-    coinValueVisibleProperty.linkAttribute( coinValueText, 'visible' );
+    Property.multilink( [ viewModeProperty, showCoinValuesProperty ], function( viewMode, showCoinValues ) {
+      coinValueText.visible = viewMode === ViewMode.COINS && showCoinValues;
+    } );
 
     // determine the max width of the textual components
     var maxTextWidth = coinIconNode.width * MAX_TERM_WIDTH_PROPORTION;
@@ -73,7 +69,6 @@ define( function( require ) {
       font: coinTerm.isConstant ? CONSTANT_FONT : VARIABLE_FONT,
       maxWidth: maxTextWidth
     } );
-    //REVIEW: Curious why this isn't included in the coinTerm.termText?
     if ( coinTerm.totalCountProperty.get() < 0 ) {
       termText.text = '-' + termText.text;
     }
@@ -96,27 +91,18 @@ define( function( require ) {
     } );
     this.addChild( termWithVariableValuesText );
 
-    // create a helper function to update the term value text
-    //REVIEW: single use, presumably inline?
-    function updateTermValueText() {
+    // update the variable text when it changes, which is triggered by changes to the underlying variable values
+    coinTerm.termValueTextProperty.link( function() {
       var termValueText = coinTerm.termValueTextProperty.value;
       var sign = coinTerm.totalCountProperty.get() > 0 ? '' : '-';
       termWithVariableValuesText.text = sign + termValueText;
       termWithVariableValuesText.center = coinCenter;
-    }
-
-    // update the variable text when it changes, which is triggered by changes to the underlying variable values
-    coinTerm.termValueTextProperty.link( updateTermValueText );
+    } );
 
     // control the visibility of the value text
-    //REVIEW Single use, but with a link. Presumably multilink instead of creating a derived property?
-    var variableTextVisibleProperty = new DerivedProperty(
-      [ viewModeProperty, showVariableValuesProperty ],
-      function( viewMode, showVariableValues ) {
-        return ( viewMode === ViewMode.VARIABLES && showVariableValues );
-      }
-    );
-    variableTextVisibleProperty.linkAttribute( termWithVariableValuesText, 'visible' );
+    Property.multilink( [ viewModeProperty, showVariableValuesProperty ], function( viewMode, showVariableValues ) {
+      termWithVariableValuesText.visible = viewMode === ViewMode.VARIABLES && showVariableValues;
+    } );
 
     this.mutate( options );
   }

js/common/view/ConstantCoinTermNode.js

@@ -67,39 +67,6 @@ define( function( require ) {
       }
     }
 
-    // function that updates the text and repositions it
-    //REVIEW: only one use. Ideally inline?
-    function updateRepresentation() {
-
-      // update value text
-      valueText.setScaleMagnitude( constantCoinTerm.scaleProperty.get() );
-      if ( constantCoinTerm.showMinusSignWhenNegativeProperty.get() ){
-        valueText.text = constantCoinTerm.valueProperty.value * constantCoinTerm.totalCountProperty.value;
-      }
-      else{
-        valueText.text = Math.abs( constantCoinTerm.valueProperty.value * constantCoinTerm.totalCountProperty.value );
-      }
-
-      // update position
-      valueText.centerX = 0;
-      valueText.y = AbstractCoinTermNode.TEXT_BASELINE_Y_OFFSET * constantCoinTerm.scaleProperty.get();
-
-      // update the card background
-      self.cardLikeBackground.visible = false; // make sure card is invisible so it doesn't affect visible bounds
-      self.cardLikeBackground.setRectBounds( self.coinAndTextRootNode.visibleLocalBounds.dilated( 10 ) );
-      if ( constantCoinTerm.cardOpacityProperty.get() === 0 ) {
-        //REVIEW: This was just set to false above?
-        self.cardLikeBackground.visible = false;
-      }
-      else {
-        self.cardLikeBackground.visible = true;
-        self.cardLikeBackground.opacity = constantCoinTerm.cardOpacityProperty.get();
-      }
-
-      // update the bounds that are registered with the model
-      updateBoundsInModel();
-    }
-
     // update the representation when model properties that affect it change
     var updateRepresentationMultilink = Property.multilink(
       [
@@ -108,7 +75,36 @@ define( function( require ) {
         constantCoinTerm.cardOpacityProperty,
         constantCoinTerm.scaleProperty
       ],
-      updateRepresentation
+      function() {
+
+        // update value text
+        valueText.setScaleMagnitude( constantCoinTerm.scaleProperty.get() );
+        if ( constantCoinTerm.showMinusSignWhenNegativeProperty.get() ) {
+          valueText.text = constantCoinTerm.valueProperty.value * constantCoinTerm.totalCountProperty.value;
+        }
+        else {
+          valueText.text = Math.abs( constantCoinTerm.valueProperty.value * constantCoinTerm.totalCountProperty.value );
+        }
+
+        // update position
+        valueText.centerX = 0;
+        valueText.y = AbstractCoinTermNode.TEXT_BASELINE_Y_OFFSET * constantCoinTerm.scaleProperty.get();
+
+        // update the card background
+        self.cardLikeBackground.visible = false; // make sure card is invisible so it doesn't affect visible bounds
+        self.cardLikeBackground.setRectBounds( self.coinAndTextRootNode.visibleLocalBounds.dilated( 10 ) );
+        if ( constantCoinTerm.cardOpacityProperty.get() === 0 ) {
+          //REVIEW: This was just set to false above?
+          self.cardLikeBackground.visible = false;
+        }
+        else {
+          self.cardLikeBackground.visible = true;
+          self.cardLikeBackground.opacity = constantCoinTerm.cardOpacityProperty.get();
+        }
+
+        // update the bounds that are registered with the model
+        updateBoundsInModel();
+      }
     );
 
     this.disposeConstantCoinTermNode = function(){

js/common/view/EditExpressionButton.js

@@ -17,15 +17,16 @@ define( function( require ) {
    */
   function EditExpressionButton( options ) {
 
-    //REVIEW: Why not _.extend( options, { ... } ) here like usual, with all of the values above?
-    options = options || {};
+    options = _.extend( {}, options );
 
     // the following options can't be overridden
-    options.content = new FontAwesomeNode( 'exchange', { scale: 0.35 } ); // scale empirically determined
-    options.xMargin = 3; // empirically determined
-    options.yMargin = 5.5; // empirically determined
-    options.baseColor = 'white';
-    options.cursor = 'pointer';
+    options = _.extend( options, {
+      content: new FontAwesomeNode( 'exchange', { scale: 0.35 } ), // scale empirically determined
+      xMargin: 3, // empirically determined
+      yMargin: 5.5, // empirically determined
+      baseColor: 'white',
+      cursor: 'pointer'
+    } );
 
     RectangularPushButton.call( this, options );
   }

js/common/view/ExpressionExplorationScreenView.js

@@ -189,9 +189,7 @@ define( function( require ) {
           new Text( 'x', {
             font: new MathSymbolFont( 36 ),
             boundsMethod: 'accurate',
-            //REVIEW: center: coinImageNode.leftCenter
-            centerX: 0,
-            centerY: coinImageNode.height / 2
+            center: coinImageNode.leftCenter
           } )
         ]
       } );
@@ -298,8 +296,6 @@ define( function( require ) {
     var resetAllButton = new ResetAllButton( {
       listener: function() {
         model.reset();
-        //REVIEW: Nice to usually have reset functions on view types themselves if they need reset, so we don't need to
-        // reach into their properties.
         myCollectionAccordionBox.expandedProperty.reset();
         totalValueAccordionBox.expandedProperty.reset();
         if ( coinTermCreatorBox.pageNumberProperty ) {
@@ -318,18 +314,16 @@ define( function( require ) {
     this.visibleBoundsProperty.link( function( visibleBounds ) {
 
       // update the positions of the floating controls
-      // REVIEW: use bounds.left/bounds.right
-      totalValueAccordionBox.left = visibleBounds.minX + FLOATING_PANEL_INSET;
-      variableValuesAccordionBox.left = visibleBounds.minX + FLOATING_PANEL_INSET;
-      myCollectionAccordionBox.right = visibleBounds.maxX - FLOATING_PANEL_INSET;
+      totalValueAccordionBox.left = visibleBounds.left + FLOATING_PANEL_INSET;
+      variableValuesAccordionBox.left = visibleBounds.left + FLOATING_PANEL_INSET;
+      myCollectionAccordionBox.right = visibleBounds.right - FLOATING_PANEL_INSET;
       showCoinValuesCheckbox.left = myCollectionAccordionBox.left;
       showVariableValuesCheckbox.left = myCollectionAccordionBox.left;
       showAllCoefficientsCheckbox.left = myCollectionAccordionBox.left;
       if ( showSubtractionCheckbox ) {
         showSubtractionCheckbox.left = myCollectionAccordionBox.left;
       }
-      //REVIEW: bounds.right
-      resetAllButton.right = visibleBounds.maxX - FLOATING_PANEL_INSET;
+      resetAllButton.right = visibleBounds.right - FLOATING_PANEL_INSET;
     } );
 
     // Add the layer where the coin terms and expressions will be shown, done here so that coin terms and expressions

js/common/view/ExpressionHintNode.js

@@ -59,17 +59,16 @@ define( function( require ) {
         if ( anchorCoinTermOnLeft ) {
           leftHalfWidth = anchorCTBounds.width + 2 * INSET;
           rightHalfWidth = movingCTBounds.width + 2 * INSET;
-          leftHalfCenterX = expressionHint.anchorCoinTerm.positionProperty.get().x +
-                            ( anchorCTBounds.minX + anchorCTBounds.maxX ) / 2; //REVIEW: bounds.centerX?
+          leftHalfCenterX = expressionHint.anchorCoinTerm.positionProperty.get().x + anchorCTBounds.centerX;
         }
         else { // anchor coin term is on the right
           leftHalfWidth = movingCTBounds.width + 2 * INSET;
           rightHalfWidth = anchorCTBounds.width + 2 * INSET;
-          //REVIEW: use bounds.centerX
-          leftHalfCenterX = expressionHint.anchorCoinTerm.positionProperty.get().x + ( anchorCTBounds.minX + anchorCTBounds.maxX ) / 2 - anchorCTBounds.width / 2 - INSET - movingCTBounds.width / 2 - INSET;
+          leftHalfCenterX =                                                                 expressionHint.anchorCoinTerm.positionProperty.get().x + anchorCTBounds.centerX
+                            - anchorCTBounds.width / 2 - INSET - movingCTBounds.width / 2 - INSET;
         }
 
-        //REVIEW: Looks like a rectangle. Any reason why we can't use the shortcuts?
+        // draw rectangle on three sides with zig-zag line on remaining side
         var leftHalfShape = new Shape()
           .moveTo( leftHalfWidth, 0 )
           .lineTo( 0, 0 )
@@ -85,8 +84,7 @@ define( function( require ) {
         } );
         self.addChild( leftHalf );
 
-        // add right half
-        //REVIEW: Looks like a rectangle. Any reason why we can't use the shortcuts?
+        // draw rectangle on three sides with zig-zag line on remaining side
         var rightHalfShape = new Shape()
           .moveTo( 0, 0 )
           .lineTo( rightHalfWidth, 0 )

js/common/view/ExpressionManipulationView.js

@@ -65,9 +65,7 @@ define( function( require ) {
     model.collectionAreas.forEach( function( collectionArea ) {
       var ejectButton = new UndoButton( {
         listener: function() { collectionArea.ejectCollectedItem(); },
-        //REVIEW: leftTop: collectionArea.bounds.leftTop
-        left: collectionArea.bounds.minX,
-        top: collectionArea.bounds.minY
+        leftTop: collectionArea.bounds.leftTop
       } );
       self.addChild( ejectButton );
 
@@ -105,22 +103,14 @@ define( function( require ) {
 
     // define a function that will update the shape of the barrier rectangle
     function updateBarrierRectangle() {
-      //REVIEW: barrierRectangleShape = Shape.bounds( barrierRectangleBounds )
-      barrierRectangleShape = Shape.rect(
-        barrierRectangleBounds.minX,
-        barrierRectangleBounds.minY,
-        barrierRectangleBounds.maxX - barrierRectangleBounds.minX,
-        barrierRectangleBounds.maxY - barrierRectangleBounds.minY
-      );
+      barrierRectangleShape = Shape.bounds( barrierRectangleBounds );
       if ( model.expressionBeingEditedProperty.get() ) {
         var barrierRectangleHoleBounds = model.expressionBeingEditedProperty.get().getBounds();
         // note - must travel counterclockwise to create a hole
         barrierRectangleShape.moveTo( barrierRectangleHoleBounds.minX, barrierRectangleHoleBounds.minY );
         barrierRectangleShape.lineTo( barrierRectangleHoleBounds.minX, barrierRectangleHoleBounds.maxY );
         barrierRectangleShape.lineTo( barrierRectangleHoleBounds.maxX, barrierRectangleHoleBounds.maxY );
         barrierRectangleShape.lineTo( barrierRectangleHoleBounds.maxX, barrierRectangleHoleBounds.minY );
-        //REVIEW: Why the moveTo before the close? The close would now do nothing, and could be removed
-        barrierRectangleShape.moveTo( barrierRectangleHoleBounds.minX, barrierRectangleHoleBounds.minY );
         barrierRectangleShape.close();
       }
       barrierRectanglePath.setShape( barrierRectangleShape );
@@ -259,14 +249,11 @@ define( function( require ) {
      * get the view node for the provided coin term model element
      * @param {CoinTerm} coinTerm
      * @returns {AbstractCoinTermNode}
+     * @public
      */
     getViewForCoinTerm: function( coinTerm ) {
-      //REVIEW: _.find( this.coinTermLayer.children, function( coinTermNode ) { return coinTermNode.coinTerm === coinTerm; } );
-      var coinTermView = null;
-      this.coinTermLayer.getChildren().forEach( function( coinTermNode ) {
-        if ( coinTermNode.coinTerm && coinTermNode.coinTerm === coinTerm ) {
-          coinTermView = coinTermNode;
-        }
+      var coinTermView = _.find( this.coinTermLayer.children, function( coinTermNode ) {
+        return coinTermNode.coinTerm === coinTerm;
       } );
       return coinTermView;
     }