Trig Tour Code Review

[dubson]

Build and Run Checks

YES Does the sim pass jshint?
YES Does the sim build without errors?
YES Does the sim start up? (requirejs and built versions)
ALL GOOD. Does the sim experience any assertion failures? (run with query parameter 'ea')
YES Does the sim pass a scenery fuzzer test? (run with query parameter 'fuzzMouse')
ALL GOOD. Are there any strings that are not being internationalized? (run with query parameter 'stringTest=x', you should see nothing but 'x' strings)
YES Does the sim layout gracefully handle internationalized strings that are twice as long as the English strings? (run with query parameter 'stringTest=double')
YES Does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter 'stringTest=X')
Repository structure

YES Are all required files and directories present?

For a sim repository named “my-repo”, the general structure should look like this (where assets/, audio/ or images/ may be omitted if the sim doesn’t have those types of assets).

my-repo/
assets/
my-repo-screenshot.png
audio/
license.json
doc/
model.md
implementation-notes.md
images/
license.json
js/
my-repo-config.js
my-repo-main.js
.gitignore
my-repo_en.html
my-repo-strings_en.json
Gruntfile.js
LICENSE
package.json
README.md
For a common-code repository, the structure is similar, but some of the files and directories may not be present if the repo doesn’t have audio, images, strings, or a demo application.

YES 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). 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 “Custom”, the general directory structure should look like this:

my-repo/
js/
common/
model/
view/
custom
model/
view/
introduction
model/
view/
my-repo-config.js
my-repo-main.js
YES or THERE WILL BE Is there a file in assets/ for every file in audio/ and images/?
YES Are all license.json files populated? audio/license.json and images/license.json should enumerate all files in those directories. For the format of license.json files, go here.
YES. Does the README.md format and content match PhET guidelines? Was it generated by grunt generate-published-README or grunt generate-unpublished-README?
YES Does Gruntfile.js point to chipper/js/grunt/Gruntfile.js?
DON'T UNDERSTAND. Are dependencies in package.json the same as js/-config.js? Do they match what is actually used by the sim? (ie, no missing dependencies, no unused dependencies)
YES Is the LICENSE file correct? (Generally GPL v3 for sims, MIT for common code, but there are exceptions to this.)
YES(APPARENTLY, SAME AS OTHER SIMS) Does .gitignore match other repositories?
YES. Does *-main.js follow PhET conventions? Does it contain correct credits (options.credits passed to Sim constructor)?
TO BE DONE LATER. Is the format of assets/-screenshot.png correct? (2048x1344, PNG, 32-bit color) Has the browser chrome been stripped off of the screenshot?
IT IS ALL GOOD. Are there git repository branches that are no longer used and should be pruned?
Coding conventions

MOSTLY, I THINK. Does the code style meet PhET's code-style guidelines, as described in the “Coding Style Guidelines” section of "PhET Development Overview"?
Is the code formatted according to PhET conventions? See phet-idea-code-style.xml for IntelliJ IDEA code style.
Documentation

MOSTLY, I THINK. Are documentation conventions followed, as described in the “Coding Style Guidelines” section of the PhET Development Overview?
NOT YET, TO BE DONE BY DUBSON. Does model.md adequately describe the model, in terms appropriate for teachers?
NOT YET, TO BE DONE BY DUBSON. Does implementation-notes.md adequately describe the implementation, with an overview that will be useful to future maintainers?
NOT APPLICABLE Are sim-specific query parameters (if any) identified and documented in one location?
Common Errors

DONE! Is Math.random used where dot.Util.randomSymmetric should be used? Math.random does not treat positive and negative numbers symmetrically, see phetsims/dot#35 (comment)
DONE! Is toFixed used where dot.Util.toFixed or dot.Util.toFixedNumber should be used? JavaScript's toFixed is notoriously buggy, behavior differs depending on browser, because the spec doesn't specify whether to round or floor.
Organization, Readability, Maintainability

LOOKS OK TO ME, BUT WHAT DO I KNOW. 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?
Is inheritance used where appropriate? Does the type hierarchy make sense?
Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)
Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates?
DON'T THINK SO. Are any significant chunks of code duplicated? This will be checked manually as well as with https://github.com/danielstjules/jsinspect
DON'T THINK SO. Is there anything that should be generalized and migrated to common code?
Performance, Usability

TESTING IN PROGRESS. Does the sim perform as desired across the range of supported platforms? (eg, not too slow on slow platforms, not too fast on fast platforms)
If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter 'webgl=false')
THEY LOOK GOOD. Are UI components sufficiently responsive? (especially continuous UI components, such as sliders)
I THINK SO. Are pointer areas optimized, especially for touch? (run with query parameter 'showPointerAreas')
THEY LOOK GOOD. Do pointer areas overlap? (run with query parameter 'showPointerAreas')
Memory Leaks

PROBABLY NOT, BUT I CANT TELL FOR CERTAIN. THOUSAND OF NEW SHAPES ARE CREATED ROUTINELY. IS THIS AN ISSUE? Does a heap comparison using Chrome Developer Tools indicate a memory leak? (Describing this process is beyond the scope of this document.)
DON'T KNOW HOW TO CHECK. Are there any leaks due to registration of AXON observers?
ALL LINKS ARE PERMANENT. IS THIS OK?? For each call to Property.link or PropertySet.link, is there a corresponding unlink, or documentation about why an unlink is unnecessary?
NOT APPLICABLE. For each DerivedProperty or Multilink created, is there a corresponding detach, or documentation about why a detach is unnecessary?
I DONT UNDERSTAND. For each common-code component (sun, scenery-phet, vegas, …) that opaquely registers an AXON observer, is there a call to that component’s dispose function, or documentation about why dispose is unnecessary?
NOT APPLICABLE. Are there any leaks due to registration of components with TANDEM? tandem.addInstance should be accompanied by tandem.removeInstance or documented why removeInstance is unnecessary.

[jessegreenberg]

PhET code-review checklist

Build and Run Checks

• Does the sim pass jshint?
• Does the sim build without errors?
• Does the sim start up? (requirejs and built versions)
• Does the sim experience any assertion failures? (run with query parameter 'ea')
• Does the sim pass a scenery fuzzer test? (run with query parameter 'fuzzMouse')
• Are there any strings that are not being internationalized? (run with query parameter 'stringTest=x', you should see nothing but 'x' strings)
• Does the sim layout gracefully handle internationalized strings that are twice as long as the English strings? (run with query parameter 'stringTest=double')
• Does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter 'stringTest=X')

Repository structure

• Are all required files and directories present?

  For a sim repository named “my-repo”, the general structure should look like this (where assets/, audio/ or images/ may be omitted if the sim doesn’t have those types of assets).

   my-repo/
      assets/
         my-repo-screenshot.png
      audio/
         license.json
      doc/
         model.md
         implementation-notes.md
      images/
         license.json
      js/
         my-repo-config.js
         my-repo-main.js
      .gitignore
      my-repo_en.html
      my-repo-strings_en.json
      Gruntfile.js
      LICENSE
      package.json
      README.md

For a common-code repository, the structure is similar, but some of the files and directories may not be present if the repo doesn’t have audio, images, strings, or a demo application.

• 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). 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 “Custom”, the general directory structure should look like this:

   my-repo/
      js/
         common/
            model/
            view/
         custom
            model/
            view/
         introduction
            model/
            view/
         my-repo-config.js
         my-repo-main.js

• Is there a file in assets/ for every file in audio/ and images/?
• Are all license.json files populated? audio/license.json and images/license.json should enumerate all files in those directories. For the format of license.json files, go here.
• Does the README.md format and content match PhET guidelines? Was it generated by grunt generate-published-README or grunt generate-unpublished-README?
• Does Gruntfile.js point to chipper/js/grunt/Gruntfile.js?
• Are dependencies in package.json the same as js/*-config.js? Do they match what is actually used by the sim? (ie, no missing dependencies, no unused dependencies)
• Is the LICENSE file correct? (Generally GPL v3 for sims, MIT for common code, but there are exceptions to this.)
• Does .gitignore match other repositories?
• Does *-main.js follow PhET conventions? Does it contain correct credits (options.credits passed to Sim constructor)?
• Is the format of assets/*-screenshot.png correct? (2048x1344, PNG, 32-bit color) Has the browser chrome been stripped off of the screenshot?
• Are there git repository branches that are no longer used and should be pruned?

Coding conventions

• Does the code style meet PhET's code-style guidelines, as described in the “Coding Style Guidelines” section of "PhET Development Overview"?
• Is the code formatted according to PhET conventions? See phet-idea-code-style.xml for IntelliJ IDEA code style.

Documentation

• Are documentation conventions followed, as described in the “Coding Style Guidelines” section of the PhET Development Overview?
• Does model.md adequately describe the model, in terms appropriate for teachers?
• Does implementation-notes.md adequately describe the implementation, with an overview that will be useful to future maintainers?
• Are sim-specific query parameters (if any) identified and documented in one location?

Common Errors

• Is Math.random used where dot.Util.randomSymmetric should be used? Math.random does not treat positive and negative numbers symmetrically, see fix nearest-neighbor rounding in Util.toFixed dot#35 (comment)
• Is toFixed used where dot.Util.toFixed or dot.Util.toFixedNumber should be used? JavaScript's toFixed is notoriously buggy, behavior differs depending on browser, because the spec doesn't specify whether to round or floor.

Organization, Readability, 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?
• Is inheritance used where appropriate? Does the type hierarchy make sense?
• Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)
• Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates?
• Are any significant chunks of code duplicated? This will be checked manually as well as with https://github.com/danielstjules/jsinspect
• Is there anything that should be generalized and migrated to common code?

Performance, Usability

• Does the sim perform as desired across the range of supported platforms? (eg, not too slow on slow platforms, not too fast on fast platforms)
• If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter 'webgl=false')
• 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')

Memory Leaks

• Does a heap comparison using Chrome Developer Tools indicate a memory leak? (Describing this process is beyond the scope of this document.)
• Are there any leaks due to registration of AXON observers?
  • For each call to Property.link or PropertySet.link, is there a corresponding unlink, or documentation about why an unlink is unnecessary?
  • For each DerivedProperty or Multilink created, is there a corresponding detach, or documentation about why a detach is unnecessary?
  • For each common-code component (sun, scenery-phet, vegas, …) that opaquely registers an AXON observer, is there a call to that component’s dispose function, or documentation about why dispose is unnecessary?
• Are there any leaks due to registration of components with TANDEM? tandem.addInstance should be accompanied by tandem.removeInstance or documented why removeInstance is unnecessary.

[jessegreenberg]

@dubson, I consider the code review to be complete. Overall the code looks very nice, great job on this sim!

Please let me know if you have any questions about issues opened during this code review.

[jessegreenberg]

The following are the last two issues related to code review:

• Sim does not handle i18n gracefully #16 (i18n, pending review)
• screenshot for deployment

All other issues have been resolved.

[jessegreenberg]

The screenshot for deployment is part of the master checklist and will be generated prior to doing the deploy. This can now be closed.