Code Review #286

zepumph · 2020-12-12T00:18:41Z

Expanding from #210, and also replacing #208. Code review is ready for this sim. Over to @jbphet. Some notes:

This is a bit earlier than I would normally do a code review, but with the holidays coming up, I think it is the best time for it right now.
Note that there are many bug reports still open in the sim. Note that there are multiple code-review issues open in the sim. I still think it is worth having a code-review even with these outstanding issues.
Please ignore TODOs that are linked to open issues, or to Will tangible input be included in the published simulation? #89 as tangible input will be worked on in the next release of this sim.
This sim is currently in dev testing, so likely more issue will come up as you review.
@jbphet, when we spoke last, you talked about perhaps early in the week of 12/14 would be when you would start this. I would love to discuss that workflow with you, because I could have time to devote to RaP during those days, but don't want to step on your toes.

Thanks! Let me know how I can support you.

NOTE! Prior to doing a code review, copy this checklist to a GitHub issue for the repository being reviewed. Delete the Table of Contents section, since the links will be incorrect.

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.

results of memory testing for brands=phet see Memory test: phet brand #269
performance testing and sign-off see Performance testing and sign off #270
review of pointer areas see review pointer areas #271
credits (will not be completed until after RC testing) see QA Credits #285

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)
Does the sim behave correctly when listener order is shuffled? (run with query parameters ea&shuffleListeners and ea&shuffleListeners&fuzz) ❌ see "Lock Ratio" unlocks itself with shuffleListeners on #300 and Ratios seem to be a little off with "shuffleListeners" on #301
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? (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. ❌ From @jbphet - There is a small leak of about 2MB over 5 min. Looking through the additional elements I saw a number of BooleanProperty instances for buttons and sound clips. I'd recommend double checking that dispose is being called everywhere that it should be.
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 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: Property.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.
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.
If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter webgl=false) (N/A)

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) (From @jbphet: I've logged a couple of minor issues on this, see Extend pointer area for "Lock Ratio" checkbox #302 and Icons appear to be individually pickable #303).
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.)
Does the sim layout gracefully handle internationalized strings that are longer than the English strings? (run with query parameters stringTest=double and stringTest=long) ❌ see stringTest=long looks bad #273
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.
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: ❌ see Use screen.<name> for screen name keys in strings file #304 for an issue about string keys

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

ES5 getters/setters defined in sims should be used judiciously when a Property exists, only when the benefit of conciseness outweighs the potential loss of readability. If ES5 getters/setters exist, try to not mix usage of them with Property access.

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

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

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

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

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, but is likely good to glance at no matter.

Does instrumentation follow the conventions described in PhET-iO Instrumentation Guide?
This could be an extensive bullet. At the very least, be sure to know what amount of instrumentation this sim
supports. Describing this further goes beyond the scope of this document.
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 a colorized Data Stream, and Studio (easily
accessed from phetmarks). Compare to testing results done by the responsible developer and previous releases.
Make sure unused PhetioObject instances are disposed, which unregisters their tandems.
Make sure JOIST dt values are used instead of Date.now() or other Date functions. Perhaps try
phet.joist.elapsedTime. Though this has already been mentioned, it is necessary for reproducible playback via input
events and deserves a comment in this PhET-iO section.
Are random numbers using phet.joist.random, and all doing so after modules are declared (non-statically)? For
example, the following methods (and perhaps others) should not be used: Math.random, _.shuffle, _.sample, _.random.
This also deserves re-iteration due to its effect on record/playback for PhET-iO.
Like JSON, keys for undefined values are omitted when serializing objects across frames. Consider this when
determining whether toStateObject should use null or undefined values.
PhET prefers to use the term "position" to refer to the physical (x,y) position of objects. This applies to both
brands, but is more important for the PhET-iO API. See location vs position phet-info#126
Are your IOType state methods violating the API of the core type by accessing private fields?
When defining a boolean Property to indicate whether something is enabled, use AXON/EnabledProperty. This
should be done in both the model and the view.

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

zepumph · 2020-12-15T00:12:29Z

See #269 for previous (recent) memory test info.

jbphet · 2020-12-23T00:25:51Z

Code review complete, please search through the code for // REVIEW tags. I also did some fixing of formatting, comments, typos and such that were unlikely to be controversial. Overall, the structure looks good, conventions have been followed, and the code is readable.

I have one thought on the model that I'll set up a time with @zepumph to discuss. I think it might be too big of a change to be considered at this point, but it's worth a discussion.

Other than that, once the // REVIEW comments have been addressed it should be good to go for the next steps.

BTW, I stopped checking all the boxes above because it was taking forever, but I think I hit everything. Man, that checklist has gotten long.

zepumph · 2020-12-28T21:07:03Z

You said:

      // REVIEW: This looks weird in terms of indentation.  Why not use \n or <br> and the + operator to keep it neater?
      phet.log && phet.log( `
left: ${antecedent}, 
right: ${consequent}, 
distance: ${Math.abs( consequent - antecedent )}, 
current ratio: ${this.ratio.currentRatio}, 
target ratio: ${this.targetRatioProperty.value},
unclampedFitness: ${unclampedFitness}
`
      );

I actually prefer this, as I think it is easier to read what the actual string will look like, and templated vars allow for the ${} shortcut to more easily fill in values. I moved the closing paren to a spot where I think it does a better job in framing the weird indentation block, bringing your eyes back to where the rest of the function is indented at. See 41f4dff.

zepumph · 2020-12-28T21:18:42Z

I updated doc about the unclamped/clamped fitness Property. Please review. I didn't rename them though, because I still kinda feel like their current naming does a good job of trying to point people towards this.ratioFitnessProperty instead of treating both as equals. If you still feel differently after reading the updated doc, I'll be happy to rename.

Just let me know what you think.

…ial values, #286

zepumph · 2020-12-28T21:30:49Z

ratio-and-proportion/js/common/model/RAPRatio.js

Lines 85 to 88 in f99e894

// REVIEW: The linkage below seems quite complex, and appears to be a consequence of the design choice to use a

// tuple for the ratio instead of just independent antecedent and consequent properties. And yet, there are derived

// versions of antecedent and consequent properties above anyway. Wouldn't it be simpler to do away with the tuple

// and thus never be in a situation where it needed to be recreated because half of it had changed?

I totally understand why you feel this way. We started with that approach, and ended up factoring things out to a tuple, even though some input modalities (like RatioHalf's dragListener) require a single Property to be passed to it, and doesn't work with the tuple. The tuple is vital for changing both hands at once, and in supporting the "locked ratio" state. Otherwise you get complicated reentrant cases where changing the antecedent cascades to the consequent to snap it back into ratio.

One thing I was thinking about doing though to make things more clear, was to remove the antecedentProperty and consequentProperty from the model, and force the view to create them as needed. This, to me, helps focus the model around a single tuple. I'll create an issue to do that. I agree that it is confusing to have both the tuple and separate Properties.

zepumph · 2020-12-28T21:32:48Z

See #327.

zepumph · 2020-12-28T23:58:03Z

All review comments have been removed, and if I felt like something more was needed, I mentioned them above. @jbphet back to you. I'm going to head over to #327 now.

jbphet · 2021-01-13T00:34:56Z

I think we're good here. There are a handful of child issues that are being followed up on, so we're safe to close this.

zepumph assigned jbphet Dec 12, 2020

jbphet added a commit that referenced this issue Dec 16, 2020

review comments and minor doc cleanup, see #286

d811650

zepumph mentioned this issue Dec 18, 2020

Inline most query parameters #274

Closed

zepumph added a commit that referenced this issue Dec 21, 2020

mix misspelling/review comment, #286

d8b0dcb

jbphet added a commit that referenced this issue Dec 23, 2020

added review comments, see #286

c37374f

jbphet assigned zepumph and unassigned jbphet Dec 23, 2020

zepumph mentioned this issue Dec 23, 2020

Rewrite fitness algorithm #325

Closed

zepumph added a commit that referenced this issue Dec 28, 2020

add doc and initialize target with default RAPRatio state, #286

9225f3a

zepumph added a commit that referenced this issue Dec 28, 2020

factor out fitnessRange into a constant, #286

fbb0b91

zepumph added a commit that referenced this issue Dec 28, 2020

updated indentation, #286

41f4dff

zepumph added a commit that referenced this issue Dec 28, 2020

update doc about unclampedFitnessProperty and ratioFitnessProperty, #286

3a3bd48

zepumph added a commit that referenced this issue Dec 28, 2020

add assertion about normalized Ratio term values, and factor out init…

f99e894

…ial values, #286

zepumph mentioned this issue Dec 28, 2020

Remove RAPRatio.antecedentProperty and RAPRatio.concequentProperty from model #327

Closed

zepumph added a commit that referenced this issue Dec 28, 2020

remove comment, #327 #286

510c28f

zepumph added a commit that referenced this issue Dec 28, 2020

use config for BothHandsInteractionListener.js, #286

eb65388

zepumph added a commit that referenced this issue Dec 28, 2020

convert too many parameters to a config object, #286

cf6c36c

zepumph added a commit that referenced this issue Dec 28, 2020

review comments, #286

1e4b796

zepumph added a commit that referenced this issue Dec 28, 2020

doc, #286

6bb7a31

zepumph added a commit that referenced this issue Dec 28, 2020

doc, #286

9fdbd5d

zepumph assigned jbphet and unassigned zepumph Dec 28, 2020

zepumph added the status:ready-for-review label Dec 28, 2020

This was referenced Dec 29, 2020

Can't open my challenge accordion box by clicking on the title with ?shuffleListeners #328

Closed

Context Responses for Lock Ratio Checkbox #227

Closed

Hands don't move with number keys when ratio is locked at 1:1 #329

Closed

zepumph added the status:blocks-sim-publication label Jan 4, 2021

jbphet closed this as completed Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review #286

Code Review #286

zepumph commented Dec 12, 2020 •

edited by jbphet

Loading

zepumph commented Dec 15, 2020

jbphet commented Dec 23, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

jbphet commented Jan 13, 2021

Code Review #286

Code Review #286

Comments

zepumph commented Dec 12, 2020 • edited by jbphet Loading

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

Table of Contents

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

zepumph commented Dec 15, 2020

jbphet commented Dec 23, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

zepumph commented Dec 28, 2020

jbphet commented Jan 13, 2021

zepumph commented Dec 12, 2020 •

edited by jbphet

Loading