search

phetsims / quadrilateral

"Quadrilateral" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 3 forks source link

Code Review #398

Closed samreid closed 1 year ago

samreid commented 1 year ago

Today @jessegreenberg met with me and @matthew-blackman to kick off the code review.

Checklist:

**To begin a code review:** * Copy this checklist to a GitHub issue titled "Code Review" and labeled `dev:code-review`. * Delete the **Table of Contents** section, since the links will be incorrect. DONE * Delete items and sections that are not relevant. * Replace {{GITHUB_ISSUE_LINK}} with links to GitHub issues. * Fill in (or delete) the **Specific Instructions** section. --- # PhET Code-Review Checklist (a.k.a "CRC") * The responsible dev is responsible for removing the irrelevant parts * A checked-off item doesn't mean "no problem here", it means "it was reviewed" * Problems can be noted in side issues that reference this issue, or through `// REVIEW` comments in the code ## Specific Instructions _Provide specific instructions here. For example: known problems that will fail CRC items, files that can be skipped, code that is not completed, shared or common code that also needs to be reviewed,... If there are no specific instructions, then delete this section._ ## GitHub Issues The following standard GitHub issues should exist. _If these issues are missing, or have not been completed, pause code review until the issues have been created and addressed by the responsible dev._ - [ ] model.md, see {{GITHUB_ISSUE_LINK}}. Familiarize yourself with the model by reading model.md. Does it adequately describe the model, in terms appropriate for teachers? Has it been reviewed by the sim designer? - [ ] implementation-notes.md, see {{GITHUB_ISSUE_LINK}}. Familiarize yourself with the implementation by reading implementation-notes.md. Does it provide an overview that will be useful to future maintainers? - [ ] results of memory testing for `brands=phet`, see {{GITHUB_ISSUE_LINK}} - [ ] results of memory testing for `brands=phet-io` (if the sim is instrumented for PhET-iO), see {{GITHUB_ISSUE_LINK}} - [ ] performance testing and sign-off, see {{GITHUB_ISSUE_LINK}} - [ ] review of pointer areas, see {{GITHUB_ISSUE_LINK}} - [ ] credits (will not be completed until after RC testing), see {{GITHUB_ISSUE_LINK}} ## **Build and Run Checks** If any of these items fail, pause code review. - [ ] Does the sim build without warnings or errors? - [ ] Does the html file size seem reasonable, compared to other similar sims? - [ ] Does the sim start up? (unbuilt and built versions) - [ ] Does the sim experience any assertion failures? (run with query parameter `ea`) - [ ] Does the sim pass a scenery fuzz test? (run with query parameters `fuzz&ea`) - [ ] Does the sim behave correctly when listener order is shuffled? (run with query parameters `ea&shuffleListeners` and `ea&shuffleListeners&fuzz`) - [ ] Does the sim output any deprecation warnings? Run with `?deprecationWarnings`. Do not use deprecated methods in new code. ## **Memory Leaks** - [ ] Does a heap comparison using Chrome Developer Tools indicate a memory leak? (This process is described [here](https://github.com/phetsims/QA/blob/master/documentation/qa-book.md#47-memory-leak-testing).) Test on a version built using `grunt --minify.mangle=false`. Compare to testing results done by the responsible developer. Results can be found in {{GITHUB_ISSUE_LINK}}. - [ ] For each common-code component (sun, scenery-phet, vegas, …) that opaquely registers observers or listeners, is there a call to that component’s `dispose` function, or is it obvious why it isn't necessary, or is there documentation about why `dispose` isn't called? An example of why no call to `dispose` is needed is if the component is used in a `ScreenView` that would never be removed from the scene graph. Note that it's also acceptable (and encouraged!) to describe what needs to be disposed in implementation-notes.md. - [ ] Are there leaks due to registering observers or listeners? The following guidelines should be followed unless documentation (in-line or in implementation-notes.md) describes why following them is not necessary. * AXON: `Property.link` or `lazyLink` is accompanied by `unlink`. * AXON: `Multilink.multilink` is accompanied by `unmultilink`. * AXON: Creation of `Multilink` is accompanied by `dispose`. * AXON: Creation of `DerivedProperty` is accompanied by `dispose`. * AXON: `Emitter.addListener` is accompanied by `removeListener`. * AXON: `ObservableArrayDef.element*Emitter.addListener` is accompanied by `ObservableArrayDef.element*Emitter.removeListener` * SCENERY: `Node.addInputListener` is accompanied by `removeInputListener` * TANDEM: Creation of an instrumented `PhetioObject` is accompanied by `dispose`. - [ ] Do all types that require a `dispose` function have one? This should expose a public `dispose` function that calls `this.disposeMyType()`, where `disposeMyType` is a private function declared in the constructor. `MyType` should exactly match the filename. ## **Performance** - [ ] Play with sim, identify any obvious performance issues. Examples: animation that slows down with large numbers of objects; animation that pauses or "hitches" during garbage collection. - [ ] If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter `webgl=false`) ## **Usability** - [ ] Are UI components sufficiently responsive? (especially continuous UI components, such as sliders) - [ ] Are pointer areas optimized, especially for touch? (run with query parameter `showPointerAreas`) - [ ] Do pointer areas overlap? (run with query parameter `showPointerAreas`) Overlap may be OK in some cases, depending on the z-ordering (if the front-most object is supposed to occlude pointer areas) and whether objects can be moved. ## **Internationalization** - [ ] Are there any strings that are not internationalized, and does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter `stringTest=X`. You should see nothing but 'X' strings.) - [ ] Does the sim layout gracefully handle internationalized strings that are longer than the English strings? (run with query parameters `stringTest=double` and `stringTest=long`) - [ ] Does the sim stay on the sim page (doesn't redirect to an external page) when running with the query parameter `stringTest=xss`? This test passes if sim does not redirect, OK if sim crashes or fails to fully start. Only test on one desktop platform. For PhET-iO sims, additionally test `?stringTest=xss` in Studio to make sure i18n strings didn't leak to phetioDocumentation, see https://github.com/phetsims/phet-io/issues/1377 - [ ] Avoid using concatenation to create strings that will be visible in the user interface. Use `StringUtils.fillIn` and a string pattern to ensure that strings are properly localized. - [ ] Use named placeholders (e.g. `"{{value}} {{units}}"`) instead of numbered placeholders (e.g. `"{0} {1}"`). - [ ] Make sure the string keys are all perfect, because they are difficult to change after 1.0.0 is published. Guidelines for string keys are: (1) Strings keys should generally match their values. E.g.: ```js "helloWorld": { value: "Hello World!" }, "quadraticTerms": { value: "Quadratic Terms" } ``` (2) If a string key would be exceptionally long, use a key name that is an abbreviated form of the string value, or that captures the purpose/essence of the value. E.g.: ```js // key is abbreviated "iWentToTheStore": { value: "I went to the store to get milk, eggs, butter, and sugar." }, // key is based on purpose "describeTheScreen": { value: "The Play Area is a small room. The Control Panel has buttons, a checkbox, and radio buttons to change conditions in the room." } ``` (3) If string key names would collide, use your judgment to disambiguate. E.g.: ```js "simplifyTitle": { value: "Simplify!" }, "simplifyCheckbox": { value: "simplify" } ``` (4) String keys for screen names should have the general form `"screen.{{screenName}}"`. E.g.: ```js "screen.explore": { "value": "Explore" }, ``` (5) String patterns that contain placeholders (e.g. `"My name is {{first}} {{last}}"`) should use keys that are unlikely to conflict with strings that might be needed in the future. For example, for `"{{price}}"` consider using key `"pricePattern"` instead of `"price"`, if you think there might be a future need for a `"price"` string. (6) It is acceptable to prefix families of strings with a prefix, like so: ```json "material.water": { "value": "Water" }, "material.wood": { "value": "Wood" }, "shape.block": { "value": "Block" }, "shape.cone": { "value": "Cone" }, ``` Nested substructure is not yet fully supported. - [ ] If the sim was already released, make sure none of the original string keys have changed. If they have changed, make sure any changes have a good reason and have been discussed with @jbphet (it is likely that an issue like https://github.com/phetsims/gravity-force-lab/issues/166 should be created). ## **Repository Structure** - [ ] The repository name should correspond to the sim title. For example, if the sim title is "Wave Interference", then the repository name should be "wave-interference". - [ ] Are all required files and directories present? For a sim repository named “my-repo”, the general structure should look like this (where assets/, images/, mipmaps/ or sounds/ may be omitted if the sim doesn’t have those types of resource files). ``` my-repo/ assets/ doc/ images/ *see annotation model.md implementation-notes.md images/ license.json js/ (see section below) mipmaps/ license.json sound/ license.json dependencies.json .gitignore my-repo_en.html my-repo-strings_en.json Gruntfile.js LICENSE package.json README.md ``` *Any images used in model.md or implementation-notes.md should be added here. Images specific to aiding with documentation do not need their own license. - [ ] Verify that the same image file is not present in both images/ and mipmaps/. If you need a mipmap, use it for all occurrences of the image. - [ ] Is the js/ directory properly structured? All JavaScript source should be in the js/ directory. There should be a subdirectory for each screen (this also applies for single-screen sims, where the subdirectory matches the repo name). For a multi-screen sim, code shared by 2 or more screens should be in a js/common/ subdirectory. Model and view code should be in model/ and view/ subdirectories for each screen and common/. For example, for a sim with screens “Introduction” and “Lab”, the general directory structure should look like this: ``` my-repo/ js/ common/ model/ view/ introduction/ model/ view/ lab/ model/ view/ my-repo-main.js myRepo.js myRepoStrings.js ``` - [ ] Do filenames use an appropriate prefix? Some filenames may be prefixed with the repository name, e.g. `MolarityConstants.js` in molarity. If the repository name is long, the developer may choose to abbreviate the repository name, e.g. `EEConstants.js` in expression-exchange. If the abbreviation is already used by another repository, then the full name must be used. For example, if the "EE" abbreviation is already used by expression-exchange, then it should not be used in equality-explorer. Whichever convention is used, it should be used consistently within a repository - don't mix abbreviations and full names. - [ ] Is there a file in assets/ for every resource file in sound/ and images/? Note that there is *not necessarily* a 1:1 correspondence between asset and resource files; for example, several related images may be in the same .ai file. Check license.json for possible documentation of why some resources might not have a corresponding asset file. - [ ] For simulations, was the README.md generated by `grunt published-README` or `grunt unpublished-README`? Common code repos can have custom README files. - [ ] Does package.json refer to any dependencies that are not used by the sim? - [ ] Is the LICENSE file correct? (Generally GPL v3 for sims and MIT for common code, see [this thread](https://github.com/phetsims/tasks/issues/875#issuecomment-312168646) for additional information). - [ ] Does .gitignore match the one in simula-rasa? - [ ] In GitHub, verify that all non-release branches have an associated issue that describes their purpose. - [ ] Are there any GitHub branches that are no longer needed and should be deleted? - [ ] Sim-specific query parameters (if any) should be identified and documented in one .js file in js/common/ or js/ ( if there is no common/). The .js file should be named `{{PREFIX}}QueryParameters.js`, for example ArithmeticQueryParameters.js for the arithmetic repository, or FBQueryParameters.js for Function Builder (where the `FB` prefix is used). - [ ] Query parameters that are public-facing should be identified using `public: true` in the schema. - [ ] All sims should use a color file named `MyRepoColors.js` or, if using abbreviations, `MRColors.js`, and use `ProfileColorProperty` where appropriate, even if they have a single (default) profile (to support color editing and PhET-iO Studio). The `ColorProfile` pattern was converted to `*Colors.js` files in https://github.com/phetsims/scenery-phet/issues/515. Please see [GasPropertiesColors.js](https://github.com/phetsims/gas-properties/blob/master/js/common/GasPropertiesColors.js) for a good example. ## **Coding Conventions** - [ ] Are coding conventions outlined in [PhET's Coding Conventions Document](https://github.com/phetsims/phet-info/blob/master/doc/coding-conventions.md) followed and adhered to? This document deals with PhET coding conventions. You do not need to exhaustively check every item in this section, nor do you necessarily need to check these items one at a time. The goal is to determine whether the code generally meets PhET standards. ## **TypeScript Conventions** - [ ] Are TypeScript conventions outlined in the [TypeScript Conventions Document](https://github.com/phetsims/phet-info/blob/master/doc/typescript-conventions.md) followed and adhered to? ## **Math Libraries** - [ ] `DOT/Utils.toFixed` or `DOT/Utils.toFixedNumber` should be used instead of `toFixed`. JavaScript's `toFixed` is notoriously buggy. Behavior differs depending on browser, because the spec doesn't specify whether to round or floor. ## IE11 - [ ] IE is no longer supported. With that in mind remove IE-specific workarounds - [ ] Use `string.includes` and `string.startsWith` where possible. ## **Organization, Readability, and Maintainability** - [ ] Does the organization and structure of the code make sense? Do the model and view contain types that you would expect (or guess!) by looking at the sim? Do the names of things correspond to the names that you see in the user interface? - [ ] Are appropriate design patterns used? See [phet-software-design-patterns.md](https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md). If new or inappropriate patterns are identified, create an issue. - [ ] Is inheritance used where appropriate? Does the type hierarchy make sense? - [ ] Is composition favored over inheritance where appropriate? See https://en.wikipedia.org/wiki/Composition_over_inheritance. - [ ] Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions). In TypeScript, you can decouple by narrowing the API like so: ```ts public constructor( tickMarksVisibleProperty: Property, model: Pick, // <-- Note the call site can pass the whole model, but we declare we will only use this part of it waterCup: WaterCup, modelViewTransform: ModelViewTransform2, providedOptions?: WaterCup3DNodeOptions ) { ``` - [ ] Is there too much unnecessary decoupling? (e.g. by passing all of the properties of an object independently instead of passing the object itself)? - [ ] Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates? - [ ] Are any significant chunks of code duplicated? In addition to manual identification, tools include: WebStorm _Code > Analyze Code > Locate Duplicates_ and https://github.com/danielstjules/jsinspect. - [ ] Is there anything that should be generalized and migrated to common code? - [ ] Are there any `TODO` or `FIXME` or `REVIEW` comments in the code? They should be addressed or promoted to GitHub issues. - [ ] Are there any [magic numbers](https://en.wikipedia.org/wiki/Magic_number_(programming)) that should be factored out as constants and documented? - [ ] Are there any constants that are duplicated in multiple files that should be factored out into a `{{REPO}}Constants.js` file? - [ ] Does the implementation rely on any specific constant values that are likely to change in the future? Identify constants that might be changed in the future. (Use your judgement about which constants are likely candidates.) Does changing the values of these constants break the sim? For example, see https://github.com/phetsims/plinko-probability/issues/84. - [ ] Is [PhetColorScheme](https://github.com/phetsims/scenery-phet/blob/master/js/PhetColorScheme.ts) used where appropriate? Verify that the sim is not inventing/creating its own colors for things that have been standardized in `PhetColorScheme`. Identify any colors that might be worth adding to `PhetColorScheme`. - [ ] Are all dependent Properties modeled as `DerivedProperty` instead of `Property`? - [ ] All dynamics should be called from Sim.step(dt), do not use window.setTimeout or window.setInterval. This will help support Legends of Learning and PhET-iO. ## **Accessibility** This section may be omitted if the sim has not been instrumented with accessibility features. Accessibility includes various features, not all are always include. Ignore sections that do not apply. ### General - [ ] Are accessibility features integrated well into the code. They should be added in a maintainable way, even if that requires upfront refactoring. ### Alternative Input - [ ] Does the sim pass an accessibility fuzz test? (run with query parameters `fuzzBoard&ea`) - [ ] Does this sim use specific keyboard shortcuts that overlap with global shortcuts? This includes the use of modifier keys like in https://github.com/phetsims/ratio-and-proportion/issues/287. **NOTE: There is currently no list of global shortcuts, and therefore no way to complete this checklist item. See https://github.com/phetsims/phet-info/issues/188.** ### Interactive Description - [ ] Run the entire built sim HTML file through an [HTML validator](https://validator.w3.org/nu/#textarea), does the HTML pass? - [ ] If applicable, are good design patterns used for interactive description, see [interactive-description-technical-guide.md](https://github.com/phetsims/phet-info/blob/master/doc/interactive-description-technical-guide.md) - [ ] Does resetting the simulation also reset the entire PDOM? - [ ] Is `Node.pdomOrder` used appropriately to maintain visual and PDOM layout balance? - [ ] Make sure accessibility strings aren't being adjusted with ascii specific javascript methods like `toUpperCase()`. Remember that one day these strings will be translatable - [ ] Make sure for accessibility strings that all end of sentence periods do not have a leading space before it. Some screen readers will read these as "dot." This can occur often when a clause is conditionally added. ## **PhET-iO** This section may be omitted if the sim has not been instrumented for PhET-iO, but is likely good to glance at no matter. - [ ] Does instrumentation follow the conventions described in [PhET-iO Instrumentation Guide](https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md)? This could be an extensive bullet. At the very least, be sure to know what amount of instrumentation this sim supports. Describing this further goes beyond the scope of this document. - [ ] PhET-iO instantiates different objects and wires up listeners that are not present in the PhET-branded simulation. It needs to be tested separately for memory leaks. To help isolate the nature of the memory leak, this test should be run separately from the PhET brand memory leak test. Test with a colorized Data Stream, and Studio (easily accessed from phetmarks). Compare to testing results done by the responsible developer and previous releases. - [ ] Make sure unused `PhetioObject` instances are disposed, which unregisters their tandems. - [ ] Make sure JOIST `dt` values are used instead of `Date.now()` or other Date functions. Perhaps try `phet.joist.elapsedTime`. Though this has already been mentioned, it is necessary for reproducible playback via input events and deserves a comment in this PhET-iO section. - [ ] Are random numbers using `DOT/dotRandom` as an imported module (not a global), and all doing so after modules are declared (non-statically)? For example, the following methods (and perhaps others) should not be used: `Math.random`, `_.shuffle`, `_.sample`, `_.random`. This also deserves re-iteration due to its effect on record/playback for PhET-iO. - [ ] Like JSON, keys for `undefined` values are omitted when serializing objects across frames. Consider this when determining whether `toStateObject` should use `null` or `undefined` values. - [ ] PhET prefers to use the term "position" to refer to the physical (x,y) position of objects. This applies to both brands, but is more important for the PhET-iO API. See https://github.com/phetsims/phet-info/issues/126 - [ ] Are your IOType state methods violating the API of the core type by accessing private fields? - [ ] When defining a boolean Property to indicate whether something is enabled, use `AXON/EnabledProperty`. This should be done in both the model and the view. If you're using a DerivedProperty, skip this item. - [ ] Do not use translated strings in `phetioDocumentaton` - it changes the PhET-iO API!
samreid commented 1 year ago

As I take an initial look in the code, I'm seeing that things are well-documented and readable, and looking in very good shape. Nice work! I wanted to start a discussion checklist of things to ask about--some of them more stylistic and about consistency across repos, and I wouldn't feel comfortable committing changes without discussing:

Subject: [PATCH] constructor properties
---
Index: js/quadrilateral/view/SideTicksNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/view/SideTicksNode.ts b/js/quadrilateral/view/SideTicksNode.ts
--- a/js/quadrilateral/view/SideTicksNode.ts    (revision 5fbeaa88abbea17b05824e82b473027f60b711db)
+++ b/js/quadrilateral/view/SideTicksNode.ts    (date 1677738730918)
@@ -25,10 +25,8 @@
 const SCRATCH_LINE = new Line( new Vector2( 0, 0 ), new Vector2( 0, 0 ) );

 class SideTicksNode extends Path {
-  private readonly side: QuadrilateralSide;
-  private readonly modelViewTransform: ModelViewTransform2;

-  public constructor( side: QuadrilateralSide, modelViewTransform: ModelViewTransform2 ) {
+  public constructor( private readonly side: QuadrilateralSide, private readonly modelViewTransform: ModelViewTransform2 ) {
     super( null, {
       stroke: QuadrilateralColors.quadrilateralShapeStrokeColorProperty
     } );

In discussion, @jessegreenberg and I agreed not to use that pattern in this sim.


Subject: [PATCH] constructor properties
---
Index: js/quadrilateral/model/QuadrilateralVertex.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/model/QuadrilateralVertex.ts b/js/quadrilateral/model/QuadrilateralVertex.ts
--- a/js/quadrilateral/model/QuadrilateralVertex.ts (revision 5fbeaa88abbea17b05824e82b473027f60b711db)
+++ b/js/quadrilateral/model/QuadrilateralVertex.ts (date 1677739581193)
@@ -56,7 +56,18 @@
   public readonly rightConstrainedProperty = new BooleanProperty( false );
   public readonly topConstrainedProperty = new BooleanProperty( false );
   public readonly bottomConstrainedProperty = new BooleanProperty( false );
-  public readonly numberOfConstrainingEdgesProperty: TReadOnlyProperty<number>;
+  public readonly numberOfConstrainingEdgesProperty = new DerivedProperty( [
+    this.topConstrainedProperty,
+    this.rightConstrainedProperty,
+    this.bottomConstrainedProperty,
+    this.leftConstrainedProperty
+  ], ( topConstrained, rightConstrained, bottomConstrained, leftConstrained ) => {
+    const topVal = topConstrained ? 1 : 0;
+    const rightVal = rightConstrained ? 1 : 0;
+    const bottomVal = bottomConstrained ? 1 : 0;
+    const leftVal = leftConstrained ? 1 : 0;
+    return topVal + rightVal + bottomVal + leftVal;
+  } );

   // A reference to vertices connected to this vertex for so we can calculate the angle at this vertex.
   // The orientation of vertex1 and vertex2 for angle calculations are as shown in the following diagram:
@@ -110,18 +121,6 @@
       return new Bounds2( position.x - HALF_WIDTH, position.y - HALF_HEIGHT, position.x + HALF_WIDTH, position.y + HALF_HEIGHT );
     } );

-    this.numberOfConstrainingEdgesProperty = new DerivedProperty( [
-      this.topConstrainedProperty,
-      this.rightConstrainedProperty,
-      this.bottomConstrainedProperty,
-      this.leftConstrainedProperty
-    ], ( topConstrained, rightConstrained, bottomConstrained, leftConstrained ) => {
-      const topVal = topConstrained ? 1 : 0;
-      const rightVal = rightConstrained ? 1 : 0;
-      const bottomVal = bottomConstrained ? 1 : 0;
-      const leftVal = leftConstrained ? 1 : 0;
-      return topVal + rightVal + bottomVal + leftVal;
-    } );

     this.tandem = tandem;
   }


Subject: [PATCH] constructor properties
---
Index: js/quadrilateral/model/QuadrilateralVertex.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/model/QuadrilateralVertex.ts b/js/quadrilateral/model/QuadrilateralVertex.ts
--- a/js/quadrilateral/model/QuadrilateralVertex.ts (revision 5fbeaa88abbea17b05824e82b473027f60b711db)
+++ b/js/quadrilateral/model/QuadrilateralVertex.ts (date 1677739825789)
@@ -72,7 +72,7 @@
   // controlled by a prototype tangible. See smoothPosition().
   private readonly smoothingLengthProperty: TReadOnlyProperty<number>;

-  // The collection of SMOOTHING_LENGTH number of positions for prototype tangible control. See smoothPosition().
+  // The collection of n <= SMOOTHING_LENGTH number of positions for prototype tangible control. See smoothPosition().
   private readonly positions: Vector2[] = [];

   // in model coordinates, the width of the QuadrilateralVertex


Subject: [PATCH] constructor properties
---
Index: js/quadrilateral/model/ShapeSnapshot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/model/ShapeSnapshot.ts b/js/quadrilateral/model/ShapeSnapshot.ts
--- a/js/quadrilateral/model/ShapeSnapshot.ts   (revision 5fbeaa88abbea17b05824e82b473027f60b711db)
+++ b/js/quadrilateral/model/ShapeSnapshot.ts   (date 1677739911289)
@@ -36,7 +36,7 @@
   public readonly sideBCLength: number;
   public readonly sideDALength: number;
   public readonly sideCDLength: number;
-  private readonly sideLengths: number[];
+  private readonly sideLengths: readonly number[];

   public constructor( shapeModel: QuadrilateralShapeModel ) {
     this.isParallelogram = shapeModel.isParallelogramProperty.value;
samreid commented 1 year ago 
Subject: [PATCH] Use ternary operator, see https://github.com/phetsims/quadrilateral/issues/398
---
Index: js/quadrilateral/view/QuadrilateralIconFactory.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/view/QuadrilateralIconFactory.ts b/js/quadrilateral/view/QuadrilateralIconFactory.ts
--- a/js/quadrilateral/view/QuadrilateralIconFactory.ts (revision 4266288d3e5c19b3d84ef0bdad529510169aa581)
+++ b/js/quadrilateral/view/QuadrilateralIconFactory.ts (date 1677770615285)
@@ -24,13 +24,13 @@
    * Creates an icon for the "Corner Labels" checkbox that toggles visibility of labels on each QuadrilateralVertex.
    */
   createCornerLabelsIcon(): Node {
-    const label = new Text( 'A', QuadrilateralConstants.SCREEN_TEXT_OPTIONS );
+
     const circle = new Circle( QuadrilateralIconFactory.ICON_HEIGHT / 2, {
       stroke: QuadrilateralColors.visibilityIconsColorProperty,
       lineWidth: QuadrilateralIconFactory.ICON_LINE_WIDTH
     } );

-    label.center = circle.center;
+    const label = new Text( 'A', { center: circle.center, ...QuadrilateralConstants.SCREEN_TEXT_OPTIONS } );
     circle.addChild( label );

     return circle;
samreid commented 1 year ago

Current patch for QuadrilateralShapeDetector:

```diff Subject: [PATCH] Add REVIEW comments, see https://github.com/phetsims/quadrilateral/issues/398 --- Index: js/quadrilateral/model/QuadrilateralShapeDetector.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/quadrilateral/model/QuadrilateralShapeDetector.ts b/js/quadrilateral/model/QuadrilateralShapeDetector.ts --- a/js/quadrilateral/model/QuadrilateralShapeDetector.ts (revision a7db24118c6c8642e6e2018e7f282b5f7300cbec) +++ b/js/quadrilateral/model/QuadrilateralShapeDetector.ts (date 1677801894817) @@ -19,79 +19,93 @@ * @author Jesse Greenberg (PhET Interactive Simulations) */ -/* eslint-disable no-bitwise*/ - import quadrilateral from '../../quadrilateral.js'; import QuadrilateralShapeModel from './QuadrilateralShapeModel.js'; import NamedQuadrilateral from './NamedQuadrilateral.js'; // Set up the bits for each shape property. -const CONCAVE = Math.pow( 2, 0 ); -const ONE_PARALLEL_PAIR = Math.pow( 2, 1 ); -const TWO_PARALLEL_PAIR = Math.pow( 2, 2 ); -const TWO_EQUAL_ADJACENT_ANGLE_PAIR = Math.pow( 2, 4 ); // REVIEW: Are we skipping 3? -const ONE_EQUAL_OPPOSITE_ANGLE_PAIR = Math.pow( 2, 5 ); -const TWO_EQUAL_OPPOSITE_ANGLE_PAIR = Math.pow( 2, 6 ); -const ALL_EQUAL_ANGLE = Math.pow( 2, 7 );// REVIEW: Are we skipping 8? -const TWO_EQUAL_ADJACENT_SIDE_PAIR = Math.pow( 2, 9 ); -const ONE_EQUAL_OPPOSITE_SIDE_PAIR = Math.pow( 2, 10 ); -const TWO_EQUAL_OPPOSITE_SIDE_PAIR = Math.pow( 2, 11 ); -const ALL_EQUAL_SIDE = Math.pow( 2, 12 ); -const ONE_ANGLE_PIE = Math.pow( 2, 13 ); +const CONCAVE = 'CONCAVE'; +const ONE_PARALLEL_PAIR = 'ONE_PARALLEL_PAIR'; +const TWO_PARALLEL_PAIR = 'TWO_PARALLEL_PAIR'; +const TWO_EQUAL_ADJACENT_ANGLE_PAIR = 'TWO_EQUAL_ADJACENT_ANGLE_PAIR'; +const ONE_EQUAL_OPPOSITE_ANGLE_PAIR = 'ONE_EQUAL_OPPOSITE_ANGLE_PAIR'; +const TWO_EQUAL_OPPOSITE_ANGLE_PAIR = 'TWO_EQUAL_OPPOSITE_ANGLE_PAIR'; +const ALL_EQUAL_ANGLE = 'ALL_EQUAL_ANGLE'; +const TWO_EQUAL_ADJACENT_SIDE_PAIR = 'TWO_EQUAL_ADJACENT_SIDE_PAIR'; +const ONE_EQUAL_OPPOSITE_SIDE_PAIR = 'ONE_EQUAL_OPPOSITE_SIDE_PAIR'; +const TWO_EQUAL_OPPOSITE_SIDE_PAIR = 'TWO_EQUAL_OPPOSITE_SIDE_PAIR'; +const ALL_EQUAL_SIDE = 'ALL_EQUAL_SIDE'; +const ONE_ANGLE_PIE = 'ONE_ANGLE_PIE'; // Set up shape masks for each shape. The quadrilateral must at least these attributes to be considered a match. -const CONCAVE_MASK = CONCAVE; +const CONCAVE_REQUIREMENTS = [ CONCAVE ]; -const TRIANGLE_MASK = ONE_ANGLE_PIE; +const TRIANGLE = [ ONE_ANGLE_PIE ]; -const DART_MASK = CONCAVE | - ONE_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_ADJACENT_SIDE_PAIR; +const DART = [ + CONCAVE, + ONE_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_ADJACENT_SIDE_PAIR +]; -const KITE_MASK = ONE_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_ADJACENT_SIDE_PAIR; +const KITE = [ + ONE_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_ADJACENT_SIDE_PAIR +]; -const TRAPEZOID_MASK = ONE_PARALLEL_PAIR; +const TRAPEZOID = [ + ONE_PARALLEL_PAIR +]; -const ISOSCELES_TRAPEZOID_MASK = ONE_PARALLEL_PAIR | - TWO_EQUAL_ADJACENT_ANGLE_PAIR | - ONE_EQUAL_OPPOSITE_SIDE_PAIR; +const ISOSCELES_TRAPEZOID = [ + ONE_PARALLEL_PAIR, + TWO_EQUAL_ADJACENT_ANGLE_PAIR, + ONE_EQUAL_OPPOSITE_SIDE_PAIR +]; -const PARALLELOGRAM_MASK = ONE_PARALLEL_PAIR | - TWO_PARALLEL_PAIR | - ONE_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_OPPOSITE_ANGLE_PAIR | - ONE_EQUAL_OPPOSITE_SIDE_PAIR | - TWO_EQUAL_OPPOSITE_SIDE_PAIR; +const PARALLELOGRAM = [ + ONE_PARALLEL_PAIR, + TWO_PARALLEL_PAIR, + ONE_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_OPPOSITE_ANGLE_PAIR, + ONE_EQUAL_OPPOSITE_SIDE_PAIR, + TWO_EQUAL_OPPOSITE_SIDE_PAIR +]; -const RECTANGLE_MASK = ONE_PARALLEL_PAIR | - TWO_PARALLEL_PAIR | - TWO_EQUAL_ADJACENT_ANGLE_PAIR | - ONE_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_OPPOSITE_ANGLE_PAIR | - ALL_EQUAL_ANGLE | - ONE_EQUAL_OPPOSITE_SIDE_PAIR | - TWO_EQUAL_OPPOSITE_SIDE_PAIR; +const RECTANGLE = [ + ONE_PARALLEL_PAIR, + TWO_PARALLEL_PAIR, + TWO_EQUAL_ADJACENT_ANGLE_PAIR, + ONE_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_OPPOSITE_ANGLE_PAIR, + ALL_EQUAL_ANGLE, + ONE_EQUAL_OPPOSITE_SIDE_PAIR, + TWO_EQUAL_OPPOSITE_SIDE_PAIR +]; -const RHOMBUS_MASK = ONE_PARALLEL_PAIR | - TWO_PARALLEL_PAIR | - ONE_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_ADJACENT_SIDE_PAIR | - ONE_EQUAL_OPPOSITE_SIDE_PAIR | - TWO_EQUAL_OPPOSITE_SIDE_PAIR | - ALL_EQUAL_SIDE; +const RHOMBUS = [ + ONE_PARALLEL_PAIR, + TWO_PARALLEL_PAIR, + ONE_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_ADJACENT_SIDE_PAIR, + ONE_EQUAL_OPPOSITE_SIDE_PAIR, + TWO_EQUAL_OPPOSITE_SIDE_PAIR, + ALL_EQUAL_SIDE +]; -const SQUARE_MASK = ONE_PARALLEL_PAIR | - TWO_PARALLEL_PAIR | - TWO_EQUAL_ADJACENT_ANGLE_PAIR | - ONE_EQUAL_OPPOSITE_ANGLE_PAIR | - TWO_EQUAL_OPPOSITE_ANGLE_PAIR | - ALL_EQUAL_ANGLE | - TWO_EQUAL_ADJACENT_SIDE_PAIR | - ONE_EQUAL_OPPOSITE_SIDE_PAIR | - TWO_EQUAL_OPPOSITE_SIDE_PAIR | - ALL_EQUAL_SIDE; +const SQUARE = [ + ONE_PARALLEL_PAIR, + TWO_PARALLEL_PAIR, + TWO_EQUAL_ADJACENT_ANGLE_PAIR, + ONE_EQUAL_OPPOSITE_ANGLE_PAIR, + TWO_EQUAL_OPPOSITE_ANGLE_PAIR, + ALL_EQUAL_ANGLE, + TWO_EQUAL_ADJACENT_SIDE_PAIR, + ONE_EQUAL_OPPOSITE_SIDE_PAIR, + TWO_EQUAL_OPPOSITE_SIDE_PAIR, + ALL_EQUAL_SIDE +]; export default class QuadrilateralShapeDetector { private readonly quadrilateralShapeModel: QuadrilateralShapeModel; @@ -108,7 +122,7 @@ * QuadrilateralShapeModel.updateOrderDependentProperties for more information. */ public getShapeName(): NamedQuadrilateral { - let currentConditionMask = 0; + const conditions: string[] = []; // First, assemble a bitwise value representing the attributes of the current shape. Start by looking for // a triangle, which is a unique case. If we have a triangle we can stop looking for more properties @@ -116,60 +130,60 @@ // to produce the final properties of the shape. const shapeModel = this.quadrilateralShapeModel; if ( _.some( shapeModel.vertices, vertex => shapeModel.isFlatAngle( vertex.angleProperty.value! ) ) ) { - currentConditionMask = currentConditionMask | TRIANGLE_MASK; + conditions.push( ONE_ANGLE_PIE ); } else { if ( _.some( shapeModel.vertices, vertex => vertex.angleProperty.value! > Math.PI ) ) { - currentConditionMask = currentConditionMask | CONCAVE; + conditions.push( CONCAVE ); } if ( shapeModel.parallelSidePairsProperty.value.length > 0 ) { - currentConditionMask = currentConditionMask | ONE_PARALLEL_PAIR; + conditions.push( ONE_PARALLEL_PAIR ); } if ( shapeModel.parallelSidePairsProperty.value.length > 1 ) { - currentConditionMask = currentConditionMask | TWO_PARALLEL_PAIR; + conditions.push( TWO_PARALLEL_PAIR ); } if ( shapeModel.adjacentEqualVertexPairsProperty.value.length > 1 ) { - currentConditionMask = currentConditionMask | TWO_EQUAL_ADJACENT_ANGLE_PAIR; + conditions.push( TWO_EQUAL_ADJACENT_ANGLE_PAIR ); } if ( shapeModel.oppositeEqualVertexPairsProperty.value.length > 0 ) { - currentConditionMask = currentConditionMask | ONE_EQUAL_OPPOSITE_ANGLE_PAIR; + conditions.push( ONE_EQUAL_OPPOSITE_ANGLE_PAIR ); } if ( shapeModel.oppositeEqualVertexPairsProperty.value.length > 1 ) { - currentConditionMask = currentConditionMask | TWO_EQUAL_OPPOSITE_ANGLE_PAIR; + conditions.push( TWO_EQUAL_OPPOSITE_ANGLE_PAIR ); } if ( shapeModel.allAnglesRightProperty.value ) { - currentConditionMask = currentConditionMask | ALL_EQUAL_ANGLE; + conditions.push( ALL_EQUAL_ANGLE ); } if ( shapeModel.adjacentEqualSidePairsProperty.value.length > 1 ) { - currentConditionMask = currentConditionMask | TWO_EQUAL_ADJACENT_SIDE_PAIR; + conditions.push( TWO_EQUAL_ADJACENT_SIDE_PAIR ); } if ( shapeModel.oppositeEqualSidePairsProperty.value.length > 0 ) { - currentConditionMask = currentConditionMask | ONE_EQUAL_OPPOSITE_SIDE_PAIR; + conditions.push( ONE_EQUAL_OPPOSITE_SIDE_PAIR ); } if ( shapeModel.oppositeEqualSidePairsProperty.value.length > 1 ) { - currentConditionMask = currentConditionMask | TWO_EQUAL_OPPOSITE_SIDE_PAIR; + conditions.push( TWO_EQUAL_OPPOSITE_SIDE_PAIR ); } if ( shapeModel.allLengthsEqualProperty.value ) { - currentConditionMask = currentConditionMask | ALL_EQUAL_SIDE; + conditions.push( ALL_EQUAL_SIDE ); } } - // Find matches for the current condition - Order of these checks is very important. conditionMatchesMask + // Find matches for the current condition - Order of these checks is very important. conditionsMatchRequirements // returns true if the condition matches the minimum requirements for a shape. But even after there is a match, // we will continue to search for the most specific attribute matches. let quadrilateralName: NamedQuadrilateral; // First look for a triangle - it is unique in that if we have a triangle there are no more shapes "under" // it that share attributes - so we can return this right away. - if ( this.conditionMatchesMask( currentConditionMask, TRIANGLE_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, TRIANGLE ) ) { quadrilateralName = NamedQuadrilateral.TRIANGLE; } - else if ( this.conditionMatchesMask( currentConditionMask, CONCAVE_MASK ) ) { + else if ( this.conditionsMatchRequirements( conditions, CONCAVE_REQUIREMENTS ) ) { // if concave, the only attribute that can "build-up" from here is the dart property quadrilateralName = NamedQuadrilateral.CONCAVE_QUADRILATERAL; - if ( this.conditionMatchesMask( currentConditionMask, DART_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, DART ) ) { quadrilateralName = NamedQuadrilateral.DART; } } @@ -178,25 +192,25 @@ // most general case, shape properties "build-up" from a convex quadrilateral quadrilateralName = NamedQuadrilateral.CONVEX_QUADRILATERAL; - if ( this.conditionMatchesMask( currentConditionMask, TRAPEZOID_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, TRAPEZOID ) ) { quadrilateralName = NamedQuadrilateral.TRAPEZOID; } - if ( this.conditionMatchesMask( currentConditionMask, ISOSCELES_TRAPEZOID_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, ISOSCELES_TRAPEZOID ) ) { quadrilateralName = NamedQuadrilateral.ISOSCELES_TRAPEZOID; } - if ( this.conditionMatchesMask( currentConditionMask, KITE_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, KITE ) ) { quadrilateralName = NamedQuadrilateral.KITE; } - if ( this.conditionMatchesMask( currentConditionMask, PARALLELOGRAM_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, PARALLELOGRAM ) ) { quadrilateralName = NamedQuadrilateral.PARALLELOGRAM; } - if ( this.conditionMatchesMask( currentConditionMask, RECTANGLE_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, RECTANGLE ) ) { quadrilateralName = NamedQuadrilateral.RECTANGLE; } - if ( this.conditionMatchesMask( currentConditionMask, RHOMBUS_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, RHOMBUS ) ) { quadrilateralName = NamedQuadrilateral.RHOMBUS; } - if ( this.conditionMatchesMask( currentConditionMask, SQUARE_MASK ) ) { + if ( this.conditionsMatchRequirements( conditions, SQUARE ) ) { quadrilateralName = NamedQuadrilateral.SQUARE; } } @@ -206,18 +220,15 @@ /** * Returns true when the condition state matches the conditions of the provided mask. Consider two values - * 10110 (mask) - * 11110 (condition) * - * The & operation returns 10110 (the original mask) if the condition has at least the bits of the mask. This is - * useful because each mask lists out the requirements for a particular shape. If the condition has at least those + * Each mask lists out the requirements for a particular shape. If the condition has at least those * requirements (or more) it will still be considered a match for that shape. * - * @param condition - bits representing the current shape conditions - * @param mask - bits representing the requirements for a particular shape + * @param condition - current shape conditions + * @param requirements - requirements for a particular shape */ - private conditionMatchesMask( condition: number, mask: number ): boolean { - return ( condition & mask ) === mask; + private conditionsMatchRequirements( conditions: string[], requirements: string[] ): boolean { + return requirements.every( maskElement => conditions.includes( maskElement ) ); } } ```
samreid commented 1 year ago

Code review complete. Nice work! The code was well organized and well documented when we got here, and we made several recommendations about how to make it even better. All review comments and side issues have been discussed with @jessegreenberg.

jessegreenberg commented 1 year ago

Excellent, thank you very much @samreid and @matthew-blackman! I will continue to make commits related to review comments against this issue and leave open for any further review or discussion. Ill close this one when all wrapped up.

jessegreenberg commented 1 year ago

OK, I hit every review comment and the side issues that came out of this one. Thank you so much for the review @samreid and @matthew-blackman. All recommendations were great improvements for the code and I went with almost all of them. The following two are the only exceptions. Feel free to give thoughts asynchronously, or we could briefly meet to discuss these and the larger changes in side issues.

// REVIEW: In fuzzing, I saw this value go greater than Math.PI*2. // REVIEW: Can the angle be computed with: const a = vector1.angleBetween( vector2 );

Vector2.angleBetween returns a value between 0 and PI, we need a value from 0-2PI in this sim to support concave quadrilaterals. I fuzzed the sim for 5 minutes or so and never saw an angle above Math.PI * 2. I added a line of doc about why there is a custom angle calculation and then decided not to go farther.

// REVIEW: In this case and others like it, we are plucking several attributes from the model, (potentially) renaming them // and passing them through. This makes it difficult to trace back to the source. // An alternative is to pass the entire model through, and use TypeScript to narrow what is accessible // Note the visibilityModel is also part of the model.

I reivewed the components in QuadrilateralScreenView (where the model is distributed to view subcomponents) and decided not to do this. It didn't seem to improve readability too much. I didn't see a good way for Pick to expose fields on model subcomponents. For example, although visibilityModel is part of the model, I don't want the QuadrilateralDiagonalGuidesNode to take the whole model.visibilityModel, I want it to take just the model.visibilityModel.diagonalGuidesVisibleProperty.

The other two issues you may want to re-sign off on:

samreid commented 1 year ago

The comments above seem good, thanks! Closing.