Added some //REVIEW markers, performed various code cleanup, see #29.

js/neuron-config.js

@@ -23,7 +23,7 @@ require.config(
       string: '../../chipper/requirejs-plugins/string',
 
       // common directories, uppercase names to identify them in require imports
-        AXON: '../../axon/js',
+      AXON: '../../axon/js',
       BRAND: '../../brand/js',
       DOT: '../../dot/js',
       JOIST: '../../joist/js',

js/neuron/chart/view/ChartCursor.js

@@ -21,7 +21,7 @@ define( function( require ) {
   var GrippyIndentNode = require( 'NEURON/neuron/chart/view/GrippyIndentNode' );
 
   // constants
-  var WIDTH_PROPORTION = 0.013;
+  var WIDTH_PROPORTION = 0.013; // empirically determined
   var CURSOR_FILL_COLOR = new Color( 50, 50, 200, 0.2 );
   var CURSOR_STROKE_COLOR = Color.DARK_GRAY;
 
@@ -32,10 +32,12 @@ define( function( require ) {
   function ChartCursor( membranePotentialChart ) {
 
     var thisChartCursor = this;
+    //REVIEW - The following comment appears to be in the wrong place.
     // Add a handler to the chart cursor that will track when it is moved
     // by the user and will set the model time accordingly.
 
-    //Chart Cursor
+    //REVIEW - Don't understand the following comment, and it doesn't seem to appear in the original Java code.
+    // Chart Cursor
     var topOfPlotArea = membranePotentialChart.chartMvt.modelToViewPosition( new Vector2( 0, membranePotentialChart.range[1] ) ); // UpperBound
     var bottomOfPlotArea = membranePotentialChart.chartMvt.modelToViewPosition( new Vector2( 0, membranePotentialChart.range[0] ) );//lowerBound
 
@@ -46,8 +48,13 @@ define( function( require ) {
     var height = bottomOfPlotArea.y - topOfPlotArea.y;
     var rectShape = new Shape().rect( -width / 2, 0, width, height );
 
-    Path.call( thisChartCursor, rectShape, { cursor: "e-resize", fill: CURSOR_FILL_COLOR, stroke: CURSOR_STROKE_COLOR, lineWidth: 0.4, lineDash: [4, 4]} );
-
+    Path.call( thisChartCursor, rectShape, {
+      cursor: "e-resize",
+      fill: CURSOR_FILL_COLOR,
+      stroke: CURSOR_STROKE_COLOR,
+      lineWidth: 0.4,
+      lineDash: [4, 4]
+    } );
 
     var chartCursorDragHandler = new SimpleDragHandler( {
 
@@ -62,9 +69,9 @@ define( function( require ) {
         this.pressTime = membranePotentialChart.chartMvt.viewToModelPosition( new Vector2( thisChartCursor.x, thisChartCursor.y ) ).x;
         membranePotentialChart.pausedWhenDragStarted = membranePotentialChart.clock.isPaused();
         if ( !membranePotentialChart.pausedWhenDragStarted ) {
-          // The user must be trying to grab the cursor while
-          // the recorded content is being played back.  Pause the
-          // clock.
+          // The user must be trying to grab the cursor while the sim is
+          // running or while recorded content is being played back.  Pause
+          // the clock.
           membranePotentialChart.setPaused( true );
         }
       },

js/neuron/chart/view/MembranePotentialChart.js

@@ -9,10 +9,10 @@
  * The user can clear the chart and trigger another action potential to start
  * recording data again.
  * <p/>
- * This chart also controls the record-and-playback state of the model.  This
- * is done so that the window of recorded data in the model matches that shown
- * in the chart, allowing the user to set the model state at any time shown in
- * the chart.
+ * This chart can also be used to control the record-and-playback state of the
+ * model.  This is done so that the window of recorded data in the model matches
+ * that shown in the chart, allowing the user to set the model state to any time
+ * shown in the chart.
  * <p/>
  *
  * @author John Blanco
@@ -61,6 +61,8 @@ define( function( require ) {
 
   var bounds2 = new Bounds2( 0, 0, 0, 0 );
 
+  //REVIEW - This seems odd - why is there a function called computeShapeBounds that does no computing and always
+  // returns a zero bounds?
   function computeShapeBounds() { return bounds2; }
 
   /**

js/neuron/model/MotionStrategy.js

@@ -25,7 +25,7 @@ define( function( require ) {
     /**
      * Move the associated model element according to the specified amount of
      * time and the nature of the motion strategy.  The fadable interface is
-     * also passed in, since it is possible for the motion stratagy to update
+     * also passed in, since it is possible for the motion strategy to update
      * the fade strategy.
      *
      * @param{Movable} movableModelElement

js/neuron/model/NeuronClockModelAdapter.js

@@ -1,10 +1,14 @@
 //  Copyright 2002-2014, University of Colorado Boulder
 
 /**
- * The clock for this simulation.
- * The simulation time change (dt) on each clock tick is constant,
- * regardless of when (in wall time) the ticks actually happen.
+ * The clock for this simulation. The simulation time change (dt) on each
+ * clock tick is constant, regardless of when (in wall time) the ticks
+ * actually happen.
  *
+ * Note: The whole approach of using explicit clocks and clock adapters is
+ * a holdover from PhET's Java days, and is present in this sim because the
+ * sim was ported from a Java version.  Use of this technique is not
+ * recommended for new HTML5/JavaScript simulations.
  *
  * @author Chris Malley ([email protected])
  * @author Sharfudeen Ashraf (for Ghent University)
@@ -22,7 +26,7 @@ define( function( require ) {
   var DEFAULT_FRAMES_PER_SECOND = 30.0;
 
   /**
-   * Constructor.
+   * @constructor
    * Creates a NeuronClock based on a number of requested frames per second.
    * This uses the same value for elapsed simulation time, and should be used for simulations
    * that run in real time (e.g. one sim time second equals one wall clock second)
@@ -60,6 +64,7 @@ define( function( require ) {
   }
 
   return inherit( PropertySet, NeuronClockModelAdapter, {
+
     step: function( dt ) {
       // step one frame, assuming 60fps
       if ( !this.isPaused() ) {
@@ -72,6 +77,7 @@ define( function( require ) {
         this.tick( this.simulationTimeChange );
       }
     },
+
     reset: function() {
       this.lastSimulationTime = 0.0;
       this.simulationTime = 0.0;
@@ -84,6 +90,7 @@ define( function( require ) {
       }
       this.model.reset();
     },
+
     /**
      * Registers a callback that will be notified when the step simulation occurs
      * Neuron Clock uses specialized real time step simulation
@@ -100,12 +107,10 @@ define( function( require ) {
       this.resetCallBacks.push( callback );
     },
 
-    setPaused: function( p ) {
-      this.paused = p;
-    },
-    isPaused: function() {
-      return this.paused;
-    },
+    setPaused: function( p ) { this.paused = p; },
+
+    isPaused: function() { return this.paused; },
+
     /**
      * Update the clock, updating the wall time and possibly simulation time.
      */
@@ -118,7 +123,9 @@ define( function( require ) {
 
 
     },
-// @private
+
+    // @private below this line
+
     fireChanged: function() {
       var changedCallbacks = this.changedCallbacks.slice( 0 ); // copy to prevent concurrent modification
       for ( var i = 0; i < changedCallbacks.length; i++ ) {
@@ -132,6 +139,7 @@ define( function( require ) {
     getDt: function() {
       this.getSimulationTimeChange();//This method uses Constant Time Strategy (The strategy interface itself is not exposed as in Java) Ashraf
     },
+
     getSimulationTimeChange: function( dt ) {
       return this.simulationTime - this.lastSimulationTime;
     },
@@ -143,6 +151,7 @@ define( function( require ) {
     getSimulationTimeChangeForPausedClock: function() {
       return this.simulationTimeChange;
     },
+
     setSimulationTimeNoUpdate: function( simulationTime ) {
       this.lastSimulationTime = this.simulationTime;
       this.simulationTime = simulationTime;
@@ -162,6 +171,4 @@ define( function( require ) {
       this.tick( -this.getSimulationTimeChangeForPausedClock() );
     }
   } );
-
-} )
-;
+} );

js/neuron/model/NeuronModel.js

@@ -57,7 +57,6 @@ define( function( require ) {
   var NUM_SODIUM_LEAK_CHANNELS = 3;
   var NUM_POTASSIUM_LEAK_CHANNELS = 7;
 
-
   // Nominal concentration values.
   var NOMINAL_SODIUM_EXTERIOR_CONCENTRATION = 145;     // In millimolar (mM)
   var NOMINAL_SODIUM_INTERIOR_CONCENTRATION = 10;      // In millimolar (mM)