Add review comments and TODOs, minor adjustments, remove unused code,…

… see #123

js/buoyancy-basics/view/BuoyancyBasicsExploreScreenView.ts

@@ -159,6 +159,8 @@ export default class BuoyancyBasicsExploreScreenView extends BuoyancyScreenView<
 
     ManualConstraint.create( this, [ this.resetAllButton, blocksModeRadioButtonGroup ],
       ( resetAllButtonWrapper, blocksModeRadioButtonGroupWrapper ) => {
+
+      // TODO: What if the reset all button is not visible due to phet-io? See https://github.com/phetsims/density-buoyancy-common/issues/123
         blocksModeRadioButtonGroupWrapper.right = resetAllButtonWrapper.left - DensityBuoyancyCommonConstants.MARGIN;
         blocksModeRadioButtonGroupWrapper.bottom = resetAllButtonWrapper.bottom;
       } );
@@ -167,6 +169,7 @@ export default class BuoyancyBasicsExploreScreenView extends BuoyancyScreenView<
 
     // DerivedProperty doesn't need disposal, since everything here lives for the lifetime of the simulation
     this.rightBarrierViewPointPropertyProperty.value = new DerivedProperty( [ rightSideVBox.boundsProperty, this.visibleBoundsProperty ], ( boxBounds, visibleBounds ) => {
+
       // We might not have a box, see https://github.com/phetsims/density/issues/110
       return new Vector2( isFinite( boxBounds.left ) ? boxBounds.left : visibleBounds.right, visibleBounds.centerY );
     } );

js/buoyancy/view/DisplayProperties.ts

@@ -17,7 +17,7 @@ import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 
 const ZOOM_SCALES: number[] = [];
 
-// Populating zoom scales with powers of 2 from -9 to 0
+// Populating zoom scales with powers of 2
 for ( let i = 8; i > 0; i-- ) {
   ZOOM_SCALES.push( Math.pow( 0.5, i ) );
 }
@@ -97,6 +97,8 @@ export default class DisplayProperties {
     this.massValuesVisibleProperty.reset();
     this.forceValuesVisibleProperty.reset();
     this.depthLinesVisibleProperty.reset();
+
+    // TODO: Reset vectorZoomLevelProperty? See https://github.com/phetsims/density-buoyancy-common/issues/123
   }
 
 }

js/buoyancy/view/FluidDensityPanel.ts

@@ -21,9 +21,15 @@ export default class FluidDensityPanel extends Panel {
     const fluidDensityControlNode = new FluidDensityControlNode( fluidMaterialProperty, fluidMaterialProperty.availableValues,
       popupLayer, {
         invisibleMaterials: invisibleMaterials,
+
+        // TODO: Passing the same tandem 2 places seems suspicious. Check it and document if OK, see https://github.com/phetsims/density-buoyancy-common/issues/123
         tandem: tandem
       } );
-    super( fluidDensityControlNode, combineOptions<PanelOptions>( { tandem: tandem }, DensityBuoyancyCommonConstants.PANEL_OPTIONS ) );
+    super( fluidDensityControlNode, combineOptions<PanelOptions>( {
+
+      // TODO: See note above, see https://github.com/phetsims/density-buoyancy-common/issues/123
+      tandem: tandem
+    }, DensityBuoyancyCommonConstants.PANEL_OPTIONS ) );
   }
 }
 

js/buoyancy/view/FluidDisplacedAccordionBox.ts

@@ -140,6 +140,7 @@ export default class FluidDisplacedAccordionBox extends AccordionBox {
       forceReadout.centerX = beakerNode.centerX;
     } );
 
+    // TODO: Does any of this layout need to be dynamic? What if things disappear or reshape? See https://github.com/phetsims/density-buoyancy-common/issues/123
     numberDisplay.bottom = beakerNode.bottom - beakerNode.height * 0.05;
     numberDisplay.right = beakerNode.right;
     scaleIcon.top = beakerNode.bottom - 13;
@@ -161,6 +162,7 @@ export default class FluidDisplacedAccordionBox extends AccordionBox {
     );
     super( panel, options );
 
+    // TODO: Rename displayedDisplacedVolumeProperty to displacedVolumeProperty to match the tandem, see https://github.com/phetsims/density-buoyancy-common/issues/123
     this.addLinkedElement( displayedDisplacedVolumeProperty, {
       tandemName: 'displacedVolumeProperty'
     } );

js/buoyancy/view/ReadoutListAccordionBox.ts

@@ -56,7 +56,7 @@ export type ReadoutListAccordionBoxOptions<ReadoutType> = SelfOptions<ReadoutTyp
 
 export default abstract class ReadoutListAccordionBox<ReadoutType> extends AccordionBox {
 
-  protected cleanupEmitter = new TinyEmitter();
+  protected readonly cleanupEmitter = new TinyEmitter();
 
   private readonly readoutBox: VBox;
   private readonly contentWidthMaxProperty: TReadOnlyProperty<number>;
@@ -107,6 +107,8 @@ export default abstract class ReadoutListAccordionBox<ReadoutType> extends Accor
 
     this.readoutBox.children = readoutItems.map( readoutItem => {
 
+      // TODO: Create a new type and file for this data structure, see https://github.com/phetsims/density-buoyancy-common/issues/123
+      // TODO: So that the map will read readoutItems.map( item => new ReadoutItemNode(item)), and dispose has a method, see https://github.com/phetsims/density-buoyancy-common/issues/123
       const readoutData = this.generateReadoutData( readoutItem.readoutItem );
       const nameProperty = readoutItem.readoutNameProperty || readoutData.nameProperty;
       const nameColonProperty = new PatternStringProperty(

js/common/model/Basin.ts

@@ -18,6 +18,7 @@ import Mass from './Mass.js';
 import { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js';
 import DensityBuoyancyCommonConstants from '../DensityBuoyancyCommonConstants.js';
 
+// TODO: Add SelfOptions for optionize, see https://github.com/phetsims/density-buoyancy-common/issues/123
 export type BasinOptions = {
   initialVolume?: number;
   initialY?: number;
@@ -42,6 +43,8 @@ export default abstract class Basin {
   public childBasin: Basin | null;
 
   protected constructor( providedOptions?: BasinOptions ) {
+
+    // TODO: Use standard optionize type parameters, see https://github.com/phetsims/density-buoyancy-common/issues/123
     const options = optionize<BasinOptions>()( {
       initialVolume: 0,
       initialY: 0,
@@ -189,6 +192,7 @@ export default abstract class Basin {
 
   /**
    * Hybrid root-finding given our constraints (guaranteed interval, value/derivative). Combines Newton's and bisection.
+   * TODO: Move to a utility file or dot, see https://github.com/phetsims/density-buoyancy-common/issues/123
    */
   private static findRoot( minX: number, maxX: number, tolerance: number, valueFunction: ( n: number ) => number, derivativeFunction: ( n: number ) => number ): number {
     let x = ( minX + maxX ) / 2;

js/common/model/BlendedNumberProperty.ts

@@ -5,7 +5,7 @@
  *
  * Please see BlendedVector2Property to see the vector implementation of this concept.
  *
- * @author Sam Reid
+ * @author Sam Reid (PhET Interactive Simulations)
  */
 
 import Property from '../../../../axon/js/Property.js';

js/common/model/BlockSetModel.ts

@@ -33,7 +33,7 @@ export type BlockSetModelOptions<BlockSetValue extends EnumerationValue> = SelfO
 
 export default class BlockSetModel<BlockSetValue extends EnumerationValue> extends DensityBuoyancyModel {
 
-  // TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 Why is this capitalized?
+  // TODO: https://github.com/phetsims/density-buoyancy-common/issues/123 Convert to lowercase
   private readonly BlockSet: Enumeration<BlockSetValue>;
   public readonly blockSetProperty: Property<BlockSetValue>;
 
@@ -50,7 +50,6 @@ export default class BlockSetModel<BlockSetValue extends EnumerationValue> exten
 
     const tandem = options.tandem;
 
-    // {Property.<BlockSetValue>}
     this.blockSetProperty = new EnumerationProperty( options.initialMode, {
       tandem: tandem.createTandem( 'blockSets' ).createTandem( 'blockSetProperty' ),
       phetioDocumentation: 'Controls the set of blocks to be displayed',
@@ -76,6 +75,7 @@ export default class BlockSetModel<BlockSetValue extends EnumerationValue> exten
       this.positionMasses( blockSet );
     } );
 
+    // TODO: Add helpful documentation for what this link is for, see https://github.com/phetsims/density-buoyancy-common/issues/257
     // This instance lives for the lifetime of the simulation, so we don't need to remove this listener
     this.blockSetProperty.link( ( blockSet, oldBlockSet ) => {
       if ( oldBlockSet ) {

js/common/model/CompareBlockSetModel.ts

@@ -257,6 +257,7 @@ export default class CompareBlockSetModel extends BlockSetModel<BlockSet> {
                              colorDensityRange: Range,
                              providedOptions?: StrictCubeOptionsNoAvailableMaterials ): Cube {
 
+    // TODO: Document that this behaves like a DerivedProperty, see https://github.com/phetsims/density-buoyancy-common/issues/123
     const densityAdjustedColorProperty = new ColorProperty( baseColorProperty.value );
 
     const options = combineOptions<StrictCubeOptions>( {
@@ -291,6 +292,7 @@ export default class CompareBlockSetModel extends BlockSetModel<BlockSet> {
       densityAdjustedColorProperty.value = color.colorUtilsBrightness( factor * scale );
     } );
 
+    // TODO: Document the reason and effect of this multilink, see https://github.com/phetsims/density-buoyancy-common/issues/123
     Multilink.multilink( [ cube.materialProperty.densityProperty, blockSetValueChangedProperty ],
       ( density, blockSetValueChanged ) => {
 

js/common/model/Cone.ts

@@ -32,11 +32,9 @@ export default class Cone extends Mass {
   public readonly radiusProperty: Property<number>;
   public readonly heightProperty: Property<number>;
   public readonly isVertexUp: boolean;
-  private readonly vertexSign: number;
 
   // Step information
   private stepRadius: number;
-  private stepHeight: number;
   private stepArea: number;
   private stepMaximumVolume: number;
 
@@ -63,9 +61,7 @@ export default class Cone extends Mass {
     } );
 
     this.isVertexUp = isVertexUp;
-    this.vertexSign = isVertexUp ? 1 : -1;
     this.stepRadius = 0;
-    this.stepHeight = 0;
     this.stepArea = 0;
     this.stepMaximumVolume = 0;
 
@@ -139,7 +135,6 @@ export default class Cone extends Mass {
     this.stepTop = yOffset + this.heightProperty.value * ( this.isVertexUp ? TOP_FROM_CENTER_RATIO : BOTTOM_FROM_CENTER_RATIO );
 
     this.stepRadius = this.radiusProperty.value;
-    this.stepHeight = this.heightProperty.value;
     this.stepArea = Math.PI * this.stepRadius * this.stepRadius;
     this.stepMaximumVolume = this.stepArea * this.heightProperty.value / 3;
   }

js/common/model/DensityBuoyancyModel.ts

@@ -216,6 +216,8 @@ export default class DensityBuoyancyModel implements TModel {
     // once per "physics engine step", and so results in potentially up to "p2MaxSubSteps" calls per simulation frame
     // (30 as of writing). This instance lives for the lifetime of the simulation, so we don't need to remove this
     // listener.
+    // TODO: This is a very complex method, and it is in many ways the heart of the model. https://github.com/phetsims/density-buoyancy-common/issues/257
+    // TODO: It is recommended to provide more clarifying high-level documentation to show the main steps and their purpose https://github.com/phetsims/density-buoyancy-common/issues/257
     this.engine.addPostStepListener( dt => {
       this.updateFluid();
 

js/common/model/HorizontalCylinder.ts

@@ -30,8 +30,6 @@ export default class HorizontalCylinder extends Mass {
 
   // Step information
   private stepRadius: number;
-  private stepHeight: number;
-  private stepArea: number;
   private stepMaximumVolume: number;
   private stepMaximumArea: number;
 
@@ -45,7 +43,6 @@ export default class HorizontalCylinder extends Mass {
 
     super( engine, options as InstrumentedMassOptions );
 
-    // {Property.<number>}
     this.radiusProperty = new NumberProperty( radius, {
       tandem: options.tandem.createTandem( 'radiusProperty' ),
       range: new Range( 0, Number.POSITIVE_INFINITY )
@@ -56,8 +53,6 @@ export default class HorizontalCylinder extends Mass {
     } );
 
     this.stepRadius = 0;
-    this.stepHeight = 0;
-    this.stepArea = 0;
     this.stepMaximumVolume = 0;
     this.stepMaximumArea = 0;
 
@@ -127,7 +122,6 @@ export default class HorizontalCylinder extends Mass {
     this.stepTop = yOffset + this.radiusProperty.value;
 
     this.stepRadius = this.radiusProperty.value;
-    this.stepHeight = this.lengthProperty.value;
     this.stepMaximumArea = 2 * this.stepRadius * this.lengthProperty.value;
     this.stepMaximumVolume = Math.PI * this.stepRadius * this.stepRadius * this.lengthProperty.value;
   }

js/common/model/InterpolatedProperty.ts

@@ -100,13 +100,6 @@ export default class InterpolatedProperty<T> extends Property<T> {
     return a.blend( b, ratio );
   }
 
-  /**
-   * Interpolation for Vector3.
-   */
-  private static interpolateVector3( a: Vector3, b: Vector3, ratio: number ): Vector3 {
-    return a.blend( b, ratio );
-  }
-
   public static readonly InterpolatedPropertyIO = ( parameterType: IOType ): IOType => {
     assert && assert( parameterType, 'InterpolatedPropertyIO needs parameterType' );
 

js/common/model/Mass.ts

@@ -304,6 +304,7 @@ export default abstract class Mass extends PhetioObject {
       }
     }, options.massPropertyOptions ) );
 
+    // TODO: It looks like massProperty behaves like a DerivedProperty, can it be implemented like one? If not, please document, see https://github.com/phetsims/density-buoyancy-common/issues/123
     Multilink.multilink( [
       this.materialProperty.densityProperty,
       this.volumeProperty,
@@ -527,9 +528,6 @@ export default abstract class Mass extends PhetioObject {
 
   /**
    * Steps forward in time.
-   *
-   * @param dt - In seconds
-   * @param interpolationRatio
    */
   public step( dt: number, interpolationRatio: number ): void {
     this.readData();