Closed pixelzoom closed 5 years ago
I'm going to unassign this for a bit - we just did a "pixel polishing" review and came up with a number of things to change (see https://github.com/phetsims/blackbody-spectrum/issues/34), so I'll assign this once those items are taken care of.
In the 9/27/2018 developer meeting it was decided that @chrisklus would do the code review instead of @pixelzoom when this is ready.
ea
)fuzz&ea
)[x] 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
. There should be a GitHub issue showing the results of testing done by the primary developer.
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/72. The rest of this section will be evaluated once the sim is no longer leaking.
[x] For each common-code component (sun, scenery-phet, vegas, …) that opaquely registers observers or listeners, is there a call to that component’s dispose
function, or documentation about why dispose
is unnecessary?
[x] Are there leaks due to registering observers or listeners? These guidelines should be followed, or documentation added about why following them is not necessary:
Property.link
is accompanied by Property.unlink
.DerivedProperty
is accompanied by dispose
.Multilink
is accompanied by dispose
.Events.on
is accompanied by Events.off
.Emitter.addListener
is accompanied by Emitter.removeListener
.Node.on
is accompanied by Node.off
tandem.addInstance
is accompanied by tandem.removeInstance
, or use PhetioObject constructor+dispose[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.
[x] 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)
webgl=false
)showPointerAreas
)showPointerAreas
) Some overlap may be OK depending on the z-ordering (if the frontmost object is supposed to occlude touch/mouse areas)[ ] Are there any strings that are not being internationalized? (run with query parameter stringTest=x
, you should see nothing but 'x' strings)
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/74.
[x] Does the sim layout gracefully handle internationalized strings that are twice as long as the English strings? (run with query parameter stringTest=double
)
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/75.
[x] Does the sim layout gracefully handle internationalized strings that are exceptionally long? (run with query parameter stringTest=long
)
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/75.
[x] Does the sim layout gracefully handle internationalized strings that are shorter than the English strings? (run with query parameter stringTest=X
)
[x] Does the sim stay on the sim page (doesn't redirect to an external page) when running with the query parameter stringTest=xss
? This test passes if sim does not redirect, OK if sim crashes or fails to fully start. Only test on one desktop platform.
[x] Use named placeholders (e.g. "{{value}} {{units}}"
) instead of numbered placeholders (e.g. "{0} {1}"
).
[x] Make sure the string keys are all perfect - they are difficult to change after 1.0.0 is published.
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/76.
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] 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).
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/77.
my-repo/
assets/
audio/
license.json
doc/
images/
*see annotation
model.md
implementation-notes.md
images/
license.json
js/
(see section below)
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.
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.
UPDATE: see https://github.com/phetsims/blackbody-spectrum/commit/1c67aaea963cb071ce2e4956d7b0ce02d74aebaa
[x] 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 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
)
[x] 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?
[x] Does a GitHub issue exist for tracking credits, to ensure that they are correct before publication?
[x] In GitHub, verify that all non-release branches have an associated issue that describes their purpose.
[x] Are there any GitHub branches that are no longer needed and should be deleted?
[x] Does model.md
adequately describe the model, in terms appropriate for teachers?
UPDATE: see #77.
[x] Does implementation-notes.md
adequately describe the implementation, with an overview that will be useful to future maintainers?
UPDATE: see #77.
[x] 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.
grunt update-copyright-dates
.var numPart; // incorrect
var numberOfParticles; // correct
var width; // incorrect
var beakerWidth; // correct
// modules
var inherit = require( 'PHET_CORE/inherit' );
var Line = require( 'SCENERY/nodes/Line' );
var Rectangle = require( 'SCENERY/nodes/Rectangle' );
// strings
var kineticString = require( 'string!ENERGY/energy.kinetic' );
var potentialString = require( 'string!ENERGY/energy.potential' );
var thermalString = require( 'string!ENERGY/energy.thermal' );
// images
var energyImage = require( 'image!ENERGY/energy.png' );
// audio
var kineticAudio = require( 'audio!ENERGY/energy' );
[x] Do the @author
annotations seem correct?
[x] Are all constructors marked with @constructor
? That will make them easier to search and review. This is not necessary
for ES6 constructors.
[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.
/**
* @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
*/
function BallNode( ball, visibleProperty, fill, stroke, lineWidth ){
// ...
}
// Call site
var 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.
/**
* @param {Ball} ball - model element
* @param {Property.<boolean>} visibleProperty - is the ball visible?
* @param {Object} [options]
* @constructor
*/
function BallNode( ball, visibleProperty, options ) {
options = _.extend( {
fill: 'white', // {Color|string} fill color
stroke: 'black', // {Color|string} stroke color
lineWidth: 1 // {number} width of the stroke
}, options );
// ...
}
// Call site
var ballNode = new BallNode( ball, visibleProperty, {
fill: 'blue',
stroke: 'black',
lineWidth: 2
} );
Example:
/**
* @param {ParticleBox} particleBox - model element
* @param {Property.<boolean>} visibleProperty - are the box and its contents visible?
* @param {Object} [options]
* @constructor
*/
function ParticleBoxNode( particleBox, visibleProperty, options ) {
options = _.extend( {
fill: 'white', // {Color|string} fill color
stroke: 'black', // {Color|string} stroke color
lineWidth: 1, // {number} width of the stroke
particleNodeOptions: null, // {*} to be filled in with defaults below
}, options );
options.particleNodeOptions = _.extend( {
fill: 'red',
stroke: 'gray',
lineWidth: 0.5
}, options.particleNodeOptions );
// 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.
@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
*/
function PhetDeveloper( 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] 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:
var 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] Where inheritance is needed, use PHET_CORE/inherit
(ES5) or extends
(ES6). 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:
return inherit( Object, Line, {
/**
* Gets the slope of the line
* @returns {number}
*/
getSlope: function() {
if ( this.undefinedSlope() ) {
return Number.NaN;
}
else {
return this.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() ) {
return Number.NaN;
}
else {
return ( this.getSlope() * ( x - this.x1 ) ) + this.y1;
}
}
} );
// avoid
self[ isFaceSmile ? 'smile' : 'frown' ]();
// OK isFaceSmile ? self.smile() : self.frown();
// OK if ( isFaceSmile ) { self.smile(); } else { self.frown(); }
- [x] It is not uncommon to use conditional shorthand and short circuiting for invocation.
```js
( 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();
var 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.
[x] Are Property value validation options (valueType
, validValues
, etc...) utilized? Is their presence or lack thereof properly documented?
[x] Line comments should generally be preceded by a blank line. For example:
// Randomly choose an existing crystal to possibly bond to
var crystal = this.crystals.get( _.random( this.crystals.length - 1 ) );
// Find a good configuration to have the particles move toward
var targetConfiguration = this.getTargetConfiguration( crystal );
[x] Line comments should have whitespace between the //
and the first letter of the line comment. See the preceding example.
[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.
[x] 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.
[x] Every type, method and property should be documented.
[x] 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] 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.
UPDATE: see https://github.com/phetsims/blackbody-spectrum/issues/78
[x] Abstract methods (normally implemented with an error) should be marked with @abstract
jsdoc.
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:
[x] Use @public
for anything that is intended to be part of the public API.
[x] Use @protected
for anything that is intended for use by subtypes.
[x] Use @private
for anything that is NOT intended to be part of the public or protected API.
[x] Put qualifiers in parenthesis after the annotation, for example:
[x] 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)
[x] To qualify that something is public to a specific repository, use (for example) @public (scenery-internal)
[x] For something made public solely for a11y, use @public (a11y)
[x] For something made public solely for phet-io, use @public (phet-io)
[x] Separate multiple qualifiers with commas. For example: @public (scenery-internal, read-only)
[x] Specify the most general type clients should know about. For example;
// @public (read-only) {Node}
this.myNode = new VeryComplicatedNodeSubclass()
[x] 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
*/
// @public Adds a {function} listener
addListener: function( listener ) { /*...*/ }
x.y = something
: [\w]+\.[\w]+\s=
[\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.Number.parseInt()
Array.prototype.find
grunt find-duplicates
TODO
or FIXME
comments in the code? They should be addressed or promoted to GitHub issues.{{REPO}}Constants.js
file?PhetColorScheme
. Identify any colors that might be worth adding to PhetColorScheme
.DerivedProperty
instead of Property
?Sim is now ready for code review
Review complete. Looking good @arnabp, nice work!
Any items on the checklist that are not checked have been marked with a corresponding "UPDATE:", with a link to the work that needs to be done to finish that item. There are other incomplete items on the checklist that have been checked off because the remaining work for them is outlined with REVIEW comments in code. There are 53 REVIEWs marked in the code, most of which are minor.
I've fixed up all REVIEW comments and resolved the code review referenced issues. Ready for @chrisklus to check that all relevant changes have been made.
Looks really good @arnabp, nice work. I reviewed the relevant issues and checked off their corresponding boxes.
https://github.com/phetsims/blackbody-spectrum/issues/74 is the only issue that requires any more work, so I'm going to close this issue.
@jbphet will assign this back to @pixelzoom when the sim is ready for code review.
I previously did an informal code review in https://github.com/phetsims/blackbody-spectrum/issues/15.