review Build a Nucleus
Closed this issue · 4 comments
@ariel-phet asked me to do a quick review of "Build a Nucleus", https://github.com/Luisav1/Build-a-Nucleus, created by a 3rd party. Instructions:
By the end of the month would be fine, not a rush
ideally have a few review comments that she could address, since that way I can in essence treat it like a "test task" and see how she does with some feedback.
coming at it from the point of view that she did this on her own, without any real guidance from us besides our documentation
I'm not familiar with the domain, but overall a nice looking sim. And a fine effort considering that this was done independently, with no direct assistance from PhET. In retrospect, it would have been better to have @jbphet review this, since it's essentially a modification of the "Atom" screen of build-an-atom. That said...
Room for improvement:
-
Repository structure is incorrect, and prevents running grunt tasks. The sim cannot be built, linted, etc.
-
Sim fails immediately when assertions are enabled with
?ea. -
Repository contains a deep copy of most dependency, but is missing some dependencies required by build tools (e.g. sherpa). Relying on repo copies means that the implementation will (and probably has) quickly diverge from master.
-
There are no useful commit messages in GitHub. Most of the commit messages say “Add files via upload”. Descriptive messages should always be used when committing changes. Summarize what work you did in the commit message.
-
Most (all?) of the work appears to have been done in Build-a-Nucleus/build-an-atom/js/buildanatom. I can’t tell if files in the copies of other (dependency) repositories were changed - it would take me awhile to figure out.
-
Code comments seem to be reasonable, following the style of build-an-atom.
-
PhET code-style formatting needs to be applied.
-
There are at least 46 .js files under Build-a-Nucleus/build-an-atom/js/ that are no longer used, but still exist. That leaves only 9 files that are actually relevant.
-
The sim was created by making big modifications to a few build-an-atom files, mainly: AtomView.js, BuildAnAtomModel.js, BuildAnAtomView.js. I suspect (but can’t confirm) that these files still contain stuff that’s specific to build-an-atom and isn’t needed or used by Build-a-Nucleus.
-
One new file was added, NuclideChart.js. This is a specialization of shred.NuclideChartNode. Inheritance is most appropriate to use here (
class ... extends NuclideChartNode), but the implementation uses composition. Missing @author annotation. -
It would have been preferable to create a new build-a-nucleus repository, and copy/adapt the bits needed from build-an-atom. Using the "copy everything" approach, it would be difficult (but not out of the question) to make this a new screen (and sim) in a build-an-atom “suite”.
Recommendations:
- Rename Build-a-Nucleus repo to build-a-nucleus
- Delete copies of dependency repos and depend on the actual PhET repos
- Move contents of build-an-atom/ up to top-level of build-a-nucleus
- Rename top-level files to build-a-nucleus (build-a-nucleus_en.html, build-a-nucleus-strings_en.json, build-a-nucleus-main.js, etc.)
- Rename js/build-an-atom to js/build-a-nucleus
- Move SymbolNode.js from js/symbol/view/ to js/common/view/
- Delete unused .js files
- Rename BuildAnAtomScreen to BuildANucleusScreen
- Rename BuildAnAtomView to BuildANucleusScreenView
- Rename BuildAnAtomModel to BuildANucleusModel, move to js/build-a-nucleus/model/
Bonus points, highly recommend if this sim were adopted by PhET:
- Convert to ES6 class
- Move PeriodicTableAndSymbol.js to common code (shred repo?), parameterize, and share with build-an-atom. The value of constant SYMBOL_WIDTH_PROPORTION is the only thing that was changed.
- Move SymbolNode.js to common code (shred repo?), parameterize, and share with build-an-atom. There were quite a few changes made to this file, but they all look like they could be handled by adding parameters to SymbolNode.
- Convert this to an additional screen in a build-an-atom “suite”.
Back to @ariel-phet for review and questions.
I have sent these review comments to the student, and she will be addressing them and let me know when the sim is ready for further review.
I'm closing this, as we now have a phet-brand for Build a Nucleus and these files are deleted.