Closed samreid closed 1 year ago
The responsible dev is responsible for removing the irrelevant parts
A checked-off item doesn't mean "no problem here", it means "it was reviewed"
Problems can be noted in side issues that reference this issue, or through // REVIEW
comments in the code
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.
[x] model.md, see https://github.com/phetsims/center-and-variability/issues/443. Familiarize yourself with the model by reading model.md. Does it adequately describe the model, in terms appropriate for teachers? Has it been reviewed by the sim designer?
[x] implementation-notes.md, see https://github.com/phetsims/center-and-variability/issues/442. Familiarize yourself with the implementation by reading implementation-notes.md. Does it provide an overview that will be useful to future maintainers?
[x] results of memory testing for brands=phet
, see https://github.com/phetsims/center-and-variability/issues/452
[x] results of memory testing for brands=phet-io
(if the sim is instrumented for PhET-iO), see https://github.com/phetsims/center-and-variability/issues/452
[x] performance testing and sign-off, see https://github.com/phetsims/center-and-variability/issues/461
[x] review of pointer areas, see https://github.com/phetsims/center-and-variability/issues/462
[x] credits (will not be completed until after RC testing), see https://github.com/phetsims/center-and-variability/issues/453
If any of these items fail, pause code review.
ea
)fuzz&ea
)ea&listenerOrder=random
and ea&listenerOrder=random&fuzz
)?deprecationWarnings
. Do not use deprecated methods in new code.grunt --minify.mangle=false
. Compare to testing results done by the responsible developer. Results can be found in https://github.com/phetsims/center-and-variability/issues/452.dispose
function, or is it obvious why it isn't necessary, or is there documentation
about why dispose
isn't called? An example of why no call to dispose
is needed is if the component is used in
a ScreenView
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
or lazyLink
is accompanied by unlink
.Multilink.multilink
is accompanied by unmultilink
.Multilink
is accompanied by dispose
.DerivedProperty
is accompanied by dispose
.Emitter.addListener
is accompanied by removeListener
.ObservableArrayDef.element*Emitter.addListener
is accompanied by ObservableArrayDef.element*Emitter.removeListener
Node.addInputListener
is accompanied by removeInputListener
PhetioObject
is accompanied by dispose
.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.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.stringTest=dynamic
and use the left/right arrow keys.stringTest=X
. You should see nothing but 'X' strings.)stringTest=double
and stringTest=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. 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.
(6) It is acceptable to prefix families of strings with a prefix, like so:
"material.water": {
"value": "Water"
},
"material.wood": {
"value": "Wood"
},
"shape.block": {
"value": "Block"
},
"shape.cone": {
"value": "Cone"
},
Nested substructure is not yet fully supported.
[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).
my-repo/
assets/
doc/
images/
*see annotation
model.md
implementation-notes.md
images/
license.json
js/
(see section below)
mipmaps/
license.json
sound/
license.json
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.
[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:
my-repo/
js/
common/
model/
view/
introduction/
model/
view/
lab/
model/
view/
my-repo-main.js
myRepo.js
myRepoStrings.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
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. The abbreviation should be all uppercase letters; e.g. MOTHAConstants, not MotHAConstants for "Model of the Hydrogen Atom".
[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
or grunt 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
the FB
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
use ProfileColorProperty
where appropriate, even if they have a single (default) profile (to support color editing
and PhET-iO Studio). The ColorProfile
pattern was converted to *Colors.js
files in
https://github.com/phetsims/scenery-phet/issues/515. Please see
GasPropertiesColors.js
for a good example.
DOT/Utils.toFixed
or DOT/Utils.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.string.includes
and string.startsWith
where possible.public constructor( tickMarksVisibleProperty: Property<boolean>,
model: Pick<IntroModel, 'changeWaterLevel'>, // <-- Note the call site can pass the whole model, but we declare we will only use this part of it
waterCup: WaterCup, modelViewTransform: ModelViewTransform2,
providedOptions?: WaterCup3DNodeOptions ) {
cd {{repo}}/js ; wc -l `find . -name "*.ts" -print` | sort
TODO
or FIXME
or REVIEW
comments in the code? They should be addressed or promoted to GitHub issues. SR: Yes, they are tracked in TODO and in issues (issue URL listed in the TODO){{REPO}}Constants.js
file?PhetColorScheme
. Identify any colors that might be worth adding to PhetColorScheme
.DerivedProperty
instead of Property
?This section may be omitted if the sim has not been instrumented with accessibility features. Accessibility includes various features, not all are always include. Ignore sections that do not apply.
fuzzBoard&ea
)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.dt
values are used instead of Date.now()
or other Date functions. Perhaps try
phet.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 whether toStateObject
should use null
or undefined
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!Are the source files reasonable in size? Scrutinize large files with too many responsibilities - can responsibilities be broken into smaller delegates?
I noticed that CardNodeContainer was significantly larger than all other files and it is getting a bit unwieldy to navigate. Especially since more logic is now going to be added for https://github.com/phetsims/center-and-variability/issues/433.
I went ahead and factored out the interactive portions of the Card Container (both view and model). I think more can be done. My next step will probably be to factor out the celebration animation code.
@marlitas and @matthew-blackman and I reviewed this issue and I think it is ready to close. It can still be used by @pixelzoom or others for reference as the code review proceeds.
We need to prepare this sim for code review by performing a self-code-review where we follow the code review checklist and make sure the code is ready for review. See comments in https://github.com/phetsims/center-and-variability/issues/447