search

phetsims / build-an-atom

"Build an Atom" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/build-an-atom
GNU General Public License v3.0
11 stars 10 forks source link

code review #2 #44

Closed pixelzoom closed 11 years ago

pixelzoom commented 11 years ago

@jbphet will indicate when it's ready for review. @pixelzoom will do it. @jonathanolson will review for scenery performance.

pixelzoom commented 11 years ago

General comment: Code would benefit from taking a look through all source files and adding a bit of JSdoc. Mostly for constructors, other functions look generally good. JSdoc type annotations (e.g. {ParticleAtom}) would be useful for parameters whose types are not obvious or might be ambiguous. For example, when i see an 'atom' parameter, I don't know if it's a NumberAtom or ParticleAtom. Not necessary for parameters that are obviously number, booleans, etc. Useful for function parameters to document what parameters they take and what (if anything) they return.

pixelzoom commented 11 years ago

Finished code review. No additional issues created, only 'REVIEW' tags in the code. Code is in overall great shape, easy to read. A bit more JSdoc (as mentioned above) is the only thing that would have made reading the code a bit easier.

I paid particular attention to loops and callbacks, but I didn't see any obvious performance issues.

Let me know if there's anything you want to discuss.

pixelzoom commented 11 years ago

Oops, I lied... I did create 1 issue during code review: https://github.com/phetsims/build-an-atom/issues/55

pixelzoom commented 11 years ago

Skimmed through the commits, looks good, closing this issue.