Open zepumph opened 6 years ago
NOTE! Prior to doing a code review, copy this checklist to a GitHub issue for the repository being reviewed. Please mark failed items with ❌
ea
)fuzzMouse&ea
)~#### Memory Leaks~ see https://github.com/phetsims/fluid-pressure-and-flow/issues/308
dispose
function, or documentation about why dispose
is unnecessary?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
.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.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)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": { "value": "Binary Probability" }
. "screen.{{screenName}}"
, e.g. "screen.lab"
. "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. my-repo/
assets/
audio/
license.json
doc/
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
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.
my-repo/
js/
common/
model/
view/
introduction/
model/
view/
lab/
model/
view/
my-repo-config.js
my-repo-main.js
myRepo.js
grunt published-README
or grunt unpublished-README
?ifphetio
plugin and PHET_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.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' );
[ ] 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.
/**
* @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
} );
@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 ) {}
[ ] 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.
[ ] 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();
[ ] 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!
[ ] 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:
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(); }
- [ ] 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 :(’;
[ ] 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 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 );
[ ] 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.
[ ] Every type, method and property should be documented.
[ ] 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.
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
*/
// @public Adds a {function} listener
addListener: function( listener ) { /*...*/ }
x.y = something
: [\w]+\.[\w]+\s=
[\w]+: function\(
dot.Util.roundSymmetric
is used instead of Math.round
. Math.round
does not treat positive and negative numbers symmetrically, see https://github.com/phetsims/dot/issues/35#issuecomment-113587879.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.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()
grunt find-duplicates
TODO
or FIXME
comments in the code? They should be addressed or promoted to GitHub issues.DerivedProperty
instead of Property
?@zepumph probably not a bad first step, but agreed, the main goal would be bringing things up to current standard. This sim was code reviewed, but it was quite some time ago (over 2 years), and lots has changed since then.
String keys for screen names should have the general form "screen.{{screenName}}", e.g. "screen.lab"
In https://github.com/phetsims/fluid-pressure-and-flow/issues/300, @samreid said that we can't change string keys for the string keys used by the "Under Pressure" screen, so I think we are stuck with underPressureScreenTitle
. My question is if it is better to keep all the screen titles like that for consistency, or to change the other two screens to use the screen.flow
type key. @samreid what do you think?
That would be a great question for @pixelzoom or @jbphet!
I recall commenting on a similar question from @jbphet about one of his sims recently, but can't find the issue. My feedback was something like "given how much work is involved in changing a string key, I would live with the inconsistency".
"given how much work is involved in changing a string key, I would live with the inconsistency".
I'm not sure if that applies directly to the question at hand, because they aren't currently inconsistent, but I'm wondering if it is worth changing the other screen title keys to match convention, creating an inconsistency and leaving the under pressure screen key in the dust.
@zepumph asked:
I'm wondering if it is worth changing the other screen title keys to match convention, creating an inconsistency and leaving the under pressure screen key in the dust.
Yes, fix unpublished screen title keys to follow convention. We can live with underPressureScreenTitle
not following convention.
I agree that we should live with it. Here is a link to a similar issue that @pixelzoom referenced above where I listed what would be involved in changing it: https://github.com/phetsims/expression-exchange/issues/126.
It was suggested by @samreid that I give this sim a code review before really diving into the sim. I like this idea, but I don't want to loose track of the goal which is gauged towards bringing the sim up to current PhET standards, and familiarizing myself with the code. I care less about pieces of the review that I think are just going to change soon anyways.
tagging @ariel-phet to make sure that this aligns with his thoughts on the matter.