Partial review comments from one device

phetsims · May 24, 2017 · 0b6c970 · 0b6c970
1 parent b06a53e
commit 0b6c970
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 0 deletions.
diff --git a/js/common/view/CoinNodeFactory.js b/js/common/view/CoinNodeFactory.js
@@ -101,9 +101,17 @@ define( function( require ) {
      * @public
      */
     createImageNode: function( coinTermTypeID, radius, backOrFront ) {
+      //REVIEW: backOrFront with enumeration constants should either be refactored to a 'Side' enumeration, or a boolean
+      // like isFront or isBack.
 
       var coinNode = null;
 
+      /*
+      REVIEW: recommend a top-level map for most of the switch statement, e.g.:
+      var imageMap = {};
+      imageMap[ Side.FRONT ][ CoinTermTypeID.X ] = coinXFrontImage;
+      imageMap[ Side.BACK ][ CoinTermTypeID.X ] = coinXBackImage;
+       */
       switch( coinTermTypeID ) {
 
         case CoinTermTypeID.X:
@@ -136,6 +144,7 @@ define( function( require ) {
 
         case CoinTermTypeID.CONSTANT:
           // this should never be depicted as a coin, so add something garish so that we'll notice if it is
+          // REVIEW: Why does this exist instead of an assertion failure? Sounds like it should never be viewed?
           coinNode = new Circle( radius, {
             fill: 'pink',
             stroke: 'red',

diff --git a/js/common/view/VariableCoinTermNode.js b/js/common/view/VariableCoinTermNode.js
@@ -63,6 +63,7 @@ define( function( require ) {
     var flipStateProperty = new Property( showCoinValuesProperty.get() ? 1 : 0 );
 
     // add the images for the front and back of the coin
+    //REVIEW: It looks like this is creating Image nodes for every instance of this. Sharing Image nodes may reduce memory usage (and be faster?)
     var coinFrontImageNode = CoinNodeFactory.createImageNode( coinTerm.typeID, coinTerm.coinRadius, 'front' );
     var coinBackImageNode = CoinNodeFactory.createImageNode( coinTerm.typeID, coinTerm.coinRadius, 'back' );