Code Review

[jessegreenberg]

@ariel-phet @samreid Energy Skate Park is ready for a code review. To assist in review, I wanted to list out the new features and changes which resulted in the largest changes to code. Other than these, the code remained mostly the same from energy-skate-park-basics.

New Features and Important Changes

• Moving energy-skate-park-basics code to energy-skate-park (#6 )
• Model and view types use more inheritance. Rather than adding things to the model and view with options/presence of fields, there are subtypes of EnergySkateParkModel that include various feature sets. For example,
  • EnergySkateParkModel
    • EnergySkateParkTrackSetModel
    • EnergySkateParkPlaygroundModel
  • Addition of gravityProperty to Skater and various calculations
  • Addition of referenceHeightProperty to Skater and various calculations
  • Addition of EnergySkateParkSaveSampleModel to support saving skater states for playback and plotting energy data over time (MeasureScreen, GraphsScreeen)
  • Changes to EnergySkateParkControlPanel to support new controls and the various layout of these controls on each screen.

New files and changes to model inheritance are also described in implementation-notes.md, so I recommend starting with implementation-notes.md and model.md as well.

Next comment will include checklist and references to pre-review issues. Assigning to @ariel-phet as well to comment on review priority. I appreciate it is an especially busy time with ES6 migration!

[jessegreenberg]

PhET Code-Review Checklist (a.k.a "CRC")

• Mark failed items with ❌
• Call attention to items with ⚠️
• Mark items that are not applicable with "N/A"

GitHub Issues

The following standard GitHub issues should exist. If any of these issues is missing, or have not been completed, pause code review until the issues have been created and addressed by the responsible dev.

GitHub issues should exist that document:

• [x] results of memory testing for brands=phet, see (https://github.com/phetsims/energy-skate-park/issues/200)
• [ ] results of memory testing for brands=phet-io (if the sim is instrumented for PhET-iO) (I don't think this is necessary for this publication)
• [x] performance testing and sign-off (Checked in https://github.com/phetsims/QA/issues/470, see also https://github.com/phetsims/energy-skate-park/issues/198)
• [x] review of pointer areas (Checked in https://github.com/phetsims/QA/issues/470)
• [x] credits (will not be completed until after RC testing) (See https://github.com/phetsims/energy-skate-park/issues/201)

Build and Run Checks

If any of these items fail, pause code review.

• [x] Does the sim build without warnings or errors?
• [x] Does the html file size seem reasonable, compared to other similar sims?
• [x] Does the sim start up? (requirejs and built versions)
• [x] 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)
• [x] Does the sim behave correctly when listener order is shuffled? (run with query parameters ea&shuffleListeners and ea&shuffleListeners&fuzz)
• [ ] Does the sim use Map? If so, make sure that it still works well in IE11 as not all Map functions are supported there.

Memory Leaks

• [ ] Does a heap comparison using Chrome Developer Tools indicate a memory leak? (Describing this process is beyond the scope of this document.) Test on a version built using grunt --minify.mangle=false. Compare to testing results done by the responsible developer.
• [ ] 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.
• [ ] Are there leaks due to registering observers or listeners? The following guidelines should be followed unless there it is obviously no need to unlink, or documentation (in-line or in the implementation nodes)added about why following them is not necessary. Unlink is not needed for Properties contained in classes that are never disposed of, such as primary model and view classes that exist for the duration of the sim.
  • [ ] AXON: Property.link is accompanied by Property.unlink.
  • [ ] AXON: Creation of DerivedProperty is accompanied by dispose.
  • [ ] AXON: Creation of Multilink is accompanied by dispose.
  • [ ] AXON: Events.on is accompanied by Events.off.
  • [ ] AXON: Emitter.addListener is accompanied by Emitter.removeListener.
  • [ ] SCENERY: Node.on is accompanied by Node.off
  • [ ] TANDEM: PhET-iO instrumented PhetioObject instances should be disposed.
• [x] Do all types that require a dispose function have one? This should expose a public dispose function that calls this.disposeMyType(), where disposeMyType is a private function declared in the constructor. MyType should exactly match the filename.

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

• [ ] 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.)
• [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 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
• [x] Use named placeholders (e.g. "{{value}} {{units}}") instead of numbered placeholders (e.g. "{0} {1}").

• [ ] Make sure the string keys are all perfect, because they are difficult to change after 1.0.0 is published. 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.

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

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/, sound/ or images/ may be omitted if the sim doesn’t have those types of assets).

   my-repo/
      assets/
         license.json
      doc/
         images/
               *see annotation
         model.md
         implementation-notes.md
      images/
         license.json
      js/
         (see section below)
      sound/
      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.

• [ ] 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-config.js
         my-repo-main.js
         myRepo.js

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

• [ ] 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 reesources might not have a corresponding asset file.

• [x] Was the README.md generated by grunt published-README or grunt unpublished-README?

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

• [x] Is the sim's -config.js up-to-date (generated by grunt generate-config)

• [ ] Is the LICENSE file correct? (Generally GPL v3 for sims and MIT for common code, see this thread for additional information).

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

• [ ] 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 .js file in js/common/ or js/ (if there is no common/)? The .js file should be named {{REPO}}QueryParameters.js, for example ArithmeticQueryParameters.js for the aritmetic repository.

Coding Conventions

This section 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.

• [x] Is the code formatted according to PhET conventions? See phet-idea-code-style.xml for IntelliJ IDEA code style.

• [x] Names (types, variables, properties, Properties, functions,...) should be sufficiently descriptive and specific, and should avoid non-standard abbreviations. For example:

  const numPart = 100;            // incorrect
  const numberOfParticles = 100;  // correct
  
  const width = 150;              // incorrect
  const beakerWidth = 150;        // correct

• [ ] Require statements should be organized into blocks, with the code modules first, followed by plugins (strings, images, sound, ifphetio - any order ok for plugins). For modules, the variable name should match the file name. Example below.

  // modules
  const inherit = require( 'PHET_CORE/inherit' );
  const Line = require( 'SCENERY/nodes/Line' );
  const Rectangle = require( 'SCENERY/nodes/Rectangle' );
  
  // strings
  const kineticString = require( 'string!ENERGY/energy.kinetic' );
  const potentialString = require( 'string!ENERGY/energy.potential' );
  const thermalString = require( 'string!ENERGY/energy.thermal' );
  
  // images
  const energyImage = require( 'image!ENERGY/energy.png' );
  
  // sound
  const kineticAudio = require( 'sound!ENERGY/energy' );

• [x] For constructors, use parameters for things that don’t have a default. Use options for things that have a default value. This improves readability at the call site, especially when the number of parameters is large. It also eliminates order dependency that is required by using parameters.

  For example, this constructor uses parameters for everything. At the call site, the semantics of the arguments are difficult to determine without consulting the constructor.

  class BallNode extends Node {
  
  /**
   * @param {Ball} ball - model element
   * @param {Property.<boolean>} visibleProperty - is the ball visible?
   * @param {Color|string} fill - fill color
   * @param {Color|string} stroke - stroke color
   * @param {number} lineWidth - width of the stroke
   */
  constructor( ball, visibleProperty, fill, stroke, lineWidth ){
    // ...
  }
  }
  
  // Call site
  const ballNode = new BallNode( ball, visibleProperty, 'blue', 'black', 2 );

  Here’s the same constructor with an appropriate use of options. The call site is easier to read, and the order of options is flexible.

  class BallNode extends Node {
  
  /**
   * @param {Ball} ball - model element
   * @param {Property.<boolean>} visibleProperty - is the ball visible?
   * @param {Object} [options]
   */
  constructor( ball, visibleProperty, options ) {
  
    options = merge( {
      fill: 'white',  // {Color|string} fill color
      stroke: 'black', // {Color|string} stroke color
      lineWidth: 1 // {number} width of the stroke
    }, options );
  
    // ...
  }
  }
  
  // Call site
  const ballNode = new BallNode( ball, visibleProperty, {
  fill: 'blue',
  stroke: 'black',
  lineWidth: 2
  } );

• [x] When options are passed through one constructor to another, a "nested options" pattern should be used. This helps to avoid duplicating option names and/or accidentally overwriting options for different components that use the same option names. Make sure to use PHETCORE/merge instead of `.extendor_.merge.mergewill automatically recurse to keys named*Options` and extend those as well.

  Example:

  class ParticleBoxNode extends Node {
  
  /**
   * @param {ParticleBox} particleBox - model element
   * @param {Property.<boolean>} visibleProperty - are the box and its contents visible?
   * @param {Object} [options]
   */
  constructor( particleBox, visibleProperty, options ) {
  
    options = merge( {
      fill: 'white',  // {Color|string} fill color
      stroke: 'black', // {Color|string} stroke color
      lineWidth: 1, // {number} width of the stroke
      particleNodeOptions: {
        fill: 'red',
        stroke: 'gray',
        lineWidth: 0.5
      },
    }, options );
  
    // add particle
    this.addChild( new ParticleNode( particleBox.particle, options.particleNodeOptions ) );
    ...
  }
  }

  A possible exception to this guideline is when the constructor API is improved by hiding the implementation details, i.e. not revealing that a sub-component exists. In that case, it may make sense to use new top-level options. This is left to developer and reviewer discretion.

  For more information on the history and thought process around the "nested options" pattern, please see https://github.com/phetsims/tasks/issues/730.

• [ ] If references are needed to the enclosing object, such as for a closure, self should be defined, but it should only be used in closures. The self variable should not be defined unless it is needed in a closure. Example:

  const self = this;
  someProperty.link( function(){
  self.doSomething();
  } );
  this.doSomethingElse();

• [x] Generally, lines should not exceed 120 columns. Break up long statements, expressions, or comments into multiple lines to optimize readability. It is OK for require statements or other structured patterns to exceed 120 columns. Use your judgment!

• [x] Use class and extends for defining classes and implementing inheritance. PHET_CORE/inherit was a pre-ES6 implementation of inheritance that is specific to PhET and has been supplanted by class and extends. inherit should not be used in new code.

• [x] Functions should be invoked using the dot operator rather than the bracket operator. For more details, please see https://github.com/phetsims/gravity-and-orbits/issues/9. For example:

  // avoid
  this[ isFaceSmile ? 'smile' : 'frown' ]();
  
  // OK
  isFaceSmile ? this.smile() : this.frown();
  
  // OK
  if ( isFaceSmile ) {
  this.smile();
  }
  else {
  this.frown();
  }

• [x] It is not uncommon to use conditional shorthand and short circuiting for invocation. Use parentheses to maximize readability.

  ( expression ) && statement;
  ( expression ) ? statement1 : statement2;
  ( foo && bar ) ? fooBar() : fooCat();
  ( foo && bar ) && fooBar();
  ( foo && !(bar && fooBar)) && nowIAmConfused();
  this.fill = ( foo && bar ) ? 'red' : 'blue';

  If the expression is only one item, the parentheses can be omitted. This is the most common use case.

  assert && assert( happy, 'Why aren\'t you happy?' );
  happy && smile();
  const thoughts = happy ? 'I am happy' : 'I am not happy :(';

• [x] Naming for Property values: All AXON/Property instances should be declared with the suffix Property. For example, if a visible property is added, it should have the name visibleProperty instead of simply visible. This will help to avoid confusion with non-Property definitions.

• [x] Properties should use type-specific subclasses where appropriate (.e.g BooleanProperty, NumberProperty, StringProperty) or provide documentation as to why they are not.

• [ ] Are Validator validation options (valueType, validValues, etc...) utilized? These are supported in a number of core types like Emitter and Property. Is their presence or lack thereof properly documented?

• [x] Files should be named like CapitalizedCamelCasing.js when returning a constructor, or lowerCaseCamelCasing.js when returning a non-constructor function or singleton. When returning a constructor or singleton, the constructor name should match the filename.

• [x] Assertions should be used appropriately and consistently. Type checking should not just be done in code comments. Use Array.isArray to type check an array.

• [x] If you need to namespace an inner class, use {{namespace}}.register, and include a comment about why the inner class needs to be namespaced. Use the form '{{outerClassname}}.{{innerClassname}}' for the key. For example:

  const myNamespace = require(...);
  
  class SlotMachineNode extends Node {
  constructor( ... ) {
    this.leverNode = new LeverNode(...);
    ...
  }
  ...
  }
  
  myNamespace.register( 'SlotMachineNode', SlotMachineNode );
  
  class LeverNode extends Node { 
  ...
  }
  
  // It was useful to be able to instantiate this in the console for testing, and we may need to do so in the future.
  myNamespace.register( 'SlotMachineNode.LeverNode', LeverNode );
  
  return SlotMachineNode;

• [x] Putting unused parameters in callbacks is up to developer discretion, as long they are correct wrt to the actual callback signature.

For example, both of these are acceptable:

Property.multilink(
  [ styleProperty, activeProperty, colorProperty ],
  ( style, active, color ) => {
    // some algorithm that uses style and active 
} );

Property.multilink(
  [ styleProperty, activeProperty, colorProperty ],
  ( style, active ) => {
    // some algorithm that uses style and active 
} );

This is not acceptable, because the 3rd parameter is incorrect.

Property.multilink(
  [ styleProperty, activeProperty, colorProperty ],
  ( style, active, lineWidth ) => {
    // some algorithm that uses style and active 
} );

Documentation

This section deals with PhET documention 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.

• [ ] All classes, methods and properties are documented.

• [ ] Documentation at the top of .js files should provide an overview of purpose, responsibilies, and (where useful) examples of API use. If the file contains a subclass definition, it should indicate what functionality it adds to the superclass.

• [ ] The HTML5/CSS3/JavaScript source code must be reasonably well documented. This is difficult to specify precisely, but the idea is that someone who is moderately experienced with HTML5/CSS3/JavaScript can quickly understand the general function of the source code as well as the overall flow of the code by reading through the comments. For an example of the type of documentation that is required, please see the example-sim repository.

• [x] Differentiate between Property and "property" in comments. They are different things. Property is a type in AXON; property is any value associated with a JavaScript object. Often "field" can be used in exchange for "property" which can help with clarity.

• [x] Line comments should generally be preceded by a blank line. For example:

  // Randomly choose an existing crystal to possibly bond to
  const crystal = this.crystals.get( _.random( this.crystals.length - 1 ) );
  
  // Find a good configuration to have the particles move toward
  const targetConfiguration = this.getTargetConfiguration( crystal );

• [x] When documenting conditionals (if/else statements), follow these guidlines:

  1. Comments above the first if in a conditional should be about the entire conditional, not just the if block.
  2. Comments should not break up sections of the conditional.
  3. If a comment is needed to describe a single block of the conditional, then add that comment just inside the block (no space between the if/else if/else and the comment), with a space below it as to not be confused with a comment about logic below.

  
  // Comment about the reason to split on peppers were pickled.
  if( peterPiperPickedAJarOfPickledPeppers ){
    // if we want to explain what this `if` statement is about
  
    peterAlsoHasBrine();
  }
  else {
  
    // documentation about why we have no peppers. This is about the next line of code, and not the "else as a whole block."
    peterHasNoPeppers();
  }

• [x] Line comments should have whitespace between the // and the first letter of the line comment. See the preceding example.

• [ ] Do the @author annotations seem correct?

• [ ] ES5 (inherit) constructors should be annotated with @constructor. ES6 (class) constructors should not be annotated with @constructor.

• [ ] Constructor and function documentation. Parameter types and names should be clearly specified for each constructor and function using @param annotations. The description for each parameter should follow a hyphen. Primitive types should use lower case. For example:

  /**
  * The PhetDeveloper is responsible for creating code for simulations and documenting their code thoroughly.
  */
  class PhetDeveloper {
  
  /**
   * @param {string} name - full name
   * @param {number} age - age, in years
   * @param {boolean} isEmployee - whether this developer is an employee of CU
   * @param {function} callback - called immediate after coffee is consumed
   * @param {Property.<number>} hoursProperty - cumulative hours worked
   * @param {string[]} friendNames - names of friends
   * @param {Object} [options]  
   */
  constructor( name, age, isEmployee, callback, hoursProperty, friendNames, options ) {
    ...
  }
  
  ...
  }

• [x] For most functions, the same form as above should be used, with a @returns annotation which identifies the return type and the meaning of the returned value. Functions should also document any side effects. For extremely simple functions that are just a few lines of simple code, an abbreviated line-comment can be used, for example: // Computes {Number} distance based on {Foo} foo.

• [x] Abstract methods (normally implemented with an error) should be marked with @abstract jsdoc.

Type Expressions

This section deals with PhET conventions for type expressions. 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.

• [x] Type expressions should conform approximately to Google Closure Compiler syntax. PhET stretches the syntax in many cases (beyond the scope of this document to describe).

• [x] Prefer the most basic/restrictive type expression when defining APIs. For example, if a client only needs to know that a parameter is {Node}, don’t describe the parameter as {Rectangle}.

• [ ] All method parameters should have type expressions. For example @param {number} width.

• [x] In sim-specific code, options and fields should have type expressions when their type is not obvious from the context. “Obvious” typically means that the value type is clearly shown in the righthand-side of the definition. E.g. const width = 42 clear shows that width is {number}. E.g. const checkbox = new Checkbox(…) clearly shows that checkbox is {Checkbox}. If the type is obvious from the context, the developer may still provide a type expression at his/her discretion. Examples:

  // @public {GameState} the current state of the game
  this.gameState = this.computeGameState();
  
  // @public (read-only) the width of the container
  this.containerWidth = 150;
  
  // @private the checkbox used to show particles
  this.particlesVisibleCheckbox = new Checkbox(...);

• [x] In common code repositories all options and fields should have type expressions, regardless of their visibility, and regardless whether their type is obvious from the context. If the same examples from above appeared in common code:

  // @public {GameState} the current state of the game
  this.gameState = this.computeGameState();
  
  // @public (read-only) {number} the width of the container
  this.containerWidth = 150;
  
  // @private {Checkbox} the checkbox used to show particles
  this.particlesVisibleCheckbox = new Checkbox(...);

• [x] Type expressions for Enumeration values should be annotated as instances of that Enumeration, see examples in https://github.com/phetsims/phet-core/blob/master/js/Enumeration.js for more.

  /**
  * @param {LeftOrRight} - whichHand
  */
  function getHand( whichHand ){
   if( whichHand === LeftOrRight.LEFT ){
     return new LeftHand();
   }
   else if( whichHand === LeftOrRight.RIGHT ){
     return new RightHand();
   }
  }

• [ ] Type expressions for functions have a variety of possibilities, increasing in complexity depending on the case. In general note that {function} is not enough information. Here are some better options:

  1. The most basic option it to use Google Closure Type syntax, for more info see https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System. This specifies the param/return types, but nothing more. Here are some examples:
     • @param {function()} noParamsAndNoReturnValue
     • @param {function(number)} giveMeNumberAndReturnNothing
     • @param {function(number, number):Vector2} getVector2
     • @param {function(new:Node)} createNode - a function that takes the Node constructor
  2. When needing to be a bit more specific, add a name to parameters of the function. Sometimes this is all that is needed for clarity on what the param does:
     • @param {function(model:MyModel, length:number, name:string): Node} getLengthNode
     • @param {function(aSelfExplanatoryNameForAString:string): Node} getStringNode
  3. If (2) isn't enough, use English to explain the parameters and return values. This is easy because they are named, and can be easily mentioned:
     • @param {function(model:MyModel, length:number, name:string): Node} getLengthNode - returns the length Node that you have always wanted, name is the name of the source of your aspirations, length is a special number according to the following 24 criteria. . .
  4. If needing more complexity, or using jsdoc rendering tools (like PhET-iO documentation does), you must use a JSDoc compatible format, not (2) or (3), and you may need to use the more complicated solution. See JSDoc docs for more info. Here is an example of a named callback:

     /**
      * @name mySpecialCallback
      * Converts a string to a number
      * @param {string}
      * @returns {number}
      */
     /**
      * @param {mySpecialCallback} callback  
     */
     x = function( callback) { callback( 'still chocolate' ) };

• [x] Type expressions for anonymous Objects have a variety of possibilities, increasing in complexity depending on the case.

  1. When the documentation is close by, then {Object} is still acceptable. This mainly applies to options and similar patterns:
     • @param {Object} [options] // this is great because of the extend call 5 lines down
  2. When using an Object with specific properties, name them and their types like so:
     • @param {name:string, address:{street:string}, returnNode:function(number):Node, [shoeSize:number]} personalData // note that shoeSize is optional here
  3. When you need a bit more explanation, keep the same type expression as (2), but feel free to outline specifics in English after the param name.

     @ param {name:string, address:{street:string}, returnNode:function(number):Node, [shoeSize:number]} personalData - use english after to explain pieces of this
         (if needed, outline properties on their own lines)
         name is something
         address is something else
         returnNode does this thing

  4. Not all objects have named keys like (2) and (3). Here is how to document dictionary-like Objects, where each key is some type, and the value is another type. For key value pairs use this:
     • {Object.<string, number>} Where keys are strings, and values are numbers.
     • {Object.<phetioID:string, count:number>} - naming each of these can help identify them too. Feel free to explain in English after the type expression if needed.
  5. If things are too complicated for the above cases, use a *Def.js file (especially is used in more than one file), or a @typedef declaration right above the jsdoc that uses the typedef.

• [x] Look for cases where the use of type expressions involving Property subclasses are incorrect. Because of the structure of the Property class hierarchy, specifying type-specific Properties ({BooleanProperty}, {NumberProperty},...) may be incorrect, because it precludes values of type {DerivedProperty} and {DynamicProperty}. Similarly, use of {DerivedProperty} and {DynamicProperty} precludes values of (e.g.) {BooleanProperty}. Especially in common code, using {Property,<TYPE>} is typically correct, unless some specific feature of the Property subclass is required. For example, {Property.<boolean>} instead of {BooleanProperty}.

Visibility Annotations

This section deals with PhET conventions for visibility annotations. 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.

Because JavaScript lacks visibility modifiers (public, protected, private), PhET uses JSdoc visibility annotations to document the intent of the programmer, and define the public API. Visibility annotations are required for anything that JavaScript makes public. Information about these annotations can be found here. (Note that other documentation systems like the Google Closure Compiler use slightly different syntax in some cases. Where there are differences, JSDoc is authoritative. For example, use Array.<Object> or Object[] instead of Array<Object>). PhET guidelines for visibility annotations are as follows:

• [ ] Use @public for anything that is intended to be part of the public API.

• [ ] Use @protected for anything that is intended for use by subtypes.

• [ ] Use @private for anything that is NOT intended to be part of the public or protected API.

• [ ] Put qualifiers in parenthesis after the annotation, for example:

  • [ ] To qualify that something is read-only, use @public (read-only). This indicates that the given Property (AND its value) should not be changed by outside code (e.g. a Property should not have its value changed)
  • [ ] To qualify that something is public to a specific repository, use (for example) @public (scenery-internal)
  • [ ] For something made public solely for a11y, use @public (a11y)
  • [ ] For something made public solely for phet-io, use @public (phet-io)
  • [ ] Separate multiple qualifiers with commas. For example: @public (scenery-internal, read-only)

• [ ] For JSDoc-style comments, the annotation should appear in context like this:

  /**
  * Creates the icon for the "Energy" screen, a cartoonish bar graph.
  * @returns {Node}
  * @public
  */

• [ ] For Line comments, the annotation can appear like this:

  // @public {function(listener:function)} - Adds a listener
  addListener: function( listener ) { /*...*/ }

• [ ] Verify that every JavaScript property and function has a visibility annotation. Here are some helpful regular expressions to search for these declarations as PhET uses them.

  • Regex for property assignment like x.y = something: [\w]+\.[\w]+\s=
  • Regex for function declarations: [\w]+: function\(

Math Libraries

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

• [x] No usage of Number.parseInt()
• [x] No usage of Array.prototype.find
• [x] No usage of String.endsWith(), please use _.endsWith() instead.

Organization, Readability, and 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? See phet-software-design-patterns.md. If new or inappropriate patterns are identified, create an issue.
• [x] 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)
• [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 or grunt find-duplicates
• [ ] 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.
• [ ] 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 https://github.com/phetsims/plinko-probability/issues/84.
• [x] 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.
• [x] Are all dependent Properties modeled as DerivedProperty instead of Property?
• [x] 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 for a11y.

• [ ] Does the sim pass an accessibility fuzz test? (run with query parameters fuzzBoard&ea)
• [ ] Run the accessible HTML through an HTML validator, does the HTML pass?
• [ ] Are accessibility features integrated well into the code. They should be added in a maintainable way, even if that requires upfront refactoring.
• [ ] Are accessible design patterns used, see accessible-design-patterns.md
• [ ] Does resetting the simulation also reset the entire PDOM?
• [ ] Is Node.accessibleOrder used appropriately to maintain visual and PDOM layout balance?
• [ ] 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.

• [ ] Does instrumentation follow the conventions described in How to Instrument a PhET Simulation for PhET-iO?
• [ ] 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 the "console" and "studio" wrappers (easily accessed from phetmarks). Compare to testing results done by the responsible developer.

[samreid]

I pushed a reference version before the review: https://phet-dev.colorado.edu/html/energy-skate-park/1.0.0-dev.10/phet/energy-skate-park_en_phet.html

[samreid]

Does the sim use Map?

Yes, but its usages look safe. I'll leave it to QA team to test built version on IE.

Does the sim behave correctly when listener order is shuffled?

Difficult to test due to another assertion failure.

Does a heap comparison using Chrome Developer Tools indicate a memory leak?

I don't see a memory leak, but the footprint is relatively large, see #208

The following guidelines should be followed unless there it is obviously no need to unlink, or documentation (in-line or in the implementation nodes)

There are many Property.link without unlink, but I don't think this sim needs the unlinks because most of these cases are static. How should that be documented?

[samreid]

Review complete. Things are generally looking good, nice work! Anything unchecked in the checklist above has one of the following:

• A comment in this issue
• A // REVIEW comment in the code, and a commit that references this issue
• An issue that references this issue.

In summary, I added 142 // REVIEW comments (note some of these are in the "basics" repo), and opened 28 issues. I also made numerous commits fixing minor problems, typos, formatting, JSDoc, etc. which should be reviewed. Please let me know if you have questions or comments about any parts of the review.

[jessegreenberg]

I had forgotten about this issue, but code review is complete. REVIEW comments were removed and we had some back and forth in dedicated issues sourced from this one, all of which have been closed.