Closed jbphet closed 1 year ago
Code review is done. The sim is in great shape. All issues that I created are linked above, and there were no "REVIEW" comments added to the code. Note the ❌ and ⚠️ annotations in the checklist above. Back to @jbphet...
Everything above has been addressed and the sim is now in the RC stage, so I'm going to go ahead and close this.
PhET Code-Review Checklist (a.k.a "CRC")
// REVIEW
comments in the codeSpecific Instructions
Greenhouse Effect is ready for code review. This sim is a bit unusual for a couple of reasons. One is that it is already available on the main site because it had been published as a prototype and then a decision was made to promote it to a full release. Another is that it contains all of the code for the Molecules and Light (M&L) sim, since one of the screens of Greenhouse Effect was supposed to be a variation of the M&L behavior, but that has not happened yet. So, for this review, please do not review any of the code in the "micro" directory. The primary functional difference between the code that was published a few months ago and the current code on master is that support for phet-io has been added to the 2nd and 3rd screens and improved in the first.
GitHub Issues
The following standard GitHub issues should exist. If these issues are missing, or have not been completed, pause code review until the issues have been created and addressed by the responsible dev.
brands=phet
, see https://github.com/phetsims/greenhouse-effect/issues/327brands=phet-io
(if the sim is instrumented for PhET-iO), see https://github.com/phetsims/greenhouse-effect/issues/327Build and Run Checks
If any of these items fail, pause code review.
ea
)fuzz&ea
)ea&listenerOrder=random
andea&listenerOrder=random&fuzz
)?deprecationWarnings
. Do not use deprecated methods in new code.Memory Leaks
grunt --minify.mangle=false
. Compare to testing results done by the responsible developer. Results can be found in https://github.com/phetsims/greenhouse-effect/issues/327.dispose
function, or is it obvious why it isn't necessary, or is there documentation about whydispose
isn't called? An example of why no call todispose
is needed is if the component is used in aScreenView
that would never be removed from the scene graph. Note that it's also acceptable (and encouraged!) to describe what needs to be disposed in implementation-notes.md.Property.link
orlazyLink
is accompanied byunlink
.Multilink.multilink
is accompanied byunmultilink
.Multilink
is accompanied bydispose
.DerivedProperty
is accompanied bydispose
.Emitter.addListener
is accompanied byremoveListener
.ObservableArrayDef.element*Emitter.addListener
is accompanied byObservableArrayDef.element*Emitter.removeListener
Node.addInputListener
is accompanied byremoveInputListener
PhetioObject
is accompanied bydispose
.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. See #336Performance
webgl=false
)Usability
showPointerAreas
)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
stringTest=dynamic
and use the left/right arrow keys.stringTest=X
. You should see nothing but 'X' strings.)stringTest=double
andstringTest=long
)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/1377StringUtils.fillIn
and a string pattern to ensure that strings are properly localized."{{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. They should be literal, they should be explicit, and they should be consistent. This will also be the PhET-iO phetioID name in Studio etc. See https://github.com/phetsims/greenhouse-effect/issues/290 for some additional information for this particular sim Guidelines for string keys are:
(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. (6) It is acceptable to prefix families of strings with a prefix, like so:Nested substructure is not yet fully supported.
Repository Structure
[x] The repository name should correspond to the sim title. For example, if the sim title is "Wave Interference", then the repository name should be "wave-interference".
[x] Are all required files and directories present? For a sim repository named “my-repo”, the general structure should look like this (where assets/, images/, mipmaps/ or sounds/ may be omitted if the sim doesn’t have those types of resource files).
*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.
[x] Verify that the same image file is not present in both images/ and mipmaps/. If you need a mipmap, use it for all occurrences of the image.
[x] 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:
[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 repository, 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.[x] Is there a file in assets/ for every resource file in sound/ 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 resources might not have a corresponding asset file.
[x] For simulations, was the README.md generated by
grunt published-README
orgrunt unpublished-README
? Common code repos can have custom README files.[x] Does package.json refer to any dependencies that are not used by the sim?
[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] 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] Sim-specific query parameters (if any) should be identified and documented in one .js file in js/common/ or js/ ( if there is no common/). The .js file should be named
{{PREFIX}}QueryParameters.js
, for example ArithmeticQueryParameters.js for the arithmetic repository, or FBQueryParameters.js for Function Builder (where theFB
prefix is used).[x] Query parameters that are public-facing should be identified using
public: true
in the schema.[x] All sims should use a color file named
MyRepoColors.js
or, if using abbreviations,MRColors.js
, and useProfileColorProperty
where appropriate, even if they have a single (default) profile (to support color editing and PhET-iO Studio). TheColorProfile
pattern was converted to*Colors.js
files in https://github.com/phetsims/scenery-phet/issues/515. Please see GasPropertiesColors.js for a good example.Coding Conventions
TypeScript Conventions
Math Libraries
DOT/Utils.toFixed
orDOT/Utils.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.IE11
string.includes
andstring.startsWith
where possible.Organization, Readability, and Maintainability
TODO
orFIXME
orREVIEW
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 toPhetColorScheme
.DerivedProperty
instead ofProperty
?Accessibility
This sim supports alternative input, and some amount of description has been implemented, but it is not complete. The general plan is that full description will be implemented "later", but it is not clear at this time when that will be. For the purposes of this review, code related to description should be largely ignored since it is likely to change quite a bit when the time is taken to fully implement it.
General
Alternative Input
fuzzBoard&ea
)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.
PhetioObject
instances are disposed, which unregisters their tandems. Some follow-up work was recommended as part of this review which has been completed, see #336.dt
values are used instead ofDate.now()
or other Date functions. Perhaps tryphet.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.DOT/dotRandom
as an imported module (not a global), 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.undefined
values are omitted when serializing objects across frames. Consider this when determining whethertoStateObject
should usenull
orundefined
values.AXON/EnabledProperty
. This should be done in both the model and the view. If you're using a DerivedProperty, skip this item.phetioDocumentaton
- it changes the PhET-iO API!