Code Review

[jbphet]

PhET Code-Review Checklist (CRC) for Quantum Measurement

• 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

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 Write model.md #94. 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 Write implementation-notes.md #95. 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 Memory and performance testing #96
• results of memory testing for brands=phet-io (if the sim is instrumented for PhET-iO),
  see Memory and performance testing #96
• performance testing and sign-off, see Memory and performance testing #96
• review of pointer areas, see Review of pointer areas #98
• credits (will not be completed until after RC testing), see Write credits for publication #97

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 Memory and performance testing #96.
• 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?
• 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!

[marlitas]

• I was surprised to see that the query parameter showGlobalPhase is not set to public: true since it is part of preferences. I'm accustomed to options in preferences having public Query Parameters. If it is not supposed to be public, maybe adding some documentation as to why.

[marlitas]

CoinsModel.ts:

• The initialBias option for CoinsExperimentSceneModel is only passed in once and the value that is passed in is the same as the default. Perhaps this option can be removed?

    this.quantumCoinExperimentSceneModel = new CoinsExperimentSceneModel( {
      systemType: 'quantum',
      initialBias: 0.5,
      tandem: providedOptions.tandem.createTandem( 'quantumCoinExperimentSceneModel' )
    } );

CoinNode.ts:

• Consider making coinFaceParameters a tuple: coinFaceParameters: [CoinFaceParameters, CoinFaceParameters]. This would removed the need for the assertion you have later and also provide typescript support.

CoinsScreenView.ts:

• It doesn't look like this multilink is doing anything right now. The colors are currently the same and additionally it says they are placeholders in the Colors file.

    // Changing the background color based on the experiment type
    Multilink.multilink(
      [
        model.experimentTypeProperty,
        QuantumMeasurementColors.classicalBackgroundColorProperty,
        QuantumMeasurementColors.quantumBackgroundColorProperty
      ],
      ( experimentType, classicalBackgroundColor, quantumBackgroundColor ) => {
        QuantumMeasurementColors.screenBackgroundColorProperty.value = experimentType === 'classical' ? classicalBackgroundColor : quantumBackgroundColor;
      } );

OutcomeProbabilityControl.ts:

• I am unaccustomed to seeing mutable variables written in all caps like a constant. Might make more sense to switch them to camelCase

    let COLOR_SPAN: ( text: string ) => string = text => text;

    let BRA_KET_TITLE_STRING = `( ${ALPHA}|${UP}${KET} + ${COLOR_SPAN( BETA )}|${COLOR_SPAN( DOWN )}${KET} )`;
    let MAGNITUDE_OF_BETA_SQUARED = COLOR_SPAN( `|${BETA}|<sup>2` );

• I see a few use cases of StringUtils.fillIn. This generally appears as a red flag to me with translations, although it looks like it may be covered properly in the DerivedProperty. Either way, this feels like an unusual pattern nowadays and I would encourage documentation to explain why StringUtils.fillIn is being used instead of a PatternStringProperty. (Use PatternStringProperty instead of StringUtils.fillIn #129)
• I don't understand in which scenarios the upProbabilityValueControl or downProbabilityValueControl would change visibility (I could not hit it in PhET brand outside of startup). Is this for PhET-iO? If so document that it is handling PhET-iO specific layout needs.

    Multilink.multilink(
      [
        upProbabilityValueControl.visibleProperty,
        downProbabilityValueControl.visibleProperty
      ], ( upVisible, downVisible ) => {
        title.visible = upVisible || downVisible;
        quantumReadout && quantumReadout.setVisible( upVisible || downVisible );
      }
    );

CoinMeasurementHistogram.ts:

• I would define the type of xAxisLabels when you create the variable instead of using the as type assertion. Like so:

const xAxisLabels: [RichText, RichText] = [
      new RichText(...

SingleCoinViewManager.ts:

• I don't know how strongly I feel about this but wanted to throw it out there. Not sure if it makes more sense to pass through the CoinsExperimentSceneView and make measurementArea a public member. It would remove some of this awkwardness... Response (@jbphet): The code in question is definitely a bit awkward, but it is getting elements from the scene graph and expecting it to be set up a particular way. The problem with changing it as described is that it would require each of these nodes to have a reference to what should eventually be its parent. This is, IMO, even more awkward. So I'm going to leave it as is.

      assert && assert( measurementArea.getParent() instanceof CoinsExperimentSceneView );
      const sceneGraphParent = measurementArea.getParent() as CoinsExperimentSceneView;

• The animations here seem a bit unfortunate in their complexity. I understand that this is due to the clip area frame, and part of me feels that maybe the screen view could have been structured differently so we wouldn't have to worry about adding and removing children from Nodes. I don't think it's worth that refactor at this point, but either way the file could benefit from a bit more acknowledgment and explanation about that complexity.

• I would also make a note at the top of the file that disposal is something to be aware of due to the nature of the coins animating from the preparation area to the measurement area being dynamically created. I would point this out since disposal is not a pattern used throughout the rest of the sim, and your implementation-notes.md also references that elements are in general statically created at the start of the sim.

ViewManagers:

• I wonder if there is a way to make a base class for these? You will know with more certainty than I, but there does seem to be some duplication in accessing the parent of the measurement area, how animations are navigating between layers in the scene graph, and the function abortIngressAnimationForCoinSet.

[marlitas]

General:

• I'm seeing a lot of type assertions sprinkled throughout the code. Enough that I'm starting to wonder if they are weakening the type support in the code. Might be good to revisit those sites. I'll keep an eye out for recommendations as I keep moving through the code as well.

[marlitas]

PhotonsModel.ts

• ExperimentModeTypes is duplicated in Laser.ts as PhotonEmissionMode

Photon.ts

• PossiblePolarizationResult is unused.
• I was recently made aware that there is a preference for export const to be switched to public static fields due to the nature of Object Oriented programming since it clearly associates the constant with what it pertains to.

export const PHOTON_SPEED = 0.3; // meters per second

// pre-fab unit vectors for the four cardinal directions
export const UP = new Vector2( 0, 1 );
export const DOWN = new Vector2( 0, -1 );
export const LEFT = new Vector2( -1, 0 );
export const RIGHT = new Vector2( 1, 0 );

Mirror.ts:

• providedOptions is unused which means any options being passed into it are currently being ignored.

PhotonsScreenView.ts:

• Recommend changing DerivedProperties that are just checking for a constant value to use DerivedProperty.valueEqualsConstant. This could be refactored throughout the code but first spotted in PhotonsScreenView.ts:

Subject: [PATCH] DerivedProperty
---
Index: js/photons/view/PhotonsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/photons/view/PhotonsScreenView.ts b/js/photons/view/PhotonsScreenView.ts
--- a/js/photons/view/PhotonsScreenView.ts	(revision 157595316d5adf433c47fdca23f2405c932b9272)
+++ b/js/photons/view/PhotonsScreenView.ts	(date 1739992347679)
@@ -59,10 +59,7 @@
       tandem: tandem.createTandem( 'singlePhotonExperimentSceneView' )
     } );
     const manyPhotonsExperimentSceneView = new PhotonsExperimentSceneView( model.manyPhotonsExperimentSceneModel, {
-      visibleProperty: new DerivedProperty(
-        [ model.experimentModeProperty ],
-        experimentMode => experimentMode === 'manyPhotons'
-      ),
+      visibleProperty: DerivedProperty.valueEqualsConstant( model.experimentModeProperty, 'manyPhotons' ),
       translation: sceneTranslation,
       tandem: tandem.createTandem( 'manyPhotonsExperimentSceneView' )
     } );

PhotonSprite.ts:

• I see these are instantiated as null because there is a possibility that we could try to access them in step before they are defined. I would document that. It took me some time to figure that out.

  // The sprites used to render the photons.
  private readonly photonInteriorSprite: Sprite | null = null;
  private photonOutlineSprite: Sprite | null = null;

• I believe photonOutlineSprite can be readonly

PhotonDetectorNode.ts:

• You might be able to use a combination of RichText with the tags options and PatternStringProperty here instead. This would remove the StringUtils.fillIn and also not require getBoldColoredString see an example of use in NumberPairs.

     // Create a derived property for the label.
      const labelStringProperty = new DerivedProperty(
        [
          QuantumMeasurementStrings.polarizationDetectorLabelPatternStringProperty,
          QuantumMeasurementStrings.horizontalStringProperty,
          QuantumMeasurementColors.horizontalPolarizationColorProperty
        ],
        ( labelStringPattern, orientationString, highlightColor ) => StringUtils.fillIn( labelStringPattern, {
          orientation: getBoldColoredString( orientationString, highlightColor )
        } )
      );

• Recommend using an alignBox or at least listening to the boundsProperty of label. It seems unorthodox to listen to the StringProperty when what we really care about is in the view. ( I am also seeing a similar pattern that should be adjusted in PolarizingBeamSplitterNode.ts, and ObliquePolarizationAngleIndicator.ts)

labelStringProperty.link( () => {
        label.centerX = body.centerX;
        label.top = body.bottom + 5;
      } );

PhotonDetectionProbabilityPanel.ts:

• Recommend using RichText with the tags option and a PatternStringProperty to do this work: (same with probabilityOfVerticalStringProperty)

const probabilityOfHorizontalStringProperty = new DerivedProperty(...
        const leftSide = `${pString}(${getColoredString( hString, horizontalColor )})`;
        const rightSide = probabilityOfHorizontal === null ?
                          unknownProbabilitySymbol :
                          Utils.toFixed( probabilityOfHorizontal, 2 );
        return `${leftSide} = ${rightSide}`;

ObliquePolarizationAngleIndicator.ts:

• I don't understand why this was needed... The bounds weren't automatically updating as the labels grew/shrank?

// Increasing bounds horizontally so the labels have space to move
      localBounds: new Bounds2( -1.5 * AXIS_LENGTH, -AXIS_LENGTH, 1.5 * AXIS_LENGTH, AXIS_LENGTH ),

PhotonsEquationNode.ts:

• Recommend using RichText with the tags option and a PatternStringProperty to do this work:

const symbolicEquationStringProperty = new DerivedProperty(...
        return `${NString}(${QuantumMeasurementConstants.CREATE_COLOR_SPAN(
          VString, QuantumMeasurementColors.verticalPolarizationColorProperty.value
        )}) - ${NString}(${QuantumMeasurementConstants.CREATE_COLOR_SPAN(
          HString, QuantumMeasurementColors.horizontalPolarizationColorProperty.value
        )})`;
      }
    );

PhotonsExperimentSceneView.ts:

• Why are you not using VerticalCheckboxGroup for your averagePolarizationDisplayControls?

[marlitas]

SpinModel.ts:

• This came up a bit in SternGerlachNode blocks radio buttons on Spin Screen #110, but it might be helpful to document the expectations of what this multilink is handling in laymans terms. It's a lot of booleans to navigate and can feel very daunting to edit in the future.

 // Multilink for changes in the experiment either via source mode or experiment selection
    Multilink.multilink(
      [
        this.currentExperimentProperty,
        this.particleSourceModel.sourceModeProperty,
        this.particleSourceModel.spinStateProperty,
        this.sternGerlachs[ 0 ].blockingModeProperty
      ],
      ( experiment, sourceMode, spinState, blockingMode ) => {...

SternGerlach.ts:

• These Vector2s appear to be constants. They should be refactored to be static members of the SternGerlach class.

  public entranceLocalPosition: Vector2;
  public topExitLocalPosition: Vector2;
  public bottomExitLocalPosition: Vector2;

• blockingModeProperty should use EnumerationProperty instead of Property<EnumerationValue>

ParticleCollection.ts:

• the code in checkParticleBlocking looks suspicious because we're only checking the first element in the sternGerlachs array. I would recommend documenting that implementation, as well as why we're only grabbing the second element in the isSpinUp array.
• I think there needs to be more documentation about stages and their relationship to the sternGerlachs, and isSpinUp. There is a reason all of those array only have 3 elements but the relationship is not entirely clear. I'm also wondering if they should actually be arrays, or if they would benefit from being an object literal { firstSomething: SternGerlach, secondSomething: SternGerlach, thirdSomething: SternGerlach }. The arrays make some of these access points feel potentially fragile in case something gets out of order or modified.
• This is another example of the above points where I am very confused about why we're accessing each of these array indices when we are:

...else if ( !usingSingleApparatus && !particle.stageCompleted[ 2 ] ) {
        const sternGerlach = particle.isSpinUp[ 1 ] ? this.model.sternGerlachs[ 1 ] : this.model.sternGerlachs[ 2 ];

• sourceModeProperty should use EnumerationProperty instead of Property<EnumerationValue>

ParticleSourceNode.ts:

• Consider renaming particleSourceBarrelWidth to particleSourceBarrelDimension since it is used for both the height and the width of the barrel.

SternGerlachNode.ts:

• This is interesting and I'm curious about why you decided to not use any of the quadraticCurve functions that Shape provides. I zoomed in and didn't see anything degrade performance wise, but I still believe that Shape would provide more rendering support in general... AV: I feel more confortable controlling the curve equation itself, quadraticCurve uses beziers and the control points have always seemed a bit confusing to me. But will keep in mind for future implementations.

for ( let i = 0; i < 1; i += 0.1 ) {
      curveUpShape.lineTo( -STERN_GERLACH_WIDTH / 2 + i * STERN_GERLACH_WIDTH, curveFunction( i ) * STERN_GERLACH_HEIGHT / 4 );
      curveDownShape.lineTo( -STERN_GERLACH_WIDTH / 2 + i * STERN_GERLACH_WIDTH, -curveFunction( i ) * STERN_GERLACH_HEIGHT / 4 );
    }

• Why did you decide to use new Path( Shape.rect(... instead of Rectangle for the mainAparatus? I think it's probably fine to leave if you'd like, but Rectangle in general just reads a little easier.

SpinStatePreparationArea.ts:

• ProbabilityValueControl is in the Coins repo but is used in SpinStatePreparationArea. Move to the common repo.
• Utils.toFixed is deprecated
• Why Are you updating the bounds every time the currentExperimentProperty changes? Do you actually want to listen to the boundsProperty instead and update center based on that?

// Reposition for a wider layout
    model.currentExperimentProperty.link( () => {
      this.centerY = layoutBounds.centerY;
    } );

[marlitas]

BlochSphereModel.ts

• selectedStateDirectionProperty & equationBasisProperty should use EnumerationProperty instead of Property<EnumerationValue>

BlochSphereNode.ts:

• Utils.toRadians is deprecated (Address deprecation of dot.Utils #131)
• This string pattern is used in a few other spots with different variable abstracting the fact that they are the same:

`|${UP}<sub>Z</sub>${KET}`

SpinMeasurementArea.ts:

`|${QuantumMeasurementConstants.SPIN_UP_ARROW_CHARACTER}<sub>Z</sub>${QuantumMeasurementConstants.KET}`

• I would recommend listening to the BoundsProperty of the UI element to set center rather than the stringProperty.

QuantumMeasurementStrings.blochSphereStringProperty.link( () => {
      title.centerX = upStateLabel.centerX;
    } );

BlochSphereMeasurementArea.ts:

• Perhaps add some documentation to describe the nesting VBoxes used to create measurementControlPanel

General:

• I started noticing that many Utils functions are now deprecated. I stopped writing them all down because it became too much. I suggest looking through the code to update patterns. (See Address deprecation of dot.Utils #131)

[marlitas]

Code Review Is complete. I'm going to take one last pass through the implementation-notes.md tomorrow since it was still getting finished when I first read through it, but other than that everything is good to go!

Most items are documented in this issue as a checklist, but more general issues were also created that link back to this one and are assigned to @jbphet and @AgustinVallejo. In addition you can find // REVIEW comments throughout the code. These are generally smaller scale comments, questions, or reference documentation clean up.

Overall, I think this code is in a really good state especially for it's complexity. I did not review expecting to have a full understanding of the logic in the sim, but it was set up in a way that made it easy for me to follow for the most part. Congrats both on some really lovely work!