code review #2

pixelzoom · 2020-02-07T22:15:16Z

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:

results of memory testing for brands=phet - see memory testing for brands=phet #32
N/A results of memory testing for brands=phet-io (if the sim is instrumented for PhET-iO)
performance testing and sign-off - see performance testing #33
review of pointer areas - see review pointer areas #31
credits (will not be completed until after RC testing) - see credits #19

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? (requirejs and built versions)
❌ see sim fails with assertions enabled #23 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)
❌ see sim fails with shuffleListeners #50 Does the sim behave correctly when listener order is shuffled? (run with query parameters ea&shuffleListeners and ea&shuffleListeners&fuzz)
N/A 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

Performance

N/A see performance testing #33 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.
N/A If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter webgl=false)

Usability

N/A see performance testing #33Are UI components sufficiently responsive? (especially continuous UI components, such as sliders)
N/A, see review pointer areas #31 Are pointer areas optimized, especially for touch? (run with query parameter showPointerAreas)
N/A, see review pointer areas #31 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

❌ see sim fails stringTest #10 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.)
❌ see sim fails stringTest #10 Does the sim layout gracefully handle internationalized strings that are twice as long as the English strings? (run with query parameter stringTest=double)
❌ see sim fails stringTest #10 Does the sim layout gracefully handle internationalized strings that are exceptionally long? (run with query parameter 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
❌ see use string pattern for omega values #12 Use named placeholders (e.g. "{{value}} {{units}}") instead of numbered placeholders (e.g. "{0} {1}").
❌ see string keys need to be adjusted #11 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.
N/A 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

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.

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.

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.

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).
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.
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(...);
```

N/A 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(...);

❌ see pattern for defining Enumerations #34 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();
   }
  }

N/A 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' ) };
```
N/A 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.
N/A 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:

Math Libraries

❌ see replace toFixed #18 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

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

Organization, Readability, and Maintainability

Accessibility

N/A

PhET-iO

N/A

The text was updated successfully, but these errors were encountered:

pixelzoom · 2020-02-10T23:37:18Z

Code review is done.

The sim is generally in nice shape, especially considering that it was done by someone outside of PhET. So great work!

Most of the deficiencies (e.g. lint, assertions) can be traced to deficiencies in PHET's documentation for 3rd-party developers. @ariel-phet we might consider triaging these GitHub issues to pull out the things that need to be improved in PhET doc.

There are 43 GitHub issues associated with this issue. They are all currently unassigned. A handful are marked "high priority", and these are ones that I recommend as essential to address. The issues marked as "ready-for-review" are ones that I fixed in order to make progress on the code review, and I left them open because that have "teachable points" or "lessons learned". I recommend that the original developers have a look at these, ask questions, and then close.

I should also note that this review focused solely on PhET development standards. I'm not familiar with the domain or the original Flash sim, so I did not verify the correctness of the sim behavior, or even compare it to the Flash version.

Assigning to @ariel-phet for next steps.

ariel-phet · 2020-02-11T17:48:19Z

@tmildemberger @Hyodar please see the comment by @pixelzoom above. The high priority issues would definitely be the ones to address first, and then the ready-for-review issues as next in priority to look over.

As you address issues, please make comments in the associated issue. If you have any questions on issues, please assign them back to @pixelzoom for clarification.

pixelzoom · 2020-09-17T21:54:23Z

This issue can be closed, since specific things are being tracked in their own GitHub issues.

pixelzoom self-assigned this Feb 7, 2020

pixelzoom added the dev:code-review label Feb 7, 2020

pixelzoom assigned ariel-phet and unassigned pixelzoom Feb 10, 2020

pixelzoom added the status:ready-for-review label Feb 10, 2020

ariel-phet assigned Hyodar and tmildemberger Feb 11, 2020

ariel-phet assigned ariel-phet and unassigned ariel-phet, Hyodar and tmildemberger Feb 11, 2020

pixelzoom mentioned this issue Feb 11, 2020

fill in gaps in public-facing developer documentation phetsims/tasks#1022

Closed

ariel-phet assigned Hyodar and tmildemberger and unassigned ariel-phet Feb 12, 2020

pixelzoom closed this as completed Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code review #2

code review #2

pixelzoom commented Feb 7, 2020 •

edited by Hyodar

Loading

pixelzoom commented Feb 10, 2020 •

edited

Loading

ariel-phet commented Feb 11, 2020 •

edited

Loading

pixelzoom commented Sep 17, 2020

code review #2

code review #2

Comments

pixelzoom commented Feb 7, 2020 • edited by Hyodar Loading

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

GitHub Issues

Build and Run Checks

Memory Leaks

Performance

Usability

Internationalization

Repository Structure

Coding Conventions

Documentation

Type Expressions

Visibility Annotations

Math Libraries

IE11

Organization, Readability, and Maintainability

Accessibility

PhET-iO

pixelzoom commented Feb 10, 2020 • edited Loading

ariel-phet commented Feb 11, 2020 • edited Loading

pixelzoom commented Sep 17, 2020

pixelzoom commented Feb 7, 2020 •

edited by Hyodar

Loading

pixelzoom commented Feb 10, 2020 •

edited

Loading

ariel-phet commented Feb 11, 2020 •

edited

Loading