From 1231d6794d67aa1b54aec67a6d89f93a62f93076 Mon Sep 17 00:00:00 2001
From: Sam Reid <reids@colorado.edu>
Date: Thu, 2 Mar 2023 16:04:26 -0700
Subject: [PATCH] Add review comments and minor updates, see
 https://github.com/phetsims/quadrilateral/issues/398

---
 js/QuadrilateralConstants.ts                      |  2 ++
 js/quadrilateral/QuadrilateralQueryParameters.ts  |  2 +-
 js/quadrilateral/model/QuadrilateralModel.ts      | 14 +++++++-------
 js/quadrilateral/model/QuadrilateralShapeModel.ts |  5 +++--
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/js/QuadrilateralConstants.ts b/js/QuadrilateralConstants.ts
index 24eb6731..e8a40b17 100644
--- a/js/QuadrilateralConstants.ts
+++ b/js/QuadrilateralConstants.ts
@@ -21,6 +21,8 @@ const QuadrilateralConstants = {
   GRID_SPACING: 0.25,
 
   // Dimensions of model bounds.
+  // REVIEW: Explain why it is 3.1.  What is the 0.1 overlap exactly?  Or recompute the dimensions based on
+  // total number of gridlines, etc.
   BOUNDS_WIDTH: 3.1,
   BOUNDS_HEIGHT: 2.1,
 
diff --git a/js/quadrilateral/QuadrilateralQueryParameters.ts b/js/quadrilateral/QuadrilateralQueryParameters.ts
index 3766bdf9..c7f92f66 100644
--- a/js/quadrilateral/QuadrilateralQueryParameters.ts
+++ b/js/quadrilateral/QuadrilateralQueryParameters.ts
@@ -99,7 +99,7 @@ const QuadrilateralQueryParameters = QueryStringMachine.getAll( {
   },
 
   // How many values to save when smoothing vertex positions when connected to a bluetooth device. Note that
-  // at this time this has no impact on the OpenCV prototype input. Only Bluetooth/Serial connections.
+  // this has no impact on the OpenCV prototype input. Only Bluetooth/Serial connections.
   smoothingLength: {
     type: 'number',
     defaultValue: 5,
diff --git a/js/quadrilateral/model/QuadrilateralModel.ts b/js/quadrilateral/model/QuadrilateralModel.ts
index 4f8454e2..cbe0f05a 100644
--- a/js/quadrilateral/model/QuadrilateralModel.ts
+++ b/js/quadrilateral/model/QuadrilateralModel.ts
@@ -2,8 +2,8 @@
 
 /**
  * The base model class for the sim. Assembles all model components and responsible for managing Properties
- * that indicate the state of the Quadrilateral shape. Will probably also manage Properties that manage
- * the state of the Sim (UI element visibility and so on).
+ * that indicate the state of the Quadrilateral shape. Also includes Properties that manage the state of the Sim (UI
+ * element visibility and so on).
  *
  * @author Jesse Greenberg (PhET Interactive Simulations)
  */
@@ -45,7 +45,7 @@ export default class QuadrilateralModel implements TModel {
 
   // The bounds of the simulation in model coordinates. Origin (0,0) is at the center. The shape and
   // vertices can be positioned within these bounds.
-  public modelBounds = new Bounds2(
+  public readonly modelBounds = new Bounds2(
     -QuadrilateralConstants.BOUNDS_WIDTH / 2,
     -QuadrilateralConstants.BOUNDS_HEIGHT / 2,
     QuadrilateralConstants.BOUNDS_WIDTH / 2,
@@ -81,11 +81,11 @@ export default class QuadrilateralModel implements TModel {
   public readonly shapeSoundEnabledProperty: BooleanProperty;
 
   // Model component for the quadrilateral shape.
-  public quadrilateralShapeModel: QuadrilateralShapeModel;
+  public readonly quadrilateralShapeModel: QuadrilateralShapeModel;
 
   // A reference to a "test" model for the simulation. Used to validate vertex positions before setting them for
   // the "real" quadrilateralShapeModel. See QuadrilateralShapeModel.isQuadrilateralShapeAllowed().
-  public quadrilateralTestShapeModel: QuadrilateralShapeModel;
+  public readonly quadrilateralTestShapeModel: QuadrilateralShapeModel;
 
   // Emits an event when a full model reset happens (but not when a shape reset happens)
   public readonly resetEmitter = new Emitter();
@@ -102,8 +102,7 @@ export default class QuadrilateralModel implements TModel {
       tandem: tandem.createTandem( 'quadrilateralShapeModel' )
     } );
     this.quadrilateralTestShapeModel = new QuadrilateralShapeModel( this.modelBounds, this.resetNotInProgressProperty, smoothingLengthProperty, {
-      validateShape: false,
-      tandem: tandem.createTandem( 'quadrilateralTestShapeModel' )
+      validateShape: false
     } );
 
     this.visibilityModel = new QuadrilateralVisibilityModel( tandem.createTandem( 'visibilityModel' ) );
@@ -167,6 +166,7 @@ export default class QuadrilateralModel implements TModel {
     // This will un-defer and call listeners for us
     this.quadrilateralTestShapeModel.setPropertiesDeferred( false );
 
+    // REVIEW: Make isQuadrilateralShapeAllowed static
     return this.quadrilateralTestShapeModel.isQuadrilateralShapeAllowed();
   }
 
diff --git a/js/quadrilateral/model/QuadrilateralShapeModel.ts b/js/quadrilateral/model/QuadrilateralShapeModel.ts
index 0cbcef04..8e73496c 100644
--- a/js/quadrilateral/model/QuadrilateralShapeModel.ts
+++ b/js/quadrilateral/model/QuadrilateralShapeModel.ts
@@ -48,7 +48,7 @@ type QuadrilateralShapeModelOptions = {
   // If true, the shape gets tested to make sure it is valid. This means no overlapping vertices and no crossed
   // sides.
   validateShape?: boolean;
-  tandem: Tandem;
+  tandem?: Tandem;
 };
 
 export default class QuadrilateralShapeModel {
@@ -152,7 +152,8 @@ export default class QuadrilateralShapeModel {
   public constructor( modelBounds: Bounds2, resetNotInProgressProperty: TProperty<boolean>, smoothingLengthProperty: TReadOnlyProperty<number>, providedOptions: QuadrilateralShapeModelOptions ) {
 
     const options = optionize<QuadrilateralShapeModelOptions, QuadrilateralShapeModelOptions>()( {
-      validateShape: true
+      validateShape: true,
+      tandem: Tandem.OPTIONAL
     }, providedOptions );
 
     this.validateShape = options.validateShape;