Code Review Checklist (5/17/2015 snapshot)

[ariel-phet]

NOTE! Prior to doing a code review, copy this checklist to a GitHub issue for the repository being reviewed.

PhET code-review checklist

Build and Run Checks

• [x] Does the sim build without warnings or errors?
• [x] Does the sim start up? (requirejs and built versions)
• [x] Does the sim experience any assertion failures? (run with query parameter 'ea')
• [x] Does the sim pass a scenery fuzzer test? (run with query parameter 'fuzzMouse&ea')

Internationalization

• [x] Are there any strings that are not being internationalized? (run with query parameter 'stringTest=x', you should see nothing but 'x' strings)
• [x] Does the sim layout gracefully handle internationalized strings that are twice as long as the English strings? (run with query parameter 'stringTest=double')
• [x] Does the sim layout gracefully handle internationalized strings that are exceptionally long? (run with query parameter 'stringTest=long')
• [x] Does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter 'stringTest=X')
• [x] Does the sim stay on the sim page (doesn't redirect to an external page) when running with the query parameter 'stringTest=xss' (test passes if sim does not redirect, OK if sim crashes or fails to fully start). Only test on one desktop platform.
• [x] Make sure the string keys are all perfect, they are difficult to change after 1.0.0 is published. Strings keys should generally match the values, such as {binaryProbability: "Binary Probability"}. Screen names should use screen.screenName instead of camelcase. Message patterns and long paragraphs will also use a different pattern.

Repository structure

• [x] 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/
      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.

• [x] 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 resource file in audio/ and images/? Note that there is not a 1:1 correspondence between asset and resource files; for example, several images may be in the same .ai file.
• [x] 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. Note that errors will automatically be detected by grunt build.
• [x] Does the README.md format and content match PhET guidelines? Was it generated by grunt published-README or grunt unpublished-README?
• [x] Does Gruntfile.js point to chipper/js/grunt/Gruntfile.js?
• [x] 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)
• [x] Is the LICENSE file correct? (Generally GPL v3 for sims, MIT for common code, but there are exceptions to this.)
• [x] Does .gitignore match other repositories?
• [x] Does *-main.js follow PhET conventions? Does it contain correct credits (options.credits passed to Sim constructor)?
• [x] Are there git repository branches that are no longer used and should be pruned?

Coding conventions

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

Documentation

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

Common Errors

• [x] Is Math.round used where dot.Util.roundSymmetric should be used? Math.round does not treat positive and negative numbers symmetrically, see https://github.com/phetsims/dot/issues/35#issuecomment-113587879
• [x] 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.
• [x] Are random numbers using phet.joist.random, and all doing so after modules are declared (non-statically)? For instance, the following methods (and perhaps others) should not be used:
• Math.random
• _.shuffle
• _.sample
• _.random
• new Random()

Organization, Readability, Maintainability

• [x] 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?
• [x] Are appropriate design patterns used?
• [x] Is inheritance used where appropriate? Does the type hierarchy make sense?
• [x] Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)
• [x] Is there too much unnecessary decoupling? (e.g. by passing all of the properties of an object independently instead of passing the object itself)?
• [x] Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates?
• [x] Are any significant chunks of code duplicated? This will be checked manually as well as with https://github.com/danielstjules/jsinspect
• [x] Is there anything that should be generalized and migrated to common code?
• [x] Are there any TODO or FIXME comments in the code? They should be addressed or promoted to GitHub issues.
• [x] 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 https://github.com/phetsims/plinko-probability/issues/84.
• [x] The simulation should use Property instead of PropertySet

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)
• [x] 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)
• [x] Are pointer areas optimized, especially for touch? (run with query parameter 'showPointerAreas')
• [x] Do pointer areas overlap? (run with query parameter 'showPointerAreas')
• [x] Is the timestep dt capped appropriately? Try switching applications or browser tabs, then switch back. Did the model take one big/long/awkward step forward? If so, dt may need to be capped. Example from faradays-law.FaradaysLawModel:

// Cap large dt values, which can occur when the tab containing
// the sim had been hidden and then re-shown
dt = Math.min( 0.1, dt );

Memory Leaks

• [x] Does a heap comparison using Chrome Developer Tools indicate a memory leak? (Describing this process is beyond the scope of this document.)
• [x] 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 documentation about why dispose is unnecessary?
• [x] Are there leaks due to registering observers or listeners? These guidelines should be followed, or documentation added about why following them is not necessary:
  • [x] AXON: Property.link is accompanied by Property.unlink.
  • [x] AXON: PropertySet.link is accompanied by PropertySet.unlink.
  • [x] AXON: Creation of DerivedProperty is accompanied by dispose.
  • [x] AXON: Creation of Multilink is accompanied by dispose.
  • [x] AXON: Events.on is accompanied by Events.off.
  • [x] AXON: Emitter.addListener is accompanied by Emitter.removeListener.
  • [x] SCENERY: Node.addEventListener is accompanied by Node.removeEventListener
  • [x] SCENERY: Node.on is accompanied by Node.off
  • [x] TANDEM: tandem.addInstance is accompanied by tandem.removeInstance.
• [x] Do all types that require a dispose function have one?

PhET-iO

• [x] Please see [How to Instrument a PhET Simulation for PhET-iO] (https://github.com/phetsims/phet-io/blob/master/doc/how-to-instrument-a-phet-simulation-for-phet-io.md) for the PhET-iO development process.

[samreid]

Hi @AshrafSharf, @jonathanolson and I are thankful for the work you have done on this sim and very excited to get it published. One of the next steps will be to prepare the code for code review. Can you please review the code review checklist above, address any of the items that you can and ask questions about anything that is unclear? Please let me and @jonathanolson know when you are ready to discuss further.

[AshrafSharf]

on stringTest=double. The screen looks like this. Shall I just make the font size a function of number of characters. Would that be enough?

[jonathanolson]

Added some review issues after a brief (very incomplete) code review. Will plan to discuss when working with @AshrafSharf (but feel free to fix issues beforehand).

(Will do a full code review at a later undermined point).

[jonathanolson]

Make sure the string keys are all perfect, they are difficult to change after 1.0.0 is published. Strings keys should generally match the values, such as {binaryProbability: "Binary Probability"}. Screen names should use screen.screenName instead of camelcase. Message patterns and long paragraphs will also use a different pattern.

@samreid, is this style of key allowable:

  "game.info.level1":{
    "value":"Add single digit numbers to get sums of 10 and under."
  },
  "screen.explore":{
    "value":"Explore"
  },
  "next":{
    "value":"Next"
  },

[samreid]

Here are some key-value pairs and my recommendations:

{
  "make-a-ten.title": {  // perfect
    "value": "Make a Ten"
  },
  "screen.adding":{ // looks great
    "value":"Adding"
  },
  "screen.explore":{ // looks great
    "value":"Explore"
  },
  "screen.game":{ // looks great
    "value":"Game"
  },
  "make-a-ten.hide.total":{ // Why is this prefixed with "make-a-ten"?  It should be hideTotal
    "value":"Hide Total"
  },
  "make-a-ten.keypad.submit":{ // Why is this prefixed with "make-a-ten"?  Just use "submit"
    "value":"Submit"
  },
  "next":{ // good
    "value":"Next"
  },
  "game.info.levelX":{ // See image below:
    "value":"Level {0}"
  },
  "game.info.level1":{ // How about level1Description?
    "value":"Add single digit numbers to get sums of 10 and under."
  },
  "game.info.level2":{ // see level 1
    "value":"Add 9 to a single digit number to get sums between 10 and 20."
  },
  "game.info.level3":{
    "value":"Add single digit numbers to get sums between 10 and 20."
  },
  "game.info.level4":{
    "value":"Add with multiples of 10."
  },
  "game.info.level5":{
    "value":"Add double digit numbers and single digit numbers."
  },
  "game.info.level6":{
    "value":"Add double digit numbers with sums under 100."
  },
  "game.info.level7":{
    "value":"Add double digit numbers with sums over 100."
  },
  "game.info.level8":{
    "value":"Add single digit numbers to triple digit numbers."
  },
  "game.info.level9":{
    "value":"Add hundreds and multiples of ten."
  },
  "game.info.level10":{
    "value":"Add triple digit numbers."
  }
}

For patterns, we inherited this from our Java sims, even though it is ugly at least it would be consistent:

[samreid]

I think Arithmetic has a level pattern like the one you need.

  "pattern.level.0levelNumber": {
    "value": "Level {0}"
  },

[jonathanolson]

Additionally, is phet-io instrumention required now, as it's on the checklist?

[samreid]

I committed a change (see above) that clarifies that the PhET-iO checklist is only to be used for simulations that are already instrumented or should be instrumented.

[jonathanolson]

Everything except the assets (https://github.com/phetsims/make-a-ten/issues/250), performance (https://github.com/phetsims/make-a-ten/issues/251) and credits (https://github.com/phetsims/make-a-ten/issues/249) has been handled here.

Closing, as those are already in stand-alone issues.