Closed Denz1994 closed 5 years ago
Assigning to @ariel-phet for prioritization and reviewer.
Went through all the additional check items, performed memory testing, and looked a bit more at code that changed a lot.
Added a few REVIEW items in commits above. Code review is complete.
Notes to the Reviewer
This sim inherits heavily from the model and view of Masses-And-Springs. It may be worth reading over the implementation-notes.md and model.md from Masses-And-Springs until the docs are fleshed out for the Masses-And-Springs-Basics repo.
Major differences involve:
ColorProfile
GravityAccordionBox
,DraggableRulerNode
onStrecthScreenView
, and adjusted checkbox options)Any changes that may leak into the Masses-And-Springs repo should be captured in the next maintenance release of that sim.
Please add
// REVIEW
comments or issue comments as needed. New issues should be opened for any problem that is non-trivial. If there are questions or concerns, I'll be available during the review via slack or zoom.NOTE! Prior to doing a code review, copy this checklist to a GitHub issue for the repository being reviewed. Please mark failed items with ❌
PhET code-review checklist
Build and Run Checks
ea
)fuzz&ea
)Memory Leaks
grunt --mangle=false
. There should be a GitHub issue showing the results of testing done by the primary developer.dispose
function, or documentation about whydispose
is unnecessary?Property.link
is accompanied byProperty.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
, or use PhetioObject constructor+disposedispose
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.Performance, Usability
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)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}"
).(1) Strings keys should generally match their values. E.g.:
(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.:
(3) If string key names would collide, use your judgment to disambiguate. E.g.:
(4) String keys for screen names should have the general form
"screen.{{screenName}}"
. E.g.:(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.Repository structure
*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.
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.grunt published-README
orgrunt unpublished-README
?grunt generate-config
)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
.[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.
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.
Example:
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:[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. Theself
variable should not be defined unless it is needed in a closure. Example:[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) orextends
(ES6). 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:// OK isFaceSmile ? self.smile() : self.frown();
// OK if ( isFaceSmile ) { self.smile(); } else { self.frown(); }
If the expression is only one item, the parentheses can be omitted. This is the most common use case.
[x] 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.[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:
[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.[x] Abstract methods (normally implemented with an error) should be marked with
@abstract
jsdoc.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 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;
[x] For JSDoc-style comments, the annotation should appear in context like this:
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 https://github.com/phetsims/dot/issues/35#issuecomment-113587879.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). Including, but not limited to:Math.random
_.shuffle
_.sample
_.random
new Random()
IE11
Number.parseInt()
Array.prototype.find
Organization, Readability, Maintainability
grunt find-duplicates
TODO
orFIXME
comments in the code? They should be addressed or promoted to GitHub issues.DerivedProperty
instead ofProperty
?PhET-iO