-
Notifications
You must be signed in to change notification settings - Fork 8
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 2.0 #251
Comments
samreid
added a commit
that referenced
this issue
Oct 14, 2015
samreid
added a commit
that referenced
this issue
Oct 14, 2015
samreid
added a commit
that referenced
this issue
Oct 14, 2015
I turned on stricter eslint rules (commented out in the .eslintrc) and iterated until everything is passing. |
samreid
added a commit
that referenced
this issue
Oct 14, 2015
I looked through common for unused code or obvious issues. Still need to do other directories. |
samreid
added a commit
that referenced
this issue
Oct 15, 2015
samreid
added a commit
that referenced
this issue
Oct 15, 2015
samreid
added a commit
that referenced
this issue
Oct 15, 2015
This was referenced Oct 16, 2015
Closed
samreid
added a commit
that referenced
this issue
Oct 16, 2015
samreid
added a commit
that referenced
this issue
Oct 16, 2015
This was referenced Oct 19, 2015
Everything is crossed off or moved to another issue, closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
After all of the changes in the last few months, the code should be repolished and re-reviewed.
PhET code-review checklist
Build and Run Checks
Repository structure
Are all required files and directories present?
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).
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.
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). 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 “Custom”, the general directory structure should look like this:
grunt generate-published-README
orgrunt generate-unpublished-README
?chipper/js/grunt/Gruntfile.js
?Coding conventions
Documentation
Common Errors
Math.round
used wheredot.Util.roundSymmetric
should be used? Math.round does not treat positive and negative numbers symmetrically, see fix nearest-neighbor rounding in Util.toFixed dot#35 (comment)toFixed
used wheredot.Util.toFixed
ordot.Util.toFixedNumber
should be used? JavaScript'stoFixed
is notoriously buggy, behavior differs depending on browser, because the spec doesn't specify whether to round or floor.Organization, Readability, Maintainability
Performance, Usability
Memory Leaks
@samreid: I tested intro and prisms screen with heap analysis, and did not see any leaks.
@samreid: Most of the instances exist for the life of the sim. Those that don't exist for the life of the sim don't create Properties.
tandem.addInstance
should be accompanied bytandem.removeInstance
or documented why removeInstance is unnecessary.@samreid: same argument as above
The text was updated successfully, but these errors were encountered: