From 0b6c970dc7e028ad8a4d24833f96e53ff4bd578c Mon Sep 17 00:00:00 2001
From: Jonathan Olson <olsonsjc@gmail.com>
Date: Tue, 23 May 2017 20:40:47 -0600
Subject: [PATCH] Partial review comments from one device

---
 js/common/view/CoinNodeFactory.js      | 9 +++++++++
 js/common/view/VariableCoinTermNode.js | 1 +
 2 files changed, 10 insertions(+)

diff --git a/js/common/view/CoinNodeFactory.js b/js/common/view/CoinNodeFactory.js
index 4e0d5d11..9affe8d5 100644
--- 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
index aca4bbc0..2923c969 100644
--- 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' );