More code cleanup and review tags. Part of code review, see #29.

doc/implementation-notes.txt

@@ -7,3 +7,8 @@ would likely have been done in a very different manner if this had been written
 important to keep this in mind when maintaining this code, since otherwise there are likely to be a lot of moments
 where the maintainer is thinking things like, "Why the heck did they do this? I'm going to do it differently!", thus
 ending up with a mish-mash of coding styles.
+
+One particular area where this Java vs. JavaScript contrast is apparent is in the use of getters.  In the Java code,
+there were many private variables that were accessed through getters.  For the most part, these getters were retained,
+even though there is no such thing as a private member variable in JavaScript.  These getter functions look a little
+weird in this context, but keeping them made the port easier.

js/neuron/model/MembraneChannel.js

@@ -41,22 +41,27 @@ define( function( require ) {
     var thisChannel = this;
 
     PropertySet.call( thisChannel, {
+      //REVIEW - I was able to figure out what the following two props were all about by looking through the code, but
+      // they could use a bit more documentation to make it clear why they're here.
       channelStateChanged: false,
       representationChanged: false // All the channel states are  updated at once at the end stepInTime.This was done for performance reasons.
     } );
 
     // position of the channel
     this.centerLocation = new Vector2();
-    // Variable that defines how open the channel is.Valid range is 0 to 1, 0 means fully closed, 1 is fully open.
+
+    // Variable that defines how open the channel is. Valid range is 0 to 1, 0 means fully closed, 1 is fully open.
     this.openness = 0;
+
     // Variable that defines how inactivated the channel is, which is distinct from openness.
     // Valid range is 0 to 1, 0 means completely active, 1 is completely inactive.
     this.inactivationAmt = 0;
 
     // Reference to the model that contains that particles that will be moving through this channel.
     thisChannel.modelContainingParticles = modelContainingParticles;
     thisChannel.rotationalAngle = 0; // In radians.
-    // Size of channel only, i.e. where the atoms pass through.
+
+    // Size of channel interior, i.e. where the atoms pass through.
     thisChannel.channelSize = new Dimension2( channelWidth, channelHeight );
     thisChannel.overallSize = new Dimension2( channelWidth * 2.1, channelHeight * SIDE_HEIGHT_TO_CHANNEL_HEIGHT_RATIO );
 
@@ -66,31 +71,31 @@ define( function( require ) {
     // developer's responsibility to position the channel appropriately on the
     // cell membrane.
 
-    // Set the "capture zone", which is a shape that represents the space
-    // from which particles may be captured.  If null is returned, this
+    // Set the initial capture zone, which is a shape that represents the
+    // space from which particles may be captured.  If null is returned, this
     // channel has no capture zone.
     this.interiorCaptureZone = new NullCaptureZone();
-
     this.exteriorCaptureZone = new NullCaptureZone();
 
     // Time values that control how often this channel requests an ion to move
     // through it.  These are initialized here to values that will cause the
-    // channel to never request any ions and must be set by the base classes
-    // in order to make capture events occur.
+    // channel to never request any ions and must be set by the descendant
+    // classes in order to make capture events occur.
     this.captureCountdownTimer = Number.POSITIVE_INFINITY;
     this.minInterCaptureTime = Number.POSITIVE_INFINITY;
     this.maxInterCaptureTime = Number.POSITIVE_INFINITY;
 
     // Velocity for particles that move through this channel.
     this.particleVelocity = DEFAULT_PARTICLE_VELOCITY;
-    this.updateChannelRect();
 
+    // Perform the initial update the shape of the channel rectangle.
+    this.updateChannelRect();
   }
 
   return inherit( PropertySet, MembraneChannel, {
 
     /**
-     * Implements the time-dependent behavior of the gate.
+     * Implements the time-dependent behavior of the channel.
      * @param dt - Amount of time step, in milliseconds.
      */
     stepInTime: function( dt ) {
@@ -139,6 +144,7 @@ define( function( require ) {
     },
     // Determine whether the provided point is inside the channel.
     isPointInChannel: function( x, y ) {
+      //REVIEW - this comment seems obsolete.  Agreed?  If so, please remove.
       // Note: A rotational angle of zero is considered to be lying on the
       // side.  Hence the somewhat odd-looking use of height and width in
       // the determination of the channel Rect.
@@ -297,6 +303,7 @@ define( function( require ) {
       return new MembraneChannelState( this );
     },
 
+    //REVIEW - non-conventional header comment.
     /*
      The Membrane Channel Node observed centerLocation,openness and inactivation
      properties separately.This resulted in too many updates to node and degraded the performance.This method checks

js/neuron/model/MembraneChannelFactory.js

@@ -1,6 +1,6 @@
 // Copyright 2002-2011, University of Colorado
 /**
- * Factory to create different types pf  MembraneChannels
+ * Factory to create different types of MembraneChannels
  *
  * @author John Blanco
  * @author Sharfudeen Ashraf (for Ghent University)
@@ -17,8 +17,8 @@ define( function( require ) {
   var SodiumDualGatedChannel = require( 'NEURON/neuron/model/SodiumDualGatedChannel' );
 
   return{
-    //factory method for creating a MembraneChannel of the specified type.
     /**
+     * factory method for creating a MembraneChannel of the specified type.
      * @param {MembraneChannelTypes} channelType
      * @param {ParticleCapture} particleModel
      * @param {HodgkinHuxleyModel} hodgkinHuxleyModel

js/neuron/model/MembraneTraversalMotionStrategy.js

@@ -20,6 +20,7 @@ define( function( require ) {
 
   }
 
+  //REVIEW: The following can be placed in the
   MembraneTraversalMotionStrategy.DEFAULT_MAX_VELOCITY = 40000;
   return  inherit( MotionStrategy, MembraneTraversalMotionStrategy, {
 

js/neuron/model/ModifiedHodgkinHuxleyModel.js

@@ -5,6 +5,8 @@
  * this) but that was modified significantly to fit the needs of this
  * simulation.  The main change is that the way that the conductance values
  * are calculated is different, and much simpler.
+ * <p/>
+ * This was used with permission from the original author of the example.
  *
  * @author Anthony Fodor
  * @author John Blanco
@@ -43,7 +45,7 @@ define( function( require ) {
 
     thisModel.resting_v = 65;// final doesn't change
 
-    // deltas of voltage-dependent gating paramaters
+    // deltas of voltage-dependent gating parameters
     thisModel.dn = 0;
     thisModel.dm = 0;
     thisModel.dh = 0;
@@ -82,7 +84,7 @@ define( function( require ) {
       this.vl = 0; // NOTE: Modified from -10.613 by jblanco on 3/12/2010 in order to make potential stay steady
       // at the desired resting potential.
 
-      //constant leak permeabilties
+      //constant leak permeabilities
       this.gna = this.perNaChannels * 120 / 100;
       this.gk = this.perKChannels * 36 / 100;
       this.gl = 0.3;

js/neuron/model/NeuronClockModelAdapter.js

@@ -4,12 +4,12 @@
  * 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.
- *
+ * <p/>
  * 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.
- *
+ * <p/>
  * @author Chris Malley ([email protected])
  * @author Sharfudeen Ashraf (for Ghent University)
  */

js/neuron/model/NeuronModel.js

@@ -1,9 +1,7 @@
 //  Copyright 2002-2014, University of Colorado Boulder
 /**
- * Model for the 'Neuron' screen.
- * This class represents the main class for modeling the axon.  It acts as the
- * central location where the interaction between the membrane, the particles
- * (i.e. ions), and the gates is all governed.
+ * Model for the 'Neuron' screen. This class represents the main class for modeling the axon.  It acts as the central
+ * location where the interaction between the membrane, the particles (i.e. ions), and the gates is all governed.
  *
  * @author John Blanco
  * @author Sam Reid (PhET Interactive Simulations)
@@ -117,7 +115,7 @@ define( function( require ) {
 
 
   /**
-   * Main constructor for NeuronModel, which contains all of the model logic for the entire sim screen.
+   * Main constructor for NeuronModel, which contains much of the model logic for the sim.
    * @constructor
    */
   function NeuronModel() {
@@ -350,6 +348,7 @@ define( function( require ) {
       // Step the transient particles.  Since these particles may remove
       // themselves as a result of being stepped, we need to copy the list
       // in order to avoid concurrent modification exceptions.
+      //REVIEW - Good catch.  I think it's safe, and less confusing to future maintainers, to simply remove this comment.
       // (Not true in observableArray as ForEach already copies the array and iterates, kept the comment for ref - Ashraf)
 
       this.transientParticles.forEach( function( particle ) {
@@ -830,7 +829,7 @@ define( function( require ) {
       }
       else {
         // Currently NOT locked out, see if that should change.
-        var backwards = this.getTime() - this.getMaxRecordedTime() > 0 ? false : true;
+        var backwards = this.getTime() - this.getMaxRecordedTime() <= 0;
 
 
         if ( this.isActionPotentialInProgress() || (this.isPlayback() && backwards) ) {
@@ -948,6 +947,7 @@ define( function( require ) {
 
       newParticle.continueExistingProperty.lazyLink( function( newValue ) {
         if ( newValue === false ) {
+          //REVIEW: This probably won't work, should be a 'self' var set at top of function.
           this.backgroundParticles.remove( newParticle );
         }
       } );
@@ -1086,7 +1086,7 @@ define( function( require ) {
       // concentration changes.
       this.concentrationChanged = true;
 
-      //If any one channel's state is changed, trigger a channel representation changed event
+      // If any one channel's state is changed, trigger a channel representation changed event
       this.channelRepresentationChanged = false;
       this.channelRepresentationChanged = _.any( this.membraneChannels.getArray(), 'channelStateChanged' );
     }

js/neuron/model/Particle.js

@@ -1,10 +1,10 @@
 // Copyright 2002-2011, University of Colorado
 
 /**
- * Abstract base class for a simulated particle.  It is intended that this be subclassed
- * for each specific particle type used in the simulation.
- * This class  serves as a Fadable  element  that can fade in or out of
- * existence in based on different  fade strategies.
+ * Abstract base class for a simulated particle.  It is intended that this be subclassed for each specific particle
+ * type used in the simulation.
+ *
+ * This class serves as a Fadable element that can fade in or out of existence in based on different fade strategies.
  * Also functions as a "Movable" element that can be move differently based on different motion strategies.
  *
  * @author John Blanco
@@ -45,11 +45,10 @@ define( function( require ) {
     this.positionX = xPos;
     this.positionY = yPos;
 
-    // Opaqueness value, ranges from 0 (completely transparent) to 1
-    // (completely opaque).
+    // Opaqueness value, ranges from 0 (completely transparent) to 1 (completely opaque).
     this.opaqueness = 1;
 
-    // Motion strategy for moving this particle around.StillnessMotionStrategy is stateless so use the singleton instance
+    // Motion strategy for moving this particle around. StillnessMotionStrategy is stateless so use the singleton instance.
     this.motionStrategy = StillnessMotionStrategy.getInstance();
 
     // Fade strategy for fading in and out.

js/neuron/model/ParticleFactory.js

@@ -2,6 +2,7 @@
 
 /**
  * Factory class for Particle
+ *
  * @Sharfudeen Ashraf (for Ghent University)
  */
 

js/neuron/model/PieSliceShapedCaptureZone.js

@@ -1,8 +1,8 @@
 // Copyright 2002-2011, University of Colorado
 
 /**
- * A "capture zone" (which is a 2D space that defines where particles may be
- * captured by a gate) that is shaped like a pie slice.
+ * A "capture zone" (which is a 2D space that defines where particles may be captured by a gate) that is shaped like a
+ * pie slice.
  *
  * @author John Blanco
  * @author Sharfudeen Ashraf (for Ghent University)

js/neuron/model/SlowBrownianMotionStrategy.js

@@ -3,7 +3,7 @@
 /**
  * A motion strategy for showing some slow Brownian motion, which is basically
  * just an occasional small jump from its initial location to a new nearby
- * location and then back.  This is intended to create noticable but
+ * location and then back.  This is intended to create noticeable but
  * non-distracting motion that doesn't consume much processor time.
  *
  * @author John Blanco

js/neuron/model/TimedFadeAwayStrategy.js

@@ -12,7 +12,6 @@ define( function( require ) {
 
 
   /**
-   *
    * @param {number} fadeTime - time, in seconds of sim time, for this to fade away.
    * @constructor
    */

js/neuron/model/TraverseChannelAndFadeMotionStrategy.js

@@ -1,9 +1,7 @@
 //  Copyright 2002-2014, University of Colorado Boulder
 
 /**
- * A motion strategy for traversing a basic membrane channel, i.e. one that
- * has only one gate, and then fading away.
- *
+ * A motion strategy for traversing a basic membrane channel (i.e. one that has only one gate) and then fading away.
  *
  * @author John Blanco
  * @author Sharfudeen Ashraf (for Ghent University)

js/neuron/model/ViewableParticle.js

@@ -2,6 +2,7 @@
 
 /**
  * Interface for a particle that can be viewed, i.e. displayed to the user.
+ *
  * @author John Blanco
  * @author Sharfudeen Ashraf (for Ghent University)
  */

js/neuron/model/WanderAwayThenFadeMotionStrategy.js

@@ -1,8 +1,7 @@
 //  Copyright 2002-2014, University of Colorado Boulder
 
 /**
- * A motion strategy that has a particle wander around for a while and then
- * fade out of existence.
+ * A motion strategy that has a particle wander around for a while and then fade out of existence.
  *
  * @author John Blanco
  * @author Sharfudeen Ashraf (for Ghent University)

js/neuron/view/AxonCrossSectionNode.js

@@ -1,4 +1,4 @@
-//// Copyright 2002-2011, University of Colorado
+// Copyright 2002-2011, University of Colorado
 
 /**
  * Representation of the transverse cross section of the axon the view.

js/neuron/view/BackgroundParticlesNode.js

@@ -1,5 +1,9 @@
 // Copyright 2002-2011, University of Colorado
 /**
+ * //REVIEW - is the comment below correct and up to date?  I'm seeing this used in ParticlesCanvasLayerNode.  Please
+ * either update the comment, remove the class, or add documentation that states why it should be kept around even if
+ * it isn't used.
+ *
  * For performance  reasons, there are multiple Background Particles canvas node each renders a subset of background particles and are rendered in a round robin fashion.
  * The assumption is since Background particles exhibit slow random brownian motion this way of rendering wont affect the realism
  * EXPERIMENTAL CLASS NOT USED because the WebGL version of Particle implementation  is found to be performing far better.

js/neuron/view/ChargeSymbolNode.js

@@ -35,6 +35,8 @@ define( function( require ) {
   function ChargeSymbolNode( axonModel, maxWidth, maxPotential, polarityReversed ) {
     var thisNode = this;
     Node.call( thisNode );
+    //REVIEW - Looks like usage for several of the vars below was moved into the constructor (very nice!) so they don't
+    // need to be retained as properties.
     this.axonModel = axonModel;
     this.maxWidth = maxWidth;
     this.polarityReversed = polarityReversed;

js/neuron/view/ChargeSymbolsLayerNode.js

@@ -16,9 +16,9 @@ define( function( require ) {
   var ChargeSymbolNode = require( 'NEURON/neuron/view/ChargeSymbolNode' );
 
   // Max size of the charge symbols, tweak as needed.
-  var MAX_CHARGE_SYMBOL_SIZE = 5;// was 8
+  var MAX_CHARGE_SYMBOL_SIZE = 5;
+
   /**
-   *
    * @param {NeuronModel} neuronModel
    * @param {ModelViewTransform2} mvt
    * @constructor

js/neuron/view/ConcentrationReadoutLayerNode.js

@@ -1,5 +1,6 @@
 //  Copyright 2002-2014, University of Colorado Boulder
 
+//REVIEW - header comment is missing
 /**
  *
  *

js/neuron/view/MembraneChannelGateCanvasNode.js

@@ -1,7 +1,8 @@
 // Copyright 2002-2011, University of Colorado
 /**
- * The Dynamic parts of the Membrane Channels namely Gate,Channel expansion,string are
- * rendered directly on a single canvas.
+ * The dynamic parts of the Membrane Channels, namely the Gate, Channel expansion, and string, are rendered directly on
+ * a single canvas for optimal performance.
+ *
  * @author John Blanco
  * @author Sharfudeen Ashraf (for Ghent University)
  */