-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code Review #63
Comments
@Denz1994 @chrisklus if either/both of you want, we can make some time to go over this sim and the ISLC code in the next week or so depending on your priorities with M&S. Just let me know. |
@mbarlow12 @Denz1994 that sounds great - friday, monday, or tuesday would be best for me in the next week. |
Friday (07/25/18) during core hours (10:00am MT) would be a good time for a preview of the code base for me. Does that sound good @mbarlow12 @chrisklus? |
@Denz1994 good with me! |
Sorry guys, I missed the notification on this. @Denz1994, I assume you're referring to 7/27, right? If so, we're all set! |
@mbarlow12 @Denz1994 @chrisklus are there any updates for this issue? Just excited to see an ISLC sim published! |
@mbarlow12 During build we get this message. Full context:
This image might be stale and should be removed. |
It was advised by @phet-steele to alphebetize this list of strings in the respective .json files (phetsims/masses-and-springs#151). I am relaying this suggestion along to @mbarlow12. |
|
Note: Commit 0d26abc above has an incorrect commit message. It is corrected in a comment on the commit. |
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Memory Profiling Session:
From this session, I have no reason to believe there is a memory leak present. A similar test was run with @jonathanolson for a shorter duration and there wasn't any signs of a memory leak. Additionally, a built version of the sim was run under the same conditions and similar results were produced (I didn't save the snapshot data unfortunately). Based I don't see an issue referencing a memory profile for this sim so I will address it here as satifactory. |
@Denz1994 I had some thoughts on your review comment
Perhaps instead of MathSymbols, do you think it'd be more useful to have a |
…Property: Property -> NumberProperty, see #63
…type docs, boolean property for options, see #63
…argeNode, positioning option updates, see #63
…tructor, property type annotations, legend positioning option, see #63
@mbarlow12 said:
Units typically require i18n, and therefore must be in strings.json files. Internally... Validation of the |
The usages of the columb unit don't use unicode characters (also true in Capacitance-Lab-Basics). Out of all of the metric prefixes only My intent in #63 (comment) was to add a unified symbol for coulumb in MathSymbol.js, but after rethinking this, it may not be needed. |
Regardless of whether the coulomb units uses a Unicode character, it still needs to be translatable. In general, it's not appropriate to put units in MathSymbols.js. |
Thanks for the discussion—am I correct in assessing that the recommended course of action is to keep the units where they are? @pixelzoom |
To make my sims "ready to instrument", I've lately been following this practice for use of Property and its subclasses. Wherever possible/appropriate:
The code-review checklist, and/or the how to instrument document might want to include something about this. |
@mbarlow12 asked:
Yes, coulombs-law-strings_en.json is the correct place for units that are visible in the UI. |
Thanks @pixelzoom I'll find a decent spot in those docs tomorrow to make a note and take the steps you mentioned in CL before handing back to @Denz1994 for final review. Thanks again for the input. |
@Denz1994 the above items from your comments/discussion are done. Back to you for final check before closing. |
Relevant issues have been addressed and |
Thanks @Denz1994! Closing and moving into dev testing at phetsims/qa#203. New dev release at: https://phet-dev.colorado.edu/html/coulombs-law/1.0.0-dev.19/phet/coulombs-law_en_phet.html |
Please mark failed items with ❌
PhET code-review checklist
Build and Run Checks
ea
)fuzzMouse&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}"
)."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.Repository structure
For a sim repository named “my-repo”, the general structure should look like this (where assets/, audio/ or images/ may be omitted if the sim doesn’t have those types of assets).
*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.
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:
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
.Do the
@author
annotations seem correct?Are all constructors marked with
@constructor
? That will make them easier to search and review.❌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 phetsims/tasks#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: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. Theself
variable should not be defined unless it is needed in a closure. Example:❌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 toinherit
. Spaces should exist between the function names unless the functions are all short and closely related. Example:If the expression is only one item, the parentheses can be omitted. This is the most common use case.
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.Line comments should generally be preceded by a blank line. For example:
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/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.
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:@public
for anything that is intended to be part of the public API.@protected
for anything that is intended for use by subtypes.@private
for anything that is NOT intended to be part of the public or protected API.@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)@public (scenery-internal)
@public (a11y)
@public (phet-io)
@public (scenery-internal, read-only)
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 fix nearest-neighbor rounding in Util.toFixed dot#35 (comment).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 example, the following methods (and perhaps others) should not be used:Math.random
_.shuffle
_.sample
_.random
new Random()
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
for the PhET-iO development process.
The text was updated successfully, but these errors were encountered: