Some initial REVIEW comments, see #317

js/model/Charge.js

@@ -43,6 +43,8 @@ define( function( require ) {
     this.circuitElement = circuitElement;
 
     // @private (read-only) {boolean} - whether the charge has been disposed, to aid in debugging
+    //REVIEW: If for debugging, can this be removed, or set only when assertions are enabled? Would help memory to remove, particularly since a lot
+    //REVIEW: of charges get created.
     this.deleted = false;
 
     // @public (read-only) {NumberProperty} - the distance the charge has traveled in its CircuitElement in view
@@ -51,19 +53,29 @@ define( function( require ) {
 
     // @public {BooleanProperty} - To improve performance, disable updating while the position of the charge is changed
     // many times during the update step.
+    //REVIEW: Presumably this can be removed, see note below for the multilink on proposed strategy
     this.updatingPositionProperty = new BooleanProperty( true );
 
     // @public (read-only) {Property.<Vector2>} - the 2d position of the charge
-    this.positionProperty = new Property( new Vector2() );
+    this.positionProperty = new Property( new Vector2() ); // REVIEW: Use Vector2.ZERO so we don't create extra vectors?
 
+    //REVIEW: It may be worse for memory (but better for simplicity/performance), but instead of an independent
+    //REVIEW: position/angle, it would be possible to have a matrixProperty that includes both. It looks like ChargeNode
+    //REVIEW: has to fully update its transform multiple times upon any change, as the position/angle changes both
+    //REVIEW: trigger a full updateTransform in ChargeNode.
     // @public (read-only) {NumberProperty} - the angle of the charge (for showing arrows)
     this.angleProperty = new NumberProperty( 0 );
 
     // @public (read-only) {BooleanProperty} - true if the Charge is on the right hand side of a light bulb and hence
     // must be layered in front of the socket node.
+    //REVIEW: I don't see where this is used (besides setting it). Can this be removed?
     this.onRightHandSideOfLightBulbProperty = new BooleanProperty( false );
 
     // When the distance or updating properties change, update the 2d position of the charge
+    //REVIEW: A multilink seems like overkill here, particularly since it's conditional. Furthermore, this looks like
+    //REVIEW: a function that should be a method (for performance and memory). Can we have an update() function or
+    //REVIEW: equivalent, and call it either when setLocation() is called or from ChargeAnimator's location where
+    //REVIEW: charges can then be updated? This should reduce calls to it, be a bit simpler, and have lower memory.
     var multilink = Property.multilink( [ this.distanceProperty, this.updatingPositionProperty ],
       function( distance, updating ) {
         if ( updating ) {
@@ -75,6 +87,8 @@ define( function( require ) {
           self.angleProperty.set( positionAndAngle.angle );
           self.positionProperty.set( position );
 
+          //REVIEW: Presumably this can get removed, so that the non-assertion parts of the function can just be:
+          //REVIEW: self.matrixProperty.set( self.circuitElement.getMatrix( distance ) );
           self.onRightHandSideOfLightBulbProperty.set(
             self.circuitElement instanceof LightBulb &&
             self.distanceProperty.get() > self.circuitElement.chargePathLength / 2
@@ -89,6 +103,8 @@ define( function( require ) {
     this.disposeEmitter = new Emitter();
 
     // @public (read-only) {function} for disposal
+    //REVIEW: We should not be creating copies of this function for objects that get created a lot, as it presumably
+    //REVIEW: increases the amount of memory used. This looks exactly like something that should be a method instead.
     this.disposeCharge = function() {
       assert && assert( !self.deleted, 'cannot delete twice' );
       multilink.dispose();
@@ -117,6 +133,7 @@ define( function( require ) {
      * @public
      */
     setLocation: function( circuitElement, distance ) {
+      //REVIEW: Are infinite distances allowed? isFinite() may be preferred over isNaN (but it's mostly a preference)
       assert && assert( !isNaN( distance ), 'Distance was NaN' );
       assert && assert( circuitElement.containsScalarLocation( distance ), 'no location in branch' );
       this.circuitElement = circuitElement;

js/model/Circuit.js

@@ -160,6 +160,7 @@ define( function( require ) {
         } );
       }
 
+      //REVIEW: Don't see a matching removeListener. Presumably it should be called before we dispose the element?
       circuitElement.moveToFrontEmitter.addListener( updateCharges );
       self.solve();
     } );

js/model/CircuitElement.js

@@ -38,7 +38,7 @@ define( function( require ) {
 
     var self = this;
 
-    // @public (read-only) - the tail of the Tandem for creating associated Tandems
+    // @public (read-only) {string} - the tail of the Tandem for creating associated Tandems
     this.tandemName = tandem.tail;
 
     // @public (read-only) {number} unique identifier for looking up corresponding views
@@ -73,13 +73,15 @@ define( function( require ) {
     this.currentProperty = new NumberProperty( 0 );
 
     // @public {BooleanProperty} - whether the CircuitElement is being dragged across the toolbox
+    //REVIEW: Don't see any read/link to this, maybe there is something I'm missing?
     this.isOverToolboxProperty = new BooleanProperty( false );
 
     // @public (read-only) {BooleanProperty} - true if the CircuitElement can be edited and dragged
     this.interactiveProperty = new BooleanProperty( options.interactive );
 
     // @public {BooleanProperty} - whether the circuit element is inside the true black box, not inside the user-created
     // black box, on the interface or outside of the black box
+    //REVIEW: Presumably this could be initialized properly on creation? Only setters are on deserialization.
     this.insideTrueBlackBoxProperty = new BooleanProperty( false );
 
     // @public {boolean} - true if the charge layout must be updated
@@ -107,13 +109,16 @@ define( function( require ) {
 
     // @public {Property.<number>} - the voltage at the end vertex minus the voltage at the start vertex
     // name voltageDifferenceProperty so it doesn't clash with voltageProperty in Battery subclass
+    //REVIEW: I don't see reads of this property, can it be removed?
     this.voltageDifferenceProperty = new Property( 0 );
 
     // Signify that a Vertex moved
+    //REVIEW: seems like it should be a method (bound to a property) for memory purposes. See notes below.
     var vertexMoved = function() {
       self.vertexMovedEmitter.emit();
     };
 
+    //REVIEW: seems like it should be a method (bound to a property) for memory purposes. See notes below.
     var vertexVoltageChanged = function() {
       self.voltageDifferenceProperty.set(
         self.endVertexProperty.get().voltageProperty.get() -
@@ -126,6 +131,8 @@ define( function( require ) {
      * @param {Vertex} newVertex - the new vertex
      * @param {Vertex} oldVertex - the previous vertex
      */
+    //REVIEW: Would be better as a method, so it doesn't create new function objects. Then bind it for the listeners
+    //REVIEW: below (and for the dispose method)
     var linkVertex = function( newVertex, oldVertex ) {
       oldVertex.positionProperty.unlink( vertexMoved );
       newVertex.positionProperty.link( vertexMoved );
@@ -137,17 +144,21 @@ define( function( require ) {
         self.vertexMovedEmitter.emit();
       }
     };
+
+    //REVIEW: The position properties of the vertex properties are used a ton. Maybe a getter for getStartPositionProperty()
+    //REVIEW: / getEndPositionProperty() would help make things more readable?
     this.startVertexProperty.get().positionProperty.link( vertexMoved );
     this.endVertexProperty.get().positionProperty.link( vertexMoved );
     this.startVertexProperty.get().voltageProperty.link( vertexVoltageChanged );
     this.endVertexProperty.get().voltageProperty.link( vertexVoltageChanged );
     this.startVertexProperty.lazyLink( linkVertex );
     this.endVertexProperty.lazyLink( linkVertex );
 
-    // @private {boolean} - for debugging
+    // @private {boolean} - for debugging REVIEW: Clarify "for debugging"? Can this now be removed (or only with assertions to reduce production memory?)
     this.disposed = false;
 
     // @private {function} - for disposal
+    //REVIEW: This should be part of dispose() as a method? It's creating extra closures for every element right now.
     this.disposeCircuitElement = function() {
       assert && assert( !self.disposed, 'Was already disposed' );
       self.disposed = true;
@@ -161,6 +172,7 @@ define( function( require ) {
       self.startVertexProperty.get().voltageProperty.hasListener( vertexVoltageChanged ) && self.startVertexProperty.get().voltageProperty.unlink( vertexVoltageChanged );
       self.endVertexProperty.get().voltageProperty.hasListener( vertexVoltageChanged ) && self.endVertexProperty.get().voltageProperty.unlink( vertexVoltageChanged );
 
+      //REVIEW: If listeners are getting notified that something will be disposed, presumably it should be before disposing inner components?
       self.disposeEmitter.emit();
       self.disposeEmitter.removeAllListeners();
     };
@@ -247,7 +259,12 @@ define( function( require ) {
     /**
      * Gets the 2D Position along the CircuitElement corresponding to the given scalar distance
      * @param {number} distanceAlongWire - the scalar distance from one endpoint to another.
-     * @returns {Vector2} the position in view coordinates
+     * @returns {Vector2} the position in view coordinates REVIEW: Definitely not returning a Vector2.
+     * REVIEW: I see no reason not to split this into two functions. Sometimes only one of the two things computed is
+     * REVIEW: used, and it wouldn't require creating another temporary object.
+     *
+     * REVIEW: If both are needed, can we just return a Matrix that has the position/angle information (assuming
+     * REVIEW: Charge switches to use a Matrix3 instead of position/angle independently)
      * @public
      */
     getPositionAndAngle: function( distanceAlongWire ) {
@@ -272,7 +289,7 @@ define( function( require ) {
     /**
      * Get all Property instances that influence the circuit dynamics.
      * @abstract must be specified by the subclass
-     * @returns {Property[]}
+     * @returns {Property[]} REVIEW: Type of Properties? Property.<Circuit>?
      * @public
      */
     getCircuitProperties: function() {
@@ -291,7 +308,7 @@ define( function( require ) {
     },
 
     /**
-     * Return the indices of the vertices, for debugging.
+     * Return the indices of the vertices, for debugging. REVIEW: Does this mean it can be removed? (See no usages)
      * @public
      * @returns {[number,number]}
      */

js/model/ModifiedNodalAnalysisCircuit.js

@@ -87,6 +87,7 @@ define( function( require ) {
   inherit( Object, ModifiedNodalAnalysisCircuit, {
 
     /**
+     * REVIEW: A lot of things are noted as for debugging. Can these be stripped out of the production build of the sim?
      * Returns a string representation of the circuit for debugging.
      * @returns {string}
      * @private

js/model/Vertex.js

@@ -26,14 +26,17 @@ define( function( require ) {
   var counter = 0;
 
   /**
+   * REVIEW: Most usages either use a Vector2, or could use Vector2.ZERO (and we duplicate initial vector references).
+   * REVIEW: Prefer Vector2 input instead of x,y
    * @param {number} x - position (screen coordinates) in x
    * @param {number} y - position (screen coordinates) in y
    * @param {Object} [options]
    * @constructor
    */
   function Vertex( x, y, options ) {
 
-    // @private - Index counter for debugging, can be shown with ?vertexDisplay=index
+    // @private {number} - Index counter for debugging, can be shown with ?vertexDisplay=index
+    //REVIEW: If this is for debugging, is it temporary (could be removed), or could only be enabled when assertions are enabled?
     this.index = counter++;
 
     options = _.extend( {