diff --git a/js/common/enum/ForcesModeChoice.js b/js/common/enum/ForcesModeChoice.js index 1eb43bea..f341ea32 100644 --- a/js/common/enum/ForcesModeChoice.js +++ b/js/common/enum/ForcesModeChoice.js @@ -12,5 +12,6 @@ define( function( require ) { var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' ); var Enumeration = require( 'PHET_CORE/Enumeration' ); + // REVIEW: The recent decision was to move enumerations out of enum/ folders. Can you put it in model/? return massesAndSprings.register( 'ForcesModeChoice', new Enumeration( [ 'FORCES', 'NET_FORCES' ] ) ); } ); \ No newline at end of file diff --git a/js/common/enum/SimSpeedChoice.js b/js/common/enum/SimSpeedChoice.js index b455e2ce..0b487fcf 100644 --- a/js/common/enum/SimSpeedChoice.js +++ b/js/common/enum/SimSpeedChoice.js @@ -12,5 +12,6 @@ define( function( require ) { var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' ); var Enumeration = require( 'PHET_CORE/Enumeration' ); + // REVIEW: The recent decision was to move enumerations out of enum/ folders. Can you put it in model/? return massesAndSprings.register( 'SimSpeedChoice', new Enumeration( [ 'NORMAL', 'SLOW' ] ) ); } ); diff --git a/js/common/model/MassesAndSpringsModel.js b/js/common/model/MassesAndSpringsModel.js index 08992d0e..e7389e95 100644 --- a/js/common/model/MassesAndSpringsModel.js +++ b/js/common/model/MassesAndSpringsModel.js @@ -46,9 +46,11 @@ define( function( require ) { basicsVersion: false }, options ); + // REVIEW: Don't store the options object, but just whether it is a basicsVersion or not. (this.basicsVersion = ...) this.options = options; if ( options.basicsVersion ) { + // REVIEW: Shouldn't have a console.log for this. console.log( 'basics version' ); } diff --git a/js/common/model/Spring.js b/js/common/model/Spring.js index 31a805a1..ae9e4b28 100644 --- a/js/common/model/Spring.js +++ b/js/common/model/Spring.js @@ -19,6 +19,7 @@ define( function( require ) { var inherit = require( 'PHET_CORE/inherit' ); var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' ); var MassesAndSpringsConstants = require( 'MASSES_AND_SPRINGS/common/MassesAndSpringsConstants' ); + // REVIEW: Can we remove commented-out lines? // var Mass = require( 'MASSES_AND_SPRINGS/common/model/Mass' ); // var MassIO = require( 'MASSES_AND_SPRINGS/common/model/MassIO' ); // var NullableIO = require( 'TANDEM/types/NullableIO' ); @@ -218,9 +219,11 @@ define( function( require ) { this.naturalRestingLengthProperty ], function( springConstant, gravity, mass, naturalRestingLength ) { + // REVIEW: Is it possible to combine this with the multilink that sets massEquilibriumYPositionProperty? + // REVIEW: The springExtensionValue is computed in both. if ( mass ) { var springExtensionValue = - ( mass.massProperty.value * self.gravityProperty.value) / self.springConstantProperty.value; + ( mass.massProperty.value * self.gravityProperty.value ) / self.springConstantProperty.value; self.equilibriumYPositionProperty.set( self.positionProperty.get().y - naturalRestingLength - springExtensionValue ); } @@ -262,7 +265,7 @@ define( function( require ) { // springExtension = mg/k var springExtensionValue = - ( mass.massProperty.value * self.gravityProperty.value) / self.springConstantProperty.value; + ( mass.massProperty.value * self.gravityProperty.value ) / self.springConstantProperty.value; self.massEquilibriumYPositionProperty.set( self.positionProperty.get().y - naturalRestingLength - springExtensionValue - mass.heightProperty.value / 2 ); @@ -340,7 +343,7 @@ define( function( require ) { * @param {Object} springState - Sets the springs's properties with previously stored properties. See getSpringState * * @public - * @returns {Object} + * @returns {Object} REVIEW: I don't see a return value here */ setSpringState: function( springState ) { this.displacementProperty.set( springState.displacement ); @@ -401,7 +404,8 @@ define( function( require ) { this.buttonEnabledProperty.set( false ); }, - /** Updates the displacement Property of the spring. + /** + * Updates the displacement Property of the spring. * @param {number} yPosition * @param {boolean} factorNaturalLength * diff --git a/js/common/view/DraggableRulerNode.js b/js/common/view/DraggableRulerNode.js index 7b389cc5..aff96377 100644 --- a/js/common/view/DraggableRulerNode.js +++ b/js/common/view/DraggableRulerNode.js @@ -67,6 +67,7 @@ define( function( require ) { }, { tandem: tandem.createTandem( 'ruler' ) } ); // @private {Vector2} (read-only) position of ruler node in screen coordinates + // REVIEW: JSDoc looks incorrect this.positionProperty = new Property( initialPosition, { tandem: tandem.createTandem( 'positionProperty' ), phetioType: PropertyIO( Vector2IO ) diff --git a/js/common/view/DraggableTimerNode.js b/js/common/view/DraggableTimerNode.js index 7055fa33..e4ce6d38 100644 --- a/js/common/view/DraggableTimerNode.js +++ b/js/common/view/DraggableTimerNode.js @@ -47,11 +47,13 @@ define( function( require ) { // @private {Vector2} (read-only) position of ruler node in screen coordinates + // REVIEW: Seems like incorrect type JSDoc this.positionProperty = new Property( initialPosition, { tandem: tandem.createTandem( 'positionProperty' ), phetioType: PropertyIO( Vector2IO ) } ); this.positionProperty.linkAttribute( this, 'translation' ); + // REVIEW: Also seems like a lot of code duplicated with DraggableRulerNode. Is there anything that can be deduplicated? // @private {MovableDragHandler} (read-only) handles timer node drag events this.timerNodeMovableDragHandler = new MovableDragHandler( this.positionProperty, { diff --git a/js/common/view/EnergyGraphNode.js b/js/common/view/EnergyGraphNode.js index e5834f1f..938557e2 100644 --- a/js/common/view/EnergyGraphNode.js +++ b/js/common/view/EnergyGraphNode.js @@ -58,7 +58,6 @@ define( function( require ) { var totalEnergyString = require( 'string!MASSES_AND_SPRINGS/totalEnergy' ); /** - * * @param {MassesAndSpringsModel} model * @param {Tandem} tandem * @constructor @@ -92,7 +91,7 @@ define( function( require ) { self.zoomLevelProperty.value += 1; } ); - // {DerivedProperty.} Responsible for adjusting the scaling of the barNode heights. + // {Property.} Responsible for adjusting the scaling of the barNode heights. var scaleFactorProperty = new DerivedProperty( [ this.zoomLevelProperty ], function( zoomLevel ) { return Math.pow( 2, zoomLevel ) * 20; } ); @@ -294,6 +293,7 @@ define( function( require ) { reset: function() { this.zoomLevelProperty.reset(); }, + /** * @public */ diff --git a/js/common/view/ForceVectorArrow.js b/js/common/view/ForceVectorArrow.js index eb62d991..b1e05476 100644 --- a/js/common/view/ForceVectorArrow.js +++ b/js/common/view/ForceVectorArrow.js @@ -29,5 +29,6 @@ define( function( require ) { } massesAndSprings.register( 'ForceVectorArrow', ForceVectorArrow ); + return inherit( ArrowNode, ForceVectorArrow ); } ); diff --git a/js/common/view/MassNode.js b/js/common/view/MassNode.js index 522675e2..bbcd6732 100644 --- a/js/common/view/MassNode.js +++ b/js/common/view/MassNode.js @@ -63,6 +63,7 @@ define( function( require ) { hookHeight = modelViewTransform2.modelToViewDeltaY( -MassesAndSpringsConstants.HOOK_HEIGHT * 0.34 ); } + // REVIEW: JSDoc? this.rect = new Rectangle( { stroke: 'black', boundsMethod: 'unstroked', @@ -102,6 +103,7 @@ define( function( require ) { } } ); + // REVIEW: Why JSDoc here? // @public Sets the gradient on the massNode. this.rect.fill = new LinearGradient( -this.rect.width / 2, 0, this.rect.width / 2, 0 ) .addColorStop( 0, Color.toColor( mass.color ).colorUtilsBrighter( 0.1 ) ) @@ -113,6 +115,7 @@ define( function( require ) { hookShape.arc( 0, 0, radius, Math.PI, ( 0.5 * Math.PI ) ); hookShape.lineTo( 0, hookHeight / 2 ); + // REVIEW: Type doc? // @public Used for hook on massNode. this.hookNode = new Path( hookShape, { stroke: 'black', @@ -255,7 +258,7 @@ define( function( require ) { * @param {Property.} arrowVisibilityProperty * @param {Node} arrowNode * - * @private + * @private REVIEW: No visibility for a local variable */ var updateForceVisibility = function( arrowVisibilityProperty, arrowNode ) { Property.multilink( [ mass.springProperty, arrowVisibilityProperty, model.forcesModeProperty ], @@ -330,7 +333,7 @@ define( function( require ) { function( spring, gravity ) { var gravitationalAcceleration = mass.mass * gravity; position = mass.positionProperty.get(); - xOffset = (forcesOrientation) * 45; + xOffset = forcesOrientation * 45; y2 = ARROW_SIZE_DEFAULT * gravitationalAcceleration; updateArrow( gravityForceArrow, position, xOffset, y2 ); } ); @@ -339,7 +342,7 @@ define( function( require ) { Property.multilink( [ mass.springForceProperty ], function( springForce ) { position = mass.positionProperty.get(); - xOffset = (forcesOrientation) * 45; + xOffset = forcesOrientation * 45; y2 = -ARROW_SIZE_DEFAULT * springForce; updateArrow( springForceArrow, position, xOffset, y2 ); } ); @@ -354,7 +357,7 @@ define( function( require ) { ], function( netForce, accelerationVisible, netAcceleration, velocityVisible ) { position = mass.positionProperty.get(); if ( Math.abs( netForce ) > 1E-6 ) { - xOffset = (forcesOrientation) * 45; + xOffset = forcesOrientation * 45; y2 = -ARROW_SIZE_DEFAULT * netForce; updateArrow( netForceArrow, position, xOffset, y2 ); } @@ -374,12 +377,13 @@ define( function( require ) { // When the mass's position changes update the forces baseline marker mass.positionProperty.link( function( position ) { forceNullLine.setLine( - self.rect.centerX + (forcesOrientation) * 40, position.y + self.rect.centerY, - self.rect.centerX + (forcesOrientation) * 50, position.y + self.rect.centerY + self.rect.centerX + forcesOrientation * 40, position.y + self.rect.centerY, + self.rect.centerX + forcesOrientation * 50, position.y + self.rect.centerY ); } ); } massesAndSprings.register( 'MassNode', MassNode ); + return inherit( Node, MassNode ); } ); diff --git a/js/common/view/MassValueControlPanel.js b/js/common/view/MassValueControlPanel.js index f625cecd..0aca301c 100644 --- a/js/common/view/MassValueControlPanel.js +++ b/js/common/view/MassValueControlPanel.js @@ -48,7 +48,7 @@ define( function( require ) { stroke: 'gray', yMargin: 6, xMargin: 6, - basicsVersion: false, + basicsVersion: false, // REVIEW: Where is this used? tandem: tandem }, options ); @@ -65,7 +65,7 @@ define( function( require ) { } } ); - // @public {Property.} + // @public {Property.} REVIEW: No JSDoc on local variable? var trackSizeProperty = new Property( options.basics ? new Dimension2( 132, 0.1 ) : new Dimension2( 125, 0.1 ) ); var numberControl = new MutableOptionsNode( NumberControl, [ massString, massInGramsProperty, range ], { @@ -116,5 +116,6 @@ define( function( require ) { } massesAndSprings.register( 'MassValueControlPanel', MassValueControlPanel ); + return inherit( Panel, MassValueControlPanel ); } ); diff --git a/js/common/view/MovableLineNode.js b/js/common/view/MovableLineNode.js index 4f443a38..5654d9d2 100644 --- a/js/common/view/MovableLineNode.js +++ b/js/common/view/MovableLineNode.js @@ -85,6 +85,7 @@ define( function( require ) { initialPosition.setX( dragBounds.minX ); // @private {Vector2} (read-write) position of line in screen coordinates + // REVIEW: Incorrect JSDoc this.positionProperty = new Property( initialPosition, { tandem: tandem.createTandem( 'positionProperty' ), phetioType: PropertyIO( Vector2IO ) @@ -113,5 +114,4 @@ define( function( require ) { this.positionProperty.reset(); } } ); - } ); \ No newline at end of file diff --git a/js/common/view/OneSpringScreenView.js b/js/common/view/OneSpringScreenView.js index fe6ae5b0..900ca2d5 100644 --- a/js/common/view/OneSpringScreenView.js +++ b/js/common/view/OneSpringScreenView.js @@ -46,7 +46,7 @@ define( function( require ) { SpringScreenView.call( this, model, tandem, options ); var self = this; - // @public {Number} centerX of the spring in view coordinates + // @public {number} centerX of the spring in view coordinates this.springCenter = self.modelViewTransform.modelToViewX( model.firstSpring.positionProperty.value.x ); // Spring Constant Control Panel @@ -55,7 +55,7 @@ define( function( require ) { new Text( largeString, { font: MassesAndSpringsConstants.LABEL_FONT, maxWidth: 60 } ) ]; - // @public {DerivedProperty.} Equilibrium of mass is dependent on the mass being attached and the visibility of the equilibrium line. + // @public {Property.} Equilibrium of mass is dependent on the mass being attached and the visibility of the equilibrium line. this.equilibriumVisibilityProperty = new DerivedProperty( [ model.equilibriumPositionVisibleProperty, model.firstSpring.massAttachedProperty ], function( equilibriumPositionVisible, massAttached ) { @@ -121,6 +121,7 @@ define( function( require ) { self.springStopperButtonNode.enabled = buttonEnabled; } ); + // REVIEW: Turn this into model.basicsVersion, and properly document it. if ( !model.options.basicsVersion ) { // @public {EnergyGraphNode} energy graph that displays energy values for the spring system. this.energyGraphNode = new EnergyGraphNode( model, tandem ); @@ -130,7 +131,7 @@ define( function( require ) { // Property that determines the zero height in the view. var zeroHeightProperty = new Property( this.modelViewTransform.modelToViewY( MassesAndSpringsConstants.FLOOR_Y ) ); - // @public {MovableLineNode} Initializes movable line + // @public {MovableLineNode} Initializes movable line REVIEW: No JSDoc on local variable var xBoundsLimit = this.springCenter + this.spacing * 1.1; this.movableLineNode = new MovableLineNode( this.springHangerNode.center.plus( new Vector2( 45, 200 ) ), @@ -176,6 +177,8 @@ define( function( require ) { this.resetAllButton.addListener( function() { self.model.reset(); self.movableLineNode.reset(); + // REVIEW: Generally prefer `self.energyGraphNode && self.energyGraphNode.reset` if it is optional, instead of + // REVIEW: repeating the same constraints. if ( !model.options.basicsVersion ) { self.energyGraphNode.reset(); } @@ -269,6 +272,7 @@ define( function( require ) { MassesAndSpringsModel.prototype.reset.call( this ); this.energyGraphNode.reset(); }, + /** * Responsible for updating the energy bar graph * diff --git a/js/common/view/OscillatingSpringNode.js b/js/common/view/OscillatingSpringNode.js index f54be357..1ef05ff3 100644 --- a/js/common/view/OscillatingSpringNode.js +++ b/js/common/view/OscillatingSpringNode.js @@ -53,6 +53,7 @@ define( function( require ) { // @public {Spring} (read-only) this.spring = spring; + this.translation = modelViewTransform2.modelToViewPosition( new Vector2( spring.positionProperty.get().x, spring.positionProperty.get().y - length ) ); diff --git a/js/common/view/ReferenceLineNode.js b/js/common/view/ReferenceLineNode.js index c693cdba..c84ae551 100644 --- a/js/common/view/ReferenceLineNode.js +++ b/js/common/view/ReferenceLineNode.js @@ -58,6 +58,7 @@ define( function( require ) { var yPos = modelViewTransform2.modelToViewY( lengthFunction( spring.naturalRestingLengthProperty.value ) ); // @private {Vector2} (read-write) position of line in screen coordinates. + // REVIEW: Incorrect JSDoc this.positionProperty = new Property( new Vector2( xPos, yPos ), { phetioType: PropertyIO( Vector2IO ) } ); @@ -94,7 +95,7 @@ define( function( require ) { // Adjust the position of the label if ( options.label ) { options.label.centerY = 0; - options.label.x = (LINE_LENGTH + 10); + options.label.x = LINE_LENGTH + 10; } } ); diff --git a/js/common/view/Shelf.js b/js/common/view/Shelf.js index 9004ab35..26447dba 100644 --- a/js/common/view/Shelf.js +++ b/js/common/view/Shelf.js @@ -17,6 +17,7 @@ define( function( require ) { * @param {Tandem} tandem * @param {Object} [options] * @constructor + * REVIEW: ShelfNode might be a better name? `Shelf` seems like the model name */ function Shelf( tandem, options ) { options = _.extend( { diff --git a/js/common/view/SpringControlPanel.js b/js/common/view/SpringControlPanel.js index e8d23b04..e68f6c62 100644 --- a/js/common/view/SpringControlPanel.js +++ b/js/common/view/SpringControlPanel.js @@ -34,8 +34,8 @@ define( function( require ) { titleFont: MassesAndSpringsConstants.TITLE_FONT, xMargin: 5, yMargin: 5, - spacing:3, - minWidth:165, + spacing: 3, + minWidth: 165, align: 'center', centerTick: false, cornerRadius: MassesAndSpringsConstants.PANEL_CORNER_RADIUS, @@ -44,6 +44,7 @@ define( function( require ) { minorTickMarksVisible: true, sliderTrackSize: new Dimension2( 120, 0.1 ), tickLabelSpacing: 6, + // REVIEW: Just pass `constrainValue: Util.roundSymmetric`? constrainValue: function( value ) { return Util.roundSymmetric( value ); } @@ -94,12 +95,9 @@ define( function( require ) { hSlider ] } ), options ); - } massesAndSprings.register( 'SpringControlPanel', SpringControlPanel ); return inherit( Panel, SpringControlPanel ); - -} ) -; +} ); diff --git a/js/common/view/SpringHangerNode.js b/js/common/view/SpringHangerNode.js index 0c111471..716eb65a 100644 --- a/js/common/view/SpringHangerNode.js +++ b/js/common/view/SpringHangerNode.js @@ -24,7 +24,7 @@ define( function( require ) { /** * @param {Array.} springs - * @param {ModelViewTransform2} modelViewTransform2 + * @param {ModelViewTransform2} modelViewTransform2 REVIEW: Usually just name modelViewTransform or mvt * @param {Tandem} tandem * @param {Object} options * @constructor @@ -41,8 +41,8 @@ define( function( require ) { var springHangerNodeWidth = springsSeparation * 1.4; // X coordinate of middle of springs - var middleOfSprings = modelViewTransform2.modelToViewX( (springs[ 0 ].positionProperty.get().x + - springs[ 1 ].positionProperty.get().x) / 2 ); + var middleOfSprings = modelViewTransform2.modelToViewX( ( springs[ 0 ].positionProperty.get().x + + springs[ 1 ].positionProperty.get().x ) / 2 ); // derived from x positions of springs. Rectangle.call( this, 0, 0, springHangerNodeWidth, 20, 8, 8, { diff --git a/js/common/view/SpringScreenView.js b/js/common/view/SpringScreenView.js index 5c37d58d..f4bba3dd 100644 --- a/js/common/view/SpringScreenView.js +++ b/js/common/view/SpringScreenView.js @@ -64,6 +64,7 @@ define( function( require ) { var self = this; + // REVIEW: JSDoc type // @public Support for expanding touchAreas near massNodes. this.backgroundDragPlane = new Plane(); var closestDragListener = new ClosestDragListener( 30, 0 ); @@ -74,7 +75,7 @@ define( function( require ) { // @public {MassesAndSpringsModel} this.model = model; - var viewOrigin = new Vector2( 0, this.visibleBoundsProperty.get().height * (1 - MassesAndSpringsConstants.SHELF_HEIGHT) ); + var viewOrigin = new Vector2( 0, this.visibleBoundsProperty.get().height * ( 1 - MassesAndSpringsConstants.SHELF_HEIGHT ) ); // @public {ModelViewTransform2} this.modelViewTransform = ModelViewTransform2.createSinglePointScaleInvertedYMapping( Vector2.ZERO, viewOrigin, 397 ); @@ -86,6 +87,7 @@ define( function( require ) { // @private {Array.} Used to reference the created springs in the view. this.springNodes = []; + // REVIEW: Don't use `self`, and can we just combine this into the "declaration" above? self.springNodes = model.springs.map( function( spring ) { var springNode = new MutableOptionsNode( OscillatingSpringNode, [ spring, self.modelViewTransform, tandem.createTandem( 'oscillatingSpringNode' ) ], @@ -242,7 +244,7 @@ define( function( require ) { tandem: tandem.createTandem( 'timeControlNode' ) } ); - // @public + // @public {AlignGroup} this.rightPanelAlignGroup = new AlignGroup( { matchVertical: false } ); // @public {ToolboxPanel} Toolbox Panel @@ -258,6 +260,7 @@ define( function( require ) { } ); + // REVIEW: JSDoc // Buttons controlling the speed of the sim, play/pause button, and the reset button this.simControlHBox = new HBox( { spacing: this.spacing * 6, @@ -336,6 +339,7 @@ define( function( require ) { sliderTrackSize: options.sliderTrackSize, tickLabelSpacing: options.tickLabelSpacing, constrainValue: function( value ) { + // REVIEW: Why using Number? To cast to a number, just do a unary plus `+` at the start return Number( Util.toFixed( value, 0 ) ); } } diff --git a/js/common/view/ToolboxPanel.js b/js/common/view/ToolboxPanel.js index a67540b1..788a3715 100644 --- a/js/common/view/ToolboxPanel.js +++ b/js/common/view/ToolboxPanel.js @@ -159,5 +159,4 @@ define( function( require ) { massesAndSprings.register( 'ToolboxPanel', ToolboxPanel ); return inherit( Panel, ToolboxPanel ); - } ); diff --git a/js/common/view/VectorArrow.js b/js/common/view/VectorArrow.js index aceba4ba..9fad8d91 100644 --- a/js/common/view/VectorArrow.js +++ b/js/common/view/VectorArrow.js @@ -29,5 +29,6 @@ define( function( require ) { } massesAndSprings.register( 'VectorArrow', VectorArrow ); + return inherit( ArrowNode, VectorArrow ); } ); diff --git a/js/energy/model/EnergyModel.js b/js/energy/model/EnergyModel.js index aca3ccc4..f488f85d 100644 --- a/js/energy/model/EnergyModel.js +++ b/js/energy/model/EnergyModel.js @@ -25,6 +25,7 @@ define( function( require ) { MassesAndSpringsModel.call( this, tandem ); // Energy screen should have spring damping + // REVIEW: This looks like we should pass in the initial value of damping, instead of overwriting the Property. this.dampingProperty = new NumberProperty( 0.0575 ); // Creation of masses and springs specific for this screen diff --git a/js/intro/enum/ConstantModeChoice.js b/js/intro/enum/ConstantModeChoice.js index 2c0b11c2..6e444e1a 100644 --- a/js/intro/enum/ConstantModeChoice.js +++ b/js/intro/enum/ConstantModeChoice.js @@ -12,9 +12,10 @@ define( function( require ) { var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' ); var Enumeration = require( 'PHET_CORE/Enumeration' ); - + // REVIEW: The recent decision was to move enumerations out of enum/ folders. Can you put it in model/? var ConstantModeChoice = new Enumeration( [ 'SPRING_CONSTANT', 'SPRING_THICKNESS' ] ); + // REVIEW: Other enums return the register, and inline the variable. Maybe that would work better? massesAndSprings.register( 'ConstantModeChoice', ConstantModeChoice ); return ConstantModeChoice; diff --git a/js/intro/enum/SceneModeChoice.js b/js/intro/enum/SceneModeChoice.js index bb17f411..5cabf822 100644 --- a/js/intro/enum/SceneModeChoice.js +++ b/js/intro/enum/SceneModeChoice.js @@ -12,5 +12,6 @@ define( function( require ) { var massesAndSprings = require( 'MASSES_AND_SPRINGS/massesAndSprings' ); var Enumeration = require( 'PHET_CORE/Enumeration' ); + // REVIEW: The recent decision was to move enumerations out of enum/ folders. Can you put it in model/? return massesAndSprings.register( 'SceneModeChoice', new Enumeration( [ 'SAME_LENGTH', 'ADJUSTABLE_LENGTH' ] ) ); } ); \ No newline at end of file diff --git a/js/intro/view/IntroScreenView.js b/js/intro/view/IntroScreenView.js index d1a5d26e..635f24a6 100644 --- a/js/intro/view/IntroScreenView.js +++ b/js/intro/view/IntroScreenView.js @@ -76,6 +76,7 @@ define( function( require ) { visible: false, centerTick: true, constrainValue: function( value ) { + // REVIEW: Could actually use Util.roundToInterval now? value = Util.roundSymmetric( value * 100 / 5 ) * 5; return value / 100; } @@ -89,6 +90,7 @@ define( function( require ) { tandem.createTandem( 'constantsControlPanel' ), { maxWidth: 160, + // REVIEW: Am I missing usage of this value? alignGroup: optionsContentAlignGroup, stroke: null } diff --git a/js/lab/model/LabModel.js b/js/lab/model/LabModel.js index ae2d84b8..4514b846 100644 --- a/js/lab/model/LabModel.js +++ b/js/lab/model/LabModel.js @@ -40,7 +40,7 @@ define( function( require ) { this.firstSpring = this.springs[ 0 ]; // @private {boolean} Flag used to determine if this is the basics version. - this.basicsVersion = this.options.basicsVersion; + this.basicsVersion = this.options.basicsVersion; // REVIEW: Don't have `this.options` in general var massXPosition = this.basicsVersion ? 0.13 : 0.625; var massValue; var color; @@ -62,6 +62,7 @@ define( function( require ) { } // Initialize masses for non-basics version + // REVIEW: This code seems to be running for both versions. Is that correct? massValue = this.basicsVersion ? 0.12 : 0.23; color = this.basicsVersion ? new Color( 'rgb( 9, 19, 174 )' ) : new Color( 'rgb( 0, 222, 224 )' ); this.createMass( massValue, massXPosition + MASS_OFFSET * 1.5, new Property( color ), null, tandem.createTandem( 'mediumMysteryMass' ), { @@ -70,6 +71,7 @@ define( function( require ) { } ); massValue = this.basicsVersion ? 0.060 : 0.37; + // REVIEW: `new Color( 'rgb(a,b,c)' )` is equivalent to `new Color( a, b, c )` if that's more helpful. color = this.basicsVersion ? new Color( 'rgb( 10, 198, 157 )' ) : new Color( 'rgb( 246, 164, 255 )' ); this.createMass( massValue, massXPosition + MASS_OFFSET, new Property( color ), null, tandem.createTandem( 'smallMysteryMass' ), { density: this.basicsVersion ? 80 : 220, @@ -84,11 +86,12 @@ define( function( require ) { return inherit( MassesAndSpringsModel, LabModel, { /** - * * @public */ reset: function() { MassesAndSpringsModel.prototype.reset.call( this ); + + // REVIEW: Usually just check `this.someProperty && this.someProperty.reset();` if ( this.basicsVersion ) { this.gravityAccordionBoxExpandedProperty.reset(); } diff --git a/js/lab/view/LabScreenView.js b/js/lab/view/LabScreenView.js index 01cfb3c6..48ca9005 100644 --- a/js/lab/view/LabScreenView.js +++ b/js/lab/view/LabScreenView.js @@ -88,6 +88,7 @@ define( function( require ) { massesAndSprings.register( 'LabScreenView', LabScreenView ); return inherit( OneSpringScreenView, LabScreenView, { + // REVIEW: JSDoc step: function( dt ) { this.energyGraphNode.update(); this.periodTraceNode.step( dt, this.model.playingProperty ); diff --git a/js/lab/view/PeriodTraceNode.js b/js/lab/view/PeriodTraceNode.js index 820a1e01..4135d9b5 100644 --- a/js/lab/view/PeriodTraceNode.js +++ b/js/lab/view/PeriodTraceNode.js @@ -64,6 +64,7 @@ define( function( require ) { /** * @param {number} dt * @param {Property.} playingProperty: whether the sim is playing or not + * REVIEW: visibility (public?) */ step: function( dt, playingProperty ) { var spring = this.periodTrace.spring; @@ -72,11 +73,11 @@ define( function( require ) { // The period trace should only be drawn when a mass is oscillating on a spring and its checkbox is toggled on. if ( mass && !mass.userControlledProperty.value && spring.periodTraceVisibilityProperty.value && - !(mass.verticalVelocityProperty.value === 0) ) { + !( mass.verticalVelocityProperty.value === 0 ) ) { // Responsible for fading the period trace.We want to fade only when the sim is playing and // the state is either 4 or when the trace has begun fading already. - if ( (this.periodTrace.stateProperty.value === 4 || this.colorAlpha !== 1) && playingProperty.value ) { + if ( ( this.periodTrace.stateProperty.value === 4 || this.colorAlpha !== 1 ) && playingProperty.value ) { this.fade( dt ); } @@ -91,7 +92,7 @@ define( function( require ) { var secondPeakYPosition = modelViewTransform.modelToViewY( massEquilibrium + this.periodTrace.secondPeakY ); var currentYPosition = modelViewTransform.modelToViewY( massEquilibrium + spring.massEquilibriumDisplacementProperty.value ); - + // REVIEW: Why setting this here? JSDoc it also, and maybe declare in constructor IF NEEDED. this.shape = new Shape(); var state = this.periodTrace.stateProperty.value; // 0 to 4 @@ -144,9 +145,10 @@ define( function( require ) { this.periodTrace.onFaded(); } }, + /** * Fades the period trace. - * @param dt + * @param {number} dt * * @private */ diff --git a/js/vectors/view/IndicatorVisibilityControlNode.js b/js/vectors/view/IndicatorVisibilityControlNode.js index 5edee546..f05c1808 100644 --- a/js/vectors/view/IndicatorVisibilityControlNode.js +++ b/js/vectors/view/IndicatorVisibilityControlNode.js @@ -111,6 +111,7 @@ define( function( require ) { maxWidth: CONTENT_MAX_WIDTH, tandem: tandem.createTandem( 'massEquilibriumString' ) } ), { xAlign: 'left', group: alignGroup } ); + // REVIEW: typo, should be "movable"? var movalbeLineAlignBox = new AlignBox( new Text( movableLineString, { font: MassesAndSpringsConstants.TITLE_FONT, maxWidth: CONTENT_MAX_WIDTH, @@ -141,6 +142,8 @@ define( function( require ) { } ); if ( options.periodTraceOption ) { + // REVIEW: Seems like some code duplication here. Can we just adjust the array beforehand (by inserting elements), + // REVIEW: and create the VerticalCheckboxGroup once? indicatorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ { node: new HBox( { children: [ componentDisplacement, new VBox( { children: [ displacementSymbol, blueLine ] } ) ], diff --git a/js/vectors/view/VectorVisibilityControlNode.js b/js/vectors/view/VectorVisibilityControlNode.js index eb219820..f8a399d7 100644 --- a/js/vectors/view/VectorVisibilityControlNode.js +++ b/js/vectors/view/VectorVisibilityControlNode.js @@ -94,6 +94,8 @@ define( function( require ) { } // Responsible for velocity and acceleration vectors checkboxes and period trace in basics version else { + // REVIEW: Looks like some duplication here, where there is just an added title. Can we reduce that by + // REVIEW: conditionally adding the node? vectorVisibilityCheckboxGroup = new VerticalCheckboxGroup( [ { node: new Text( periodTraceString, { font: MassesAndSpringsConstants.TITLE_FONT,