From 4afbd4522df01628b07ef0c2c03a52a5bd2dfe92 Mon Sep 17 00:00:00 2001 From: Jonathan Olson Date: Fri, 4 Jan 2019 12:52:13 -0700 Subject: [PATCH] Some review changes, see https://github.com/phetsims/fractions-common/issues/29 --- js/building/model/Group.js | 2 ++ js/building/model/NumberGroup.js | 3 +-- js/building/model/NumberSpot.js | 7 +++---- js/building/model/NumberStack.js | 1 + js/building/model/ShapeContainer.js | 3 +-- js/building/model/ShapeGroup.js | 6 ++---- js/building/model/ShapePiece.js | 23 ++++++++++++----------- js/building/view/NumberGroupNode.js | 1 + js/building/view/NumberPieceNode.js | 1 + js/building/view/ShapePieceNode.js | 1 + js/common/FractionsCommonConstants.js | 13 ++++++------- 11 files changed, 31 insertions(+), 30 deletions(-) diff --git a/js/building/model/Group.js b/js/building/model/Group.js index a1d4dd7..b088a26 100644 --- a/js/building/model/Group.js +++ b/js/building/model/Group.js @@ -52,6 +52,8 @@ define( require => { // Keep our hover target up-to-date // REVIEW: Does this need an unlink? + // REVIEW*: I don't believe so. Since we "own" the Property, any reference to this is going to also reference the + // REVIEW*: Property (and vice versa). this.hoveringTargetProperty.lazyLink( ( newTarget, oldTarget ) => { oldTarget && oldTarget.hoveringGroups.remove( this ); newTarget && newTarget.hoveringGroups.push( this ); diff --git a/js/building/model/NumberGroup.js b/js/building/model/NumberGroup.js index db1b2af..1927163 100644 --- a/js/building/model/NumberGroup.js +++ b/js/building/model/NumberGroup.js @@ -154,8 +154,7 @@ define( require => { */ updateAllowedSpots() { if ( this.isMixedNumber ) { - // REVIEW: 'let' instead of 'var'? - var range = this.activeNumberRangeProperty.value; + const range = this.activeNumberRangeProperty.value; this.numeratorSpot.showNotAllowedProperty.value = range === null ? false : !this.canPlaceNumberInSpot( range.min, this.numeratorSpot ); this.denominatorSpot.showNotAllowedProperty.value = range === null ? false : !this.canPlaceNumberInSpot( range.max, this.denominatorSpot ); diff --git a/js/building/model/NumberSpot.js b/js/building/model/NumberSpot.js index 991aa86..af36085 100644 --- a/js/building/model/NumberSpot.js +++ b/js/building/model/NumberSpot.js @@ -8,11 +8,10 @@ define( require => { 'use strict'; - //REVIEW: `const` instead of `var`? // modules - var BooleanProperty = require( 'AXON/BooleanProperty' ); - var fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' ); - var Property = require( 'AXON/Property' ); + const BooleanProperty = require( 'AXON/BooleanProperty' ); + const fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' ); + const Property = require( 'AXON/Property' ); class NumberSpot { /** diff --git a/js/building/model/NumberStack.js b/js/building/model/NumberStack.js index 2bb889e..226aba7 100644 --- a/js/building/model/NumberStack.js +++ b/js/building/model/NumberStack.js @@ -22,6 +22,7 @@ define( require => { */ // REVIEW: Could you respond in #29 about this notation/syntax "isMutable = true" in the constructor arguements? // REVIEW: If 'isMutable' is always true why is it a parameter and not a const? + // REVIEW*: It's a default value for a parameter (that it gets if not provided). BuildingLabModel provides false. constructor( number, layoutQuantity, isMutable = true ) { super( BuildingType.NUMBER, layoutQuantity, isMutable ); diff --git a/js/building/model/ShapeContainer.js b/js/building/model/ShapeContainer.js index 39e222c..fc468e5 100644 --- a/js/building/model/ShapeContainer.js +++ b/js/building/model/ShapeContainer.js @@ -20,8 +20,7 @@ define( require => { const Util = require( 'DOT/Util' ); const Vector2 = require( 'DOT/Vector2' ); - // REVIEW: Please add doc about how this is used. - const scratchVector = new Vector2(); + const scratchVector = new Vector2(); // Used as a shared Vector2 so that we can avoid allocating vectors dynamically. class ShapeContainer { /** diff --git a/js/building/model/ShapeGroup.js b/js/building/model/ShapeGroup.js index c593e67..1662308 100644 --- a/js/building/model/ShapeGroup.js +++ b/js/building/model/ShapeGroup.js @@ -107,8 +107,7 @@ define( require => { * @returns {boolean} */ hasAnyPieces() { - // REVIEW: Are we still using 'var' in for loops? - for ( var i = 0; i < this.shapeContainers.length; i++ ) { + for ( let i = 0; i < this.shapeContainers.length; i++ ) { if ( this.shapeContainers.get( i ).shapePieces.length ) { return true; } @@ -121,8 +120,7 @@ define( require => { * @public */ increaseContainerCount() { - // REVIEW: 'let' instead of 'var'? - var offset = new Vector2( this.shapeContainers.length * ( FractionsCommonConstants.SHAPE_SIZE + FractionsCommonConstants.SHAPE_CONTAINER_PADDING ), 0 ); + const offset = new Vector2( this.shapeContainers.length * ( FractionsCommonConstants.SHAPE_SIZE + FractionsCommonConstants.SHAPE_CONTAINER_PADDING ), 0 ); this.shapeContainers.push( new ShapeContainer( this, this.partitionDenominatorProperty, this.representation, this.changedEmitter, offset ) ); } diff --git a/js/building/model/ShapePiece.js b/js/building/model/ShapePiece.js index c78e041..21d3f84 100644 --- a/js/building/model/ShapePiece.js +++ b/js/building/model/ShapePiece.js @@ -22,7 +22,9 @@ define( require => { const NumberProperty = require( 'AXON/NumberProperty' ); const Property = require( 'AXON/Property' ); const Vector2 = require( 'DOT/Vector2' ); -// REVIEW: Added doc about how this is used would be helpful + + // globals + // Used for unique identifiers for every ShapePiece (so we can efficiently store a map from piece ID to other objects. let globalID = 0; class ShapePiece { @@ -88,6 +90,7 @@ define( require => { this.trueTargetRotation = 0; // REVIEW: Does this property need an unlink? + // REVIEW*: Shouldn't need it, since they have the same lifetimes. this.isUserControlledProperty.link( isUserControlled => { if ( isUserControlled ) { this.shadowProperty.value = 1; @@ -96,16 +99,15 @@ define( require => { // Handle rotational animation towards a target (if any) // REVIEW: Does this need a dispose? + // REVIEW*: Shouldn't need it, since they have the same lifetimes. Property.multilink( [ this.isUserControlledProperty, this.targetRotationProperty ], ( isUserControlled, targetRotation ) => { if ( isUserControlled ) { - // REVIEW: 'let' instead of 'var' - var currentRotation = this.rotationProperty.value; + const currentRotation = this.rotationProperty.value; this.trueTargetRotation = Animator.modifiedEndAngle( currentRotation, this.targetRotationProperty.value ); - // REVIEW: 'let' instead of 'var' - // REVIEW: Documentation for these variables would be helpful, being they are only used below. - var damping = 1; - var force = 50; + // Constants tweaked to give the damped harmonic a pleasing behavior. + const damping = 1; + const force = 50; this.dampedHarmonicTimeElapsed = 0; this.dampedHarmonic = new DampedHarmonic( 1, Math.sqrt( 4 * force ) * damping, force, currentRotation - this.trueTargetRotation, this.angularVelocityProperty.value ); } @@ -162,12 +164,11 @@ define( require => { return Vector2.ZERO; } else { - // REVIEW: 'let' instead of 'var' - var positiveAngle = fraction.value * 2 * Math.PI; + const positiveAngle = fraction.value * 2 * Math.PI; // Compute the centroid for a circular sector - var radius = FractionsCommonConstants.SHAPE_SIZE / 2; - var distanceFromCenter = 4 / 3 * radius * Math.sin( positiveAngle / 2 ) / positiveAngle; + const radius = FractionsCommonConstants.SHAPE_SIZE / 2; + const distanceFromCenter = 4 / 3 * radius * Math.sin( positiveAngle / 2 ) / positiveAngle; return Vector2.createPolar( distanceFromCenter, -positiveAngle / 2 ); } } diff --git a/js/building/view/NumberGroupNode.js b/js/building/view/NumberGroupNode.js index 475f11c..e036038 100644 --- a/js/building/view/NumberGroupNode.js +++ b/js/building/view/NumberGroupNode.js @@ -153,6 +153,7 @@ define( require => { // Keep the group in the drag bounds (when they change) // REVIEW: Does this need an unlink? + // REVIEW*: Shouldn't need it, these have the same lifetime. this.dragBoundsProperty.lazyLink( dragBounds => { numberGroup.positionProperty.value = dragBounds.closestPointTo( numberGroup.positionProperty.value ); } ); diff --git a/js/building/view/NumberPieceNode.js b/js/building/view/NumberPieceNode.js index 9dd0b30..195101c 100644 --- a/js/building/view/NumberPieceNode.js +++ b/js/building/view/NumberPieceNode.js @@ -102,6 +102,7 @@ define( require => { } ); // REVIEW: Does this need an unlink? + // REVIEW*: Shouldn't need it, the two objects have the same lifetime. this.dragListener.isUserControlledProperty.link( controlled => { numberPiece.isUserControlledProperty.value = controlled; } ); diff --git a/js/building/view/ShapePieceNode.js b/js/building/view/ShapePieceNode.js index 3ed9028..b8a60df 100644 --- a/js/building/view/ShapePieceNode.js +++ b/js/building/view/ShapePieceNode.js @@ -147,6 +147,7 @@ define( require => { } } ); // REVIEW: Does this need an unlink? + // REVIEW*: Shouldn't need it, the two objects have the same lifetime. this.dragListener.isUserControlledProperty.link( controlled => { shapePiece.isUserControlledProperty.value = controlled; } ); diff --git a/js/common/FractionsCommonConstants.js b/js/common/FractionsCommonConstants.js index 89c5d51..d514888 100644 --- a/js/common/FractionsCommonConstants.js +++ b/js/common/FractionsCommonConstants.js @@ -8,16 +8,15 @@ define( require => { 'use strict'; - // REVEIW: 'let' instead of 'var' // modules - var fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' ); - var PhetFont = require( 'SCENERY_PHET/PhetFont' ); + const fractionsCommon = require( 'FRACTIONS_COMMON/fractionsCommon' ); + const PhetFont = require( 'SCENERY_PHET/PhetFont' ); - //REVIEW: Missing "// constants" notation - var WHOLE_FRACTIONAL_SIZE_RATIO = 2; - var SHAPE_RADIUS = 50; + // constants + const WHOLE_FRACTIONAL_SIZE_RATIO = 2; + const SHAPE_RADIUS = 50; - var FractionsCommonConstants = { + const FractionsCommonConstants = { // {number} PANEL_MARGIN: 10,