Code review #331

jbphet · 2023-07-13T15:57:01Z

PhET Code-Review Checklist (a.k.a "CRC")

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

Greenhouse Effect is ready for code review. This sim is a bit unusual for a couple of reasons. One is that it is already available on the main site because it had been published as a prototype and then a decision was made to promote it to a full release. Another is that it contains all of the code for the Molecules and Light (M&L) sim, since one of the screens of Greenhouse Effect was supposed to be a variation of the M&L behavior, but that has not happened yet. So, for this review, please do not review any of the code in the "micro" directory. The primary functional difference between the code that was published a few months ago and the current code on master is that support for phet-io has been added to the 2nd and 3rd screens and improved in the first.

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 Complete model.md #325. 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 Complete implementation-notes.md #326. 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 Test for memory leaks as part of code review #327
results of memory testing for brands=phet-io (if the sim is instrumented for PhET-iO), see Test for memory leaks as part of code review #327
performance testing and sign-off, see Performance testing and sign-off #328
review of pointer areas, see Review pointer areas #329
credits (will not be completed until after RC testing), see Finalize credits for phet-io release #330

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&listenerOrder=random and ea&listenerOrder=random&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.) Test on a version built using grunt --minify.mangle=false. Compare to testing results done by the responsible developer. Results can be found in Test for memory leaks as part of code review #327.
❌ See Complete implementation-notes.md #326 and Identify things that are not disposable #336. 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. See Identify things that are not disposable #336

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

Does the sim behave correctly with dynamic layout, to support dynamic locale? Run with stringTest=dynamic and use the left/right arrow keys.
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. They are difficult to change after 1.0.0 is published. They should be
literal, they should be explicit, and they should be consistent. This will also be the PhET-iO phetioID name in Studio etc. See Use camelCase for string keys #290 for some additional information for this particular sim
Guidelines for string keys are:

(1) Strings keys should generally match their values. E.g.:
```
"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.:
```
// 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.:
```
"simplifyTitle": {
   value: "Simplify!"
},
"simplifyCheckbox": {
   value: "simplify"
}
```
(4) String keys for screen names should have the general form "screen.{{screenName}}". E.g.:
```
  "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:

  "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 Translations will lose some strings the next time this is published off of master gravity-force-lab#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 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
PhET-iO instrumentation for ColorProfile scenery-phet#515. Please see
GasPropertiesColors.js
for a good example.

Coding Conventions

Are coding conventions outlined in PhET's Coding Conventions Document 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 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. 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). (A separate issue was logged about this and subsequently addressed, see Unnecessary coupling #339.) In TypeScript, you can decouple by narrowing the API like so:

  public constructor( tickMarksVisibleProperty: Property<boolean>,
                      model: Pick<IntroModel, 'changeWaterLevel'>, // <-- 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? To see file sizes for TypeScript sims, run this shell command:

cd {{repo}}/js ; wc -l `find . -name "*.ts" -print` | sort

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. (An issue was logged about this as a result of the review and subsequently addressed, see Duplicate code #337.)
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 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 allow minimum rows to go to "1" and address dependency on current minimum of "5" plinko-probability#84.
Is PhetColorScheme 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 sim supports alternative input, and some amount of description has been implemented, but it is not complete. The general plan is that full description will be implemented "later", but it is not clear at this time when that will be. For the purposes of this review, code related to description should be largely ignored since it is likely to change quite a bit when the time is taken to fully implement it.

General

⚠️ @pixelzoom reviewed pdomOrder. But I'm not familiar with description/describers, so I cannot evaluate.
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)
⚠️ Looking at the keyboard help dialogs, I do not see any sim-specific shortcuts. So this checklist items seems to be "not applicable". Does this sim use specific keyboard shortcuts that overlap with global shortcuts? This includes the use of modifier keys like in Unintended zoom/keyboard behavior ratio-and-proportion#287. NOTE: There is currently no list of global shortcuts, and therefore no way to complete this checklist item. See Create a list of global keyboard shortcuts. phet-info#188.

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.

The text was updated successfully, but these errors were encountered:

pixelzoom · 2023-07-13T18:27:59Z

Code review is done. The sim is in great shape. All issues that I created are linked above, and there were no "REVIEW" comments added to the code. Note the ❌ and ⚠️ annotations in the checklist above. Back to @jbphet...

jbphet · 2023-08-23T17:35:01Z

Everything above has been addressed and the sim is now in the RC stage, so I'm going to go ahead and close this.

jbphet added the dev:code-review label Jul 13, 2023

jbphet assigned pixelzoom Jul 13, 2023

pixelzoom assigned jbphet and unassigned pixelzoom Jul 13, 2023

pixelzoom added the status:ready-for-review label Jul 13, 2023

jbphet closed this as completed Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review #331

Code review #331

jbphet commented Jul 13, 2023 •

edited

Loading

pixelzoom commented Jul 13, 2023

jbphet commented Aug 23, 2023

Code review #331

Code review #331

Comments

jbphet commented Jul 13, 2023 • edited Loading

PhET Code-Review Checklist (a.k.a "CRC")

Specific Instructions

GitHub Issues

Build and Run Checks

Memory Leaks

Performance

Usability

Internationalization

Repository Structure

Coding Conventions

TypeScript Conventions

Math Libraries

IE11

Organization, Readability, and Maintainability

Accessibility

General

Alternative Input

PhET-iO

pixelzoom commented Jul 13, 2023

jbphet commented Aug 23, 2023

jbphet commented Jul 13, 2023 •

edited

Loading