You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Does the html file size seem reasonable, compared to other similar sims?
Does the sim start up? (requirejs and built versions)
Does the sim experience any assertion failures? (run with query parameter ea)
Does the sim pass a scenery fuzz test? (run with query parameters fuzzMouse&ea)
Internationalization
Are there any strings that are not being internationalized? (run with query parameter stringTest=x, you should see nothing but 'x' strings)
Does the sim layout gracefully handle internationalized strings that are twice as long as the English strings? (run with query parameter stringTest=double)
❌ Does the sim layout gracefully handle internationalized strings that are exceptionally long? (run with query parameter stringTest=long)
Does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter stringTest=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.
Use named placeholders (e.g. "{{value}} {{units}}") instead of numbered placeholders (e.g. "{0} {1}").
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 camelcase, like so screen.screenName. For patterns that contain placeholders (e.g. "My name is {{first}} {{last}}") choose 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.
Repository structure
Are all required files and directories present?
For a sim repository named “my-repo”, the general structure should look like this (where assets/, audio/ or images/ may be omitted if the sim doesn’t have those types of assets).
For a common-code repository, the structure is similar, but some of the files and directories may not be present if the repo doesn’t have audio, images, strings, or a demo application.
Is the js/ directory properly structured?
All JavaScript source should be in the js/ directory. There should be a subdirectory for each screen (this also applies for single-screen sims, 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:
Is there a file in assets/ for every resource file in audio/ and images/? Note that there is not necessarily a 1:1 correspondence between asset and resource files; for example, several 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.
Was the README.md generated by grunt published-README or grunt unpublished-README?
Does package.json refer to any dependencies that are not used by the sim?
Does sim-config.js refer to any dependencies that are not used by the sim? Note: ifphetio plugin and PHET_IO dependencies should be included in all config files.
Does package.json contain phetLibs for all non-default entries in sim-config.json?
Is the LICENSE file correct? (GPL v3 for sims, MIT for common code)
Does .gitignore match the one in simula-rasa?
Does a GitHub issue exist for tracking credits, to ensure that they are correct before publication?
Are there git repository branches that are no longer used and should be pruned?
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
Is the code formatted according to PhET conventions? See phet-idea-code-style.xml for IntelliJ IDEA code style.
Are copyright headers present and up to date? Run grunt update-copyright-dates.
Names (types, variables, properties, functions,...) should be sufficiently descriptive and specific, and should avoid non-standard abbreviations. For example:
Require statements should be organized into blocks, with the code modules first, followed by strings, images and audio (any order ok for strings/images/audio). For modules, the var name should match the file name. Example below.
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.
/** * @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 */functionBallNode(ball,visibleProperty,fill,stroke,lineWidth){
...
}// Call sitevarballNode=newBallNode(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.
/** * @param {Ball} ball - model element * @param {Property.<boolean>} visibleProperty - is the ball visible? * @param {Object} [options] * @constructor */functionBallNode(ball,visibleProperty,options){options=_.extend({fill: 'white',// {Color|string} fill colorstroke: 'black',// {Color|string} stroke colorlineWidth: 1// {number} width of the stroke},options);// ...}// Call sitevarballNode=newBallNode(ball,visibleProperty,{fill: 'blue',stroke: 'black',lineWidth: 2});
Constructor and function documentation. Parameter types and names should be clearly specified for each function and constructor (if there are any parameters) using @param annotations. The description for each parameter should follow a hyphen. Primitive types should use lower case. Constructors should additionally include the @constructor annotation. For example:
/** * The PhetDeveloper is responsible for creating code for simulations * and documenting their code thoroughly. * * @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] - optional configuration, see constructor * @constructor */functionPhetDeveloper(name,age,isEmployee,callback,hoursProperty,friendNames,options){}
For most functions, the same form as above should be used, with a @return 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.
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:
Comments should not extend beyond 120 columns. Break long comments into multiple lines to optimize readability (use your judgement).
Where inheritance is needed, use PHET_CORE/inherit. Add prototype and static functions via the appropriate arguments to inherit. Spaces should exist between the function names unless the functions are all short and closely related. Example:
returninherit(Object,Line,{/** * Gets the slope of the line * @returns {number} */getSlope: function(){if(this.undefinedSlope()){returnNumber.NaN;}else{returnthis.rise/this.run;}},/** * Given x, solve y = m(x - x1) + y1. Returns NaN if the solution is not unique, or there is no solution (x can't * possibly be on the line.) This occurs when we have a vertical line, with no run. * @param {number} x - the x coordinate * @returns {number} the solution */solveY: function(x){if(this.undefinedSlope()){returnNumber.NaN;}else{return(this.getSlope()*(x-this.x1))+this.y1;}}});
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.
Line comments should be preceded by a blank line. For example:
// Randomly choose an existing crystal to possibly bond tovarcrystal=this.crystals.get(_.random(this.crystals.length-1));// Find a good configuration to have the particles move towardvartargetConfiguration=this.getTargetConfiguration(crystal);
Line comments should have whitespace between the // and the first letter of the line comment. See the preceding example.
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.
Files should be named like CapitalizedCamelCasing.js when returning a constructor, or lower-case-style.js when returning a non-constructor function. When returning a constructor, the constructor name should match the filename.
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/CSS5/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.
Visibility Annotations
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)
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 Adds a {function} listener
addListener: function(listener){/*...*/}
Now make sure every javascript property has a visibility annotation. Here are some helpful regexes 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\(
DOT/Util.toFixed or DOT/Util.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.
Check that random numbers are generated using phet.joist.random, and are 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
new Random()
Organization, Readability, Maintainability
Does the organization and structure of the code make sense? Do the model and view contain types that you would expect (or guess!) by looking at the sim? Do the names of things correspond to the names that you see in the user interface?
Are appropriate design patterns used?
Is inheritance used where appropriate? Does the type hierarchy make sense?
Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)
Is there too much unnecessary decoupling? (e.g. by passing all of the properties of an object independently instead of passing the object itself)?
Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates?
The simulation should use Property instead of PropertySet.
Are all dependent properties modeled as DerivedProperty instead of Property?
Performance, Usability
Does the sim perform as desired across the range of supported platforms? (eg, not too slow on slow platforms, not too fast on fast platforms)
If the sim uses WebGL, does it have a fallback? Does the fallback perform reasonably well? (run with query parameter webgl=false)
Are UI components sufficiently responsive? (especially continuous UI components, such as sliders)
Are pointer areas optimized, especially for touch? (run with query parameter showPointerAreas)
Do pointer areas overlap? (run with query parameter showPointerAreas)
Memory Leaks
Does a heap comparison using Chrome Developer Tools indicate a memory leak? (Describing this process is beyond the scope of this document.)
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?
Are there leaks due to registering observers or listeners? These guidelines should be followed, or documentation added about why following them is not necessary:
AXON: Property.link is accompanied by Property.unlink.
AXON: PropertySet.link is accompanied by PropertySet.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: tandem.addInstance is accompanied by tandem.removeInstance.
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.
@andrea-phet yes, the html file in built versions. Basically we noticed that john travoltage was quite large after various a11y features were added. Since we have been adding features to the sims (a11y related, PhET-iO related, it seemed wise to start making sure we were not accidentally/unknowingly bloating the size of the production sim too much).
A checklist for AL to follow
PhET code-review checklist
Build and Run Checks - Initial
ea
)fuzzMouse&ea
)Internationalization
stringTest=x
, you should see nothing but 'x' strings)stringTest=double
)stringTest=long
)stringTest=X
)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."{{value}} {{units}}"
) instead of numbered placeholders (e.g."{0} {1}"
).{binaryProbability: "Binary Probability"}
. Screen names should use camelcase, like soscreen.screenName
. For patterns that contain placeholders (e.g."My name is {{first}} {{last}}"
) choose 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.Repository structure
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).
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.
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:
grunt published-README
orgrunt unpublished-README
?ifphetio
plugin andPHET_IO
dependencies should be included in all config files.model.md
adequately describe the model, in terms appropriate for teachers?implementation-notes.md
adequately describe the implementation, with an overview that will be useful to future maintainers?{{REPO}}QueryParameters.js
, for example ArithmeticQueryParameters.js for the aritmetic repository.Coding Conventions
grunt update-copyright-dates
.Do the
@author
annotations seem correct?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.
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.
@param
annotations. The description for each parameter should follow a hyphen. Primitive types should use lower case. Constructors should additionally include the@constructor
annotation. For example:For most functions, the same form as above should be used, with a
@return
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.
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. Theself
variable should not be defined unless it is needed in a closure. Example:Comments should not extend beyond 120 columns. Break long comments into multiple lines to optimize readability (use your judgement).
Where inheritance is needed, use
PHET_CORE/inherit
. Add prototype and static functions via the appropriate arguments toinherit
. Spaces should exist between the function names unless the functions are all short and closely related. Example:If the expression is only one item, the parentheses can be omitted. This is the most common use case.
Naming for Property values: All
AXON/Property
instances should be declared with the suffixProperty
. For example, if a visible property is added, it should have the namevisibleProperty
instead of simplyvisible
. This will help to avoid confusion with non-Property definitions.Line comments should be preceded by a blank line. For example:
Line comments should have whitespace between the
//
and the first letter of the line comment. See the preceding example.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.Files should be named like CapitalizedCamelCasing.js when returning a constructor, or lower-case-style.js when returning a non-constructor function. When returning a constructor, the constructor name should match the filename.
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/CSS5/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.
Visibility Annotations
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>
orObject[]
instead ofArray<Object>
). PhET guidelinesfor 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)
Separate multiple qualifiers with commas. For example:
@public (scenery-internal, read-only)
For JSDoc-style comments, the annotation should appear in context like this:
these declarations as PhET uses them.
x.y = something
:[\w]+\.[\w]+\s=
[\w]+: function\(
Math Libraries
dot.Util.roundSymmetric
is used instead ofMath.round
.Math.round
does not treat positive and negative numbers symmetrically, see fix nearest-neighbor rounding in Util.toFixed dot#35 (comment).DOT/Util.toFixed
orDOT/Util.toFixedNumber
should be used instead oftoFixed
. JavaScript'stoFixed
is notoriously buggy. Behavior differs depending on browser, because the spec doesn't specify whether to round or floor.phet.joist.random
, and are 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
new Random()
Organization, Readability, Maintainability
grunt find-duplicates
TODO
orFIXME
comments in the code? They should be addressed or promoted to GitHub issues.Does changing the values of these constants break the sim? For example, see allow minimum rows to go to "1" and address dependency on current minimum of "5" plinko-probability#84.
Property
instead ofPropertySet
.DerivedProperty
instead ofProperty
?Performance, Usability
webgl=false
)showPointerAreas
)showPointerAreas
)Memory Leaks
dispose
function, or documentation about whydispose
is unnecessary?Property.link
is accompanied byProperty.unlink
.PropertySet.link
is accompanied byPropertySet.unlink
.DerivedProperty
is accompanied bydispose
.Multilink
is accompanied bydispose
.Events.on
is accompanied byEvents.off
.Emitter.addListener
is accompanied byEmitter.removeListener
.Node.on
is accompanied byNode.off
tandem.addInstance
is accompanied bytandem.removeInstance
.dispose
function have one? This should expose a publicdispose
function that callsthis.disposeMyType()
, wheredisposeMyType
is a private function declared in the constructor.MyType
should exactly match the filename.PhET-iO
for the PhET-iO development process.
Build and Run Checks - Final
ea
)fuzzMouse&ea
)The text was updated successfully, but these errors were encountered: