Closed pixelzoom closed 7 years ago
5/11/17 dev meeting:
@jbphet will do memory testing and go through the checklist.
Plan A: Try to have this ready for @pixelzoom to do on Tuesday 5/16/17. Plan B: Have someone else do the review.
@jbphet assign to me when the sim is ready for review.
Summary of 5/15/17 email update from @jbphet and @ariel-phet: Sim is not ready for review, so I won't be doing the code review. Someone else will be assigned to this issue when when the sim is ready.
Hmm... I noticed that some of the items in the checklist are checked off. @jbphet are you checking them off as you do your own preliminary review?
Yes I was. I can rescind those checkmarks if the intent was to have someone other than me go through the whole thing.
This checklist was intended for the reviewer. But no harm in using it yourself, then unchecking everything before assigning it to the reviewer. Until then, everyone just needs to be aware that the checklist doesn't represent the status of the code review.
I've unchecked them to avoid any confusion and will track my own progress on the list separately.
Marking for dev meeting to draw straws and figure out who gets to do this review.
I believe I've completed the code review to the depth that would be best. I haven't fully verified all of the logic (there is a lot required by this sim), but so far it looks good.
A few questions/notes that aren't covered by inline review comments or github issues:
@params
and @returns
or visibility documentation was also missing too. The ordering between visibility, @constructor
, @returns
, @params
, and the main documentation seemed inconsistent, but seems to be fine to leave as-is. Documenting the parameters and return values of functions passed with {Function}
would almost always be helpful. Keeping {Object} [options]
as 'optional' be default is probably better for documentation, with a note if something named 'options' is required. Documenting the type of options in the _.extend
is also helpful for readability, but I don't believe it's required by our style?null
as the initial shape to a Path
is preferred to something that isn't used.left
, right
, center
, etc. as Nodes do, and is preferred to minX
, maxX
, etc. in layout code.cornerRadius: radius
in Rectangle
options may be better than passing the radius two times in the parameters list.null
or 'transparent'
are the preferred "transparent" fill/stroke values (null
preferred, except for cases where it is not allowed, like in a color Property).I've left most review issues inline in the code (there were no major structural issues that would benefit from standalone issues). @jbphet, feel free to remove REVIEW statements at your discretion, or ideally create an issue referencing the line if there are questions or objections about the comment. Feedback on what review comments are helpful, and which are unhelpful or unneeded would be great!
Memory issues are most likely to be resolved by not recreating certain nodes in CoinNodeFactory, and reusing the results (through DAG usage). If that doesn't work (i.e. if it's a problem with Scenery's instance tree being too large), rasterization of coin parts may be helpful. Discussing with @jbphet on a call would be most helpful before memory work is done, so let me know when you're available.
Checked off all issues that have pending review notes.
Review and responses complete - closing.
PhET code-review checklist
Build and Run Checks
Internationalization
{binaryProbability: "Binary Probability"}
. Screen names should use camelcase, like soscreen.screenName
. For patterns that contain placeholders (e.g."My name is {{first}} {{last}}"
) choose keys that are unlikley 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
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.
grunt published-README
orgrunt unpublished-README
?Coding conventions
grunt update-copyright-dates
.Documentation
{{REPO}}QueryParameters
, for example ArithmeticQueryParameters for the aritmetic repository.@author
annotations seem correct?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). For instance, the following methods (and perhaps others) should not be used:Math.random
_.shuffle
_.sample
_.random
new Random()
Organization, Readability, Maintainability
Performance, Usability
dt
capped appropriately? Try switching applications or browser tabs, then switch back. Did the model take one big/long/awkward step forward? If so,dt
may need to be capped. Example fromfaradays-law.FaradaysLawModel
:Memory Leaks
Property.link
is accompanied byProperty.unlink
.PropertySet.link
is accompanied byPropertySet.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
.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.PhET-iO