Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Review #165

Closed
64 of 71 tasks
zepumph opened this issue Aug 23, 2023 · 2 comments
Closed
64 of 71 tasks

Code Review #165

zepumph opened this issue Aug 23, 2023 · 2 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Aug 23, 2023

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

Table of Contents

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?
  • (memory testing was not complete at the time of this review) results of memory testing for brands=phet, see Memory test #119
  • performance testing and sign-off #174
  • review of pointer areas #175
  • credits (will not be completed until after RC testing), see Update credits #123

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)
  • (The fuzz testing went fine with several different seeds, but I found two bugs fairly quickly while doing manual testing - see Arrow doesn't disappear from half-life chart in some randomized listener cases #171 and Buttons for adding nucleons can get stuck in disabled state when shuffling listeners #172 - so I'd suggest the devs do more testing on this) 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 {{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.
  • ⚠️ (reviewer recommended some docs, see Add docs to implementation-notes.md about not needing to dispose much #177) 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. This should expose a public dispose function that
    calls this.dispose{{CLASS_NAME}}(), where dispose{{CLASS_NAME}} is a private function declared in the
    constructor. {{CLASS_NAME}} should exactly match the class name. Reviewer note: The only things disposed in this sim are particles and particle views, and these come from the 'shred' repo and already had dispose functions, so we're good to go.
  • 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.
    Reviewer note: N/A since no new classes requiring dispose functions were added.

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) _Reviewer comment: Sim does not use WebGL, so we're good to go on this.

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}").

  • 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. 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.

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. The abbreviation should be all uppercase
    letters; e.g. MOTHAConstants, not MotHAConstants for "Model of the Hydrogen Atom".

  • ⚠️ There is no asset file for fullNuclideChart.png, but it may not be possible to have one, see Can we retain any sort of asset file for the fullNuclideChart.png image? #180 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.

  • The README file needs to be updated, see Update the README file #181 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.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.

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

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:
  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?
  • This reviewer added a number of REVIEW comments, and there was one TODO that could potentially be addressed
    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 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. Reviewer note: The sim has keyboard nav, but no description, voicing, or sound, so I just looked at things like pdomOrder, which seemed fine.

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 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.
    Reviewer note: There don't appear to be any sim-specific keyboard shortcuts, so this is good to go.
  • Is Node.pdomOrder used appropriately to maintain visual and PDOM layout balance?
jbphet added a commit that referenced this issue Aug 23, 2023
jbphet added a commit that referenced this issue Aug 23, 2023
jbphet added a commit that referenced this issue Aug 24, 2023
jbphet added a commit that referenced this issue Aug 24, 2023
jbphet added a commit to phetsims/shred that referenced this issue Aug 25, 2023
jbphet added a commit to phetsims/shred that referenced this issue Aug 25, 2023
@jbphet
Copy link
Contributor

jbphet commented Aug 25, 2023

Code review is complete. Overall the sim seems to be in good shape. The code is well structured and documented, and it's a very cool sim! Back to @Luisav1 and @zepumph for follow up on the issues that were logged during the review and the REVIEW comments that were placed in the code.

@zepumph
Copy link
Member Author

zepumph commented Aug 28, 2023

Alright! All review comments have been handled, and the remaining work here is covered by side issues. Thanks @jbphet so much for the thorough and speedy code review. Closing

@zepumph zepumph closed this as completed Aug 28, 2023
zepumph added a commit to phetsims/aqua that referenced this issue Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants