Code review

[pixelzoom]

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 #116. 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 #117. 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 PhET and PhET-iO brands, see #120
• performance testing and sign-off, see #121.
• review of pointer areas, see #119
• credits (will not be completed until after RC testing), see #23

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? Run profile-file-size.ts to see how the size breaks down, and see if anything is unexpected.
• 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 #120.
• 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.
• All classes that require a dispose function should have one. There are many reasonable ways to implement dispose, including but not limited to: (a) declaring a private function like this.dispose{{CLASS_NAME}}() in the constructor, where {{CLASS_NAME}} should exactly match the class name. (b) Using Disposable.ts/disposeEmitter, (c) calling super.dispose() and myChild.dispose() etc.
• All classes that do not properly override dispose should either (a) use isDisposable: false, or (b) implement a dispose method that calls Disposable.assertNotDisposable. Use (a) for classes that inherit a dispose method from Disposable. Use (b) for classes that inherit a dispose method from something other than Disposable. The goal here is to prevent clients from calling dispose for something that does not properly clean itself up, creating a memory leak. This is especially important for common code, but is good defensive programming for sim-specific code.

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. This is relevant in cases where order should be translatable.
• Use named placeholders (e.g. "{{value}} {{units}}") instead of numbered placeholders (e.g. "{0} {1}").
• Inspect ${REPO}-strings_en.json and verify that all string keys conform
  to string key conventions. String keys are difficult to change after a sim has been published, and they appear in the PhET-iO API (and Studio) as the phetioIDs for StringProperties.
• 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
    sounds/
      license.json
    dependencies.json
    .gitignore
    my-repo_en.html
    my-repo-strings_en.json
    Gruntfile.cjs
    LICENSE
    package.json
    tsconfig.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. The abbreviation should be all uppercase letters; e.g. MOTHAConstants, not MotHAConstants for "Model of the Hydrogen Atom".

• Is there a file in assets/ for every resource file in sounds/ 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.

• Is the LICENSE file correct? (Generally GPL v3 for sims and MIT for common code, see this thread for additional information). Note that "common" repos that are mostly sim-specific code (like circuit-construction-kit-common and density-buoyancy-common) should be GPL v3, see other thread.

• 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.ts or, if using abbreviations, MRColors.ts, 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.ts files in PhET-iO instrumentation for ColorProfile scenery-phet#515. Please see GasPropertiesColors.ts for a good example.

• Sim-specific preferences (which appear in the Simulation tab of the Preferences dialog) should appear as Properties in {{PREFIX}}Preferences.ts, for example KeplersLawsPreferences.ts. The initial value of each such preferences Property should be set via a query parameter from {{PREFIX}}QueryParameters.js.

• Does package.json refer to any dependencies that are not used by the sim?

• Does package.json, tsconfig.json, or eslint.config.mjs include any config that was only needed for development? For example:

  • Lint rules turned off or made more graceful.
  • Certain testing or simFeatures turned off with flags.

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.

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). If you only need a few fields from a large object, pass them in as separate parameters. The threshold for the number of parameters is up to you - use your judgement. Alternatively in TypeScript, you can decouple by narrowing the API using Pick, but this is a bit of a hack. Here's an example:

class MyClass {
  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.
• 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.
  • This means that there should be no TODOs pointing to One issue to hold every TODO in the project tasks#1129
  • This means that your repo should have the "todo-should-have-repo" lint rule enabled.
• 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 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 the sim use ScreenView.pdomPlayAreaNode.pdomOrder and Screenview.pdomControlAreaNode.pdomOrder to control the traversal order and categorize Nodes in the PDOM? See alternative-input-quickstart-guide.md for more information.
• Does this sim use KeyboardListener? If so, keys should be defined with HotkeyData. Both the KeyboardListener and keyboard help dialog content should use the HotkeyData. This factors used keys into a single location and supports auto-generated documentation about used keys.
• 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. Review https://phetsims.github.io/binder/ to see a list of all global modifier keys in common code.

Interactive Description

• Run the entire built sim HTML file through an HTML validator, does the HTML pass? See HTML validator results in many Error/Warning/Info messages. scenery#1687.
• If applicable, are good design patterns used for interactive description, see interactive-description-technical-guide.md
• Does resetting the simulation also reset the entire PDOM?
• 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? 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 location vs position phet-info#126
• When defining a boolean Property to indicate whether something is enabled with a tandem name of enabledProperty, 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!

[pixelzoom]

@AgustinVallejo This sim is ready for code review. Let me know if you'd like to meet to discuss.

Note that I'm still working on #115, debugging and optimizing the Schrodinger view. So please skip these 3 files in your review:

• SchrodingerNode.ts
• SchrodingerOrbitalNode.ts
• SchrodingerImageCache.ts

[pixelzoom]

@AgustinVallejo and I had a kick-off meeting at 11:00AM on 2/11/2025.

[AgustinVallejo]

2025-02-17: Today I'm fully starting the review process. Below is a checklist of things that should be addressed:

---

• There's still a TODO in the implementation-notes.md

@pixelzoom: ... as noted in #125 (comment). I’ll be wrapping that up today.

---

• Margin: 10 shows up 8 times. Should that be enought for factoring into a constant?

While all of these have the value 10, they are margins for different types of UI components -- ComboBox, Panel, and Dialog. So it's not appropriate to factor out a single constant - that could do more harm than good. I have factored out local const MARGINS = 10 within a couple of these UI components - see f89811a and 0a01065.

---

• The IDE suggests to use the correct spelling of 'Schrödinger'. Just wanted to point it out, since it's a fair correction. But I suppose this would be an annoying fix to change only in comments and tip-toe around variables and method names.

@pixelzoom: Changed to 'Schrödinger' when it appears in comments and is not included in the name of a class. See d3de41f.

---

• Was the gridSize of 35 pixels chosen for performance reasons? It feels like it could be significantly bigger without a very significant loss. And currently I still see visual artifacts due to interpolation.

You were probably looking at an old implementation of the Schrodinger view, before #129 was completed. The new implementation achieves smooth orbitals with small grid sizes. gridSize=35 was chosen via a very specific test process that involved the designers.

Test protocol: #129 (comment)
Test results: #129 (comment)
Consensus on defaultValue: #129 (comment)

And in MOTHAQueryParameters:

  gridSize: {
    type: 'number',
    defaultValue: 35, // See https://github.com/phetsims/models-of-the-hydrogen-atom/issues/129#issuecomment-2670189907
    isValidValue: value => Number.isInteger( value ) && ( value >= 10 )
  },

---

• coding-conventions.md "Properties should use type-specific subclasses where appropriate (e.g. BooleanProperty, NumberProperty, StringProperty) or provide documentation as to why they are not." there are 12 Property<number> and 8 Property<boolean>

You are confusing declaration and definition, probably because coding-conventions.md is unclear about the distinction.

coding-conventions.md is referring to definition, which is where the object is instantiated and assigned to a field. This is where you should be using new BooleanProperty, new NumberProperty, new StringProperty, etc. because they provide PhET-iO instrumentation. Inspecting MOTHA, there are only 8 occurences of new Property, and they are all for Properties that have sim-specific value types.

Declarations (of fields, parameters, return types,...) should generally use less-specific types, with the appropriate type that provides read-only or write access as needed. These type definitions are Property<boolean>, TReadOnlyProperty<boolean>, Property<number>, TReadOnlyProperty<number>, etc. This is where you are finding 12 Property<number> and 8 Property<boolean>, and they are correct.

Let me know if you need further clarification about this topic.

---

• Just a note about A11y Tier 1: In QM, we're adding all the a11yName and a11yHelpText under QMStrings.a11y.translatable.etc.... I realize the a11y strings in MOTHA are not under the translatable nesting. Perhaps make sure if that's something that should be moved before publication.

@pixelzoom: Addressed in #133.

[AgustinVallejo]

Something minor I saw during stringTest, this string can overlap with the dotted line (photon legend)

[pixelzoom]

... So please skip these 3 files in your review:

@AgustinVallejo The Schrodinger view implementation is complete, so all files are now ready for review.

[pixelzoom]

@AgustinVallejo said:

Something minor I saw during stringTest, this string can overlap with the dotted line (photon legend) ...

Thanks, fixed in 8567423. Screenshot below.

[pixelzoom]

@AgustinVallejo Thanks for the REVIEW comments that you've added so far. I've addressed them in the above commits. Feel free to inspect the commits, and let me know if you have questions or disagree with how I've addressed them.

There are 2 REVIEW comments that cannot be addressed, described below. Let me know if you have questions about these.

---

In BohrModel.ts:

// Minimum time (in sec) that the electron must be in a state before transitions can occur.
// REVIEW: Since this is time to absorption OR emission, would rename to something more general like MIN_TIME_IN_STATE_BEFORE_TRANSITION https://github.com/phetsims/models-of-the-hydrogen-atom/issues/125
const MIN_TIME_IN_STATE_BEFORE_ABSORPTION = 0.75;
const MIN_TIME_IN_STATE_BEFORE_STIMULATED_EMISSION = 1;
const MIN_TIME_IN_STATE_BEFORE_SPONTANEOUS_EMISSION = 1;

This is NOT "time to absorption OR emission". These are the individual times for absorption and 2 types of emission (stimulated and spontaneous). They each require a separate const, so that they can be tuned separately.

---

In SchrodingerQuantumNumbers.ts:

// This should match STATE_SCHEMA, but with JavaScript types.
type SchrodingerQuantumNumbersStateObject = {
  n: number;
  l: number;
  m: number;
};

// This should match SchrodingerQuantumNumbersStateObject, but with IOTypes.
// REVIEW: Should an assertion or test be added to ensure these two matching? https://github.com/phetsims/models-of-the-hydrogen-atom/issues/125
const STATE_SCHEMA = {
  n: NumberIO,
  l: NumberIO,
  m: NumberIO
};

SchrodingerQuantumNumbersStateObject is a TypeScript type, so there is no way that I can assert/test that it matches STATE_SCHEMA. If they do not match, TypeScript will complain, or (worst case) PhET-iO will fail, and that seems sufficient.

[pixelzoom]

@AgustinVallejo I addressed the items in #125 (comment) and made notes in that comment. Let me know if you'd like to discuss any of those.

[AgustinVallejo]

I guess this one is a bit unclear with all the Tier 1 stuff going on, but I will consider it done:

Are accessibility features integrated well into the code? They should be added in a maintainable way, even if that requires upfront refactoring.

[AgustinVallejo]

Code review is done! Back to @pixelzoom for future steps.

[pixelzoom]

Thanks @AgustinVallejo!

We discussed a few topics this morning via Zoom. And I have addressed the remaining REVIEW issues (see above commits). So closing.