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

directories and top-level classes do not follow PhET naming conventions #218

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

Noted while working on https://github.com/phetsims/tasks/issues/1052.

build-an-atom violates several PhET naming conventions. This made it unnecessarily difficult to find things while doing the above review.

Violations and suggested changes:

(1) js/ subdirectories are named based on screens. "Atom" screen should be in js/atom/, not js/build-an-atom.

(2) ScreenView subclass names have "ScreenView" suffix. Violated by AtomView, BuildAnAtomView, and SymbolView. BAAGameScreenView is the only ScreenView subclass that follows the convention.

(3) Naming of Screen, ScreenView, and Model subclasses is mostly not following PhET conventions. SymbolScreen and GameScreen follow PhET conventions, others do not. Rename these to follow PhET conventions:

jbphet commented 3 years ago

A lot of this stems from the name of the first screen having been changed from "Build an Atom" to simply "Atom" a while back. I guess I never updated all the type names when that happened.

I've implemented all of the suggestions except this one:

BuildAnAtomModel -> AtomModel, the model for the "Atom" screen, and move to js/atom/model/

BuildAnAtomModel is actually used by the first two screens, which is why it's in the common directory. I updated the header comment to reflect this and left the name as is. Hopefully this will help to avoid confusion in the future.

@pixelzoom - I'm going to go ahead and close this and assume that you don't feel the need to review, but you're welcome to if you're so inclined.