Main review items for MASB, see #48

js/bounce/BounceScreen.js

@@ -19,7 +19,7 @@ define( require => {
   // strings
   const screenBounceString = require( 'string!MASSES_AND_SPRINGS_BASICS/screen.bounce' );
 
-  // image
+  // images
   const bounceHomeScreenImage = require( 'image!MASSES_AND_SPRINGS_BASICS/bounce_screen_icon.png' );
 
   class BounceScreen extends Screen {
@@ -49,6 +49,7 @@ define( require => {
           model.addDefaultMasses( modelTandem );
           return model;
         },
+        // REVIEW: In general, do we need `tandem.createTandem( 'view' )`?
         model => { return new BounceScreenView( model, tandem ); },
         options
       );

js/bounce/view/BounceScreenView.js

@@ -22,7 +22,7 @@ define( require => {
 
   class BounceScreenView extends TwoSpringScreenView {
     /**
-     * @param {VectorsModel} model
+     * @param {VectorsModel} model REVIEW: VectorsModel is a projectile-motion thing?
      * @param {Tandem} tandem
      */
     constructor( model, tandem ) {
@@ -101,6 +101,8 @@ define( require => {
         rectY: this.modelViewTransform.modelToViewY( MassesAndSpringsConstants.FLOOR_Y ) - this.shelf.rectHeight
       } );
       this.addChild( labeledMassesShelf );
+      // REVIEW: Rather than moving things to the very back, consider having layer Nodes on the supertype, so that
+      // REVIEW: these nodes can be directly added in the correct place. (If that is inconvenient, this is fine)
       labeledMassesShelf.moveToBack();
 
       const mysteryMassesShelf = new Shelf( tandem, {

js/common/view/LineOptionsNode.js

@@ -35,7 +35,7 @@ define( require => {
      * @param {MassesAndSpringsModel} model
      * @param {Tandem} tandem
      * @param {Object} options
-     * @constructor
+     * @constructor - REVIEW: We aren't using @constructor annotations for ES6 code
      */
     constructor( model, tandem, options ) {
 
@@ -49,6 +49,7 @@ define( require => {
       super( options );
 
       // Lines added for reference in panel
+      // REVIEW: No need to wrap these with Color objects (can pass the strings in directly)
       const greenLine = MassesAndSpringsConstants.CREATE_LINE_ICON( new Color( 'rgb(0, 180, 0)' ), tandem.createTandem( 'greenLine' ) );
       const blueLine = MassesAndSpringsConstants.CREATE_LINE_ICON( new Color( 'rgb( 65, 66, 232 )' ), tandem.createTandem( 'blueLine' ) );
       const redLine = MassesAndSpringsConstants.CREATE_LINE_ICON( new Color( 'rgb( 255, 0, 0 )' ), tandem.createTandem( 'redLine' ) );

js/lab/LabScreen.js

@@ -28,7 +28,7 @@ define( require => {
      * @param {Tandem} tandem
      * @param {object} options
      *
-     * @constructor
+     * @constructor - REVIEW: We aren't using @constructor annotations for ES6 code
      */
     constructor( tandem, options ) {
 

js/lab/view/GravityAccordionBox.js

@@ -38,7 +38,7 @@ define( require => {
      * @param {Tandem} tandem
      * @param {Object} [options]
      *
-     * @constructor
+     * @constructor - REVIEW: We aren't using @constructor annotations for ES6 code
      */
     constructor( model, listNodeParent, alignGroup, tandem, options ) {
 
@@ -63,7 +63,7 @@ define( require => {
       } );
       gravitySlider.addMajorTick( MassesAndSpringsConstants.GRAVITY_RANGE.min, new Text( noneString, {
         font: MassesAndSpringsConstants.LABEL_FONT,
-        tandem: tandem.createTandem( 'gravityNoneString' ),
+        tandem: tandem.createTandem( 'gravityNoneString' ), // REVIEW: Tandem should be for the 'Text', not a 'String'?
         maxWidth: MAX_WIDTH * 0.5
       } ) );
       gravitySlider.addMajorTick( MassesAndSpringsConstants.GRAVITY_RANGE.max, new Text( lotsString, {

js/lab/view/LabScreenView.js

@@ -33,7 +33,7 @@ define( require => {
      * @param {LabModel} model
      * @param {Tandem} tandem
      *
-     * @constructor
+     * @constructor - REVIEW: We aren't using @constructor annotations for ES6 code
      */
     constructor( model, tandem ) {
 
@@ -86,6 +86,7 @@ define( require => {
       this.addChild( shelf );
       shelf.moveToBack();
 
+      // REVIEW: No visibility annotations on consts
       // @public {ReferenceLineNode} Initializes equilibrium line for an attached mass
       const equilibriumLineNode = new ReferenceLineNode(
         this.modelViewTransform,
@@ -128,6 +129,7 @@ define( require => {
       );
       this.addChild( centerOfOscillationLineNode );
 
+      // REVIEW: Local variables shouldn't have visibility
       // @public {MassValueControlPanel} Accessed in Basics version to adjust to a larger width.
       const massValueControlPanel = new MassValueControlPanel(
         model.masses[ 0 ],
@@ -143,7 +145,7 @@ define( require => {
       ] );
       this.springSystemControlsNode.spacing = this.spacing * 1.2;
 
-      // @protected {PeriodTraceNode}
+      // @protected {PeriodTraceNode} REVIEW: If protected, is there a subtype of this referencing periodTraceNode?
       this.periodTraceNode = new PeriodTraceNode( model.periodTrace, this.modelViewTransform, model.options.basicsVersion, {
         center: this.massEquilibriumLineNode.center
       } );
@@ -166,8 +168,8 @@ define( require => {
     }
 
     /**
-     *
      * @param {number} dt
+     * REVIEW: Visibility (public presumably)?
      */
     step( dt ) {
       this.periodTraceNode.step( dt, this.model.playingProperty );

js/stretch/StretchScreen.js

@@ -27,7 +27,7 @@ define( require => {
      * @param {Tandem} tandem
      * @param {object} options
      *
-     * @constructor
+     * @constructor - REVIEW: We aren't using @constructor annotations for ES6 code
      */
     constructor( tandem, options ) {
 

js/stretch/view/StretchScreenView.js

@@ -26,7 +26,7 @@ define( require => {
     /**
      * @param {VectorsModel} model
      * @param {Tandem} tandem
-     * @constructor
+     * @constructor - REVIEW: We aren't using @constructor annotations for ES6 code
      */
     constructor( model, tandem ) {
 
@@ -64,6 +64,9 @@ define( require => {
       );
 
       // Setting specific colors for this screen's springs.
+      // REVIEW: This is setting the value of PaintColorProperty, which is generally meant only for it internally to do
+      // REVIEW: itself. It's effectively making e.g. springFrontColorProperty into a Property.<PaintColorProperty>,
+      // REVIEW: but it is documented as {PaintColorProperty} itself.
       this.springFrontColorProperty.set( new PaintColorProperty( 'rgb( 162, 106, 172 )' ) );
       this.springMiddleColorProperty.set( new PaintColorProperty( 'rgb( 100, 6, 117 )' ) );
       this.springBackColorProperty.set( new PaintColorProperty( 'rgb( 50, 3, 58 )' ) );