Closed jbphet closed 1 year ago
That makes a ton of sense to me. Thanks for the thoughtful ticket and excellent suggestions. I'll take a look.
@Luisav1 can you please review these changes and feel free to make updates.
These changes look great thanks @zepumph! Closing.
Related to #165.
The model class
ParticleNucleus
has a model-view transform (MVT) that is used to implement some significant parts of its functionality. It is pretty unconventional to see an MVT in the model code, since the general idea in a model-view-controller architecture is that a model is this standalone entity that does its thing in some real coordinate frame (e.g. meters) or an arbitrary one (e.g. normalized 2D space where x and y range from 0 to 1) and that model information is then mapped into one or more views. The MVTs are therefore generally in the view code, not in the model code.Seeing this during the code review created a bit of cognitive dissonance for me, but after scrutinizing it for a short while, I think I understand the intention. The
NUCLEON_ENERGY_LEVEL_ARRAY_MVT
is being used to create the essentially arbitrary coordinate frame in the model based on the radii of the particles, which is then used to position the particles, which allows the particles' internal movement code to implement the desired animations.So, assuming I've understood the situation correctly, I have some suggestions for how to make this code easier for future maintainers to understand:
ParticleNucleus
amodelViewTransform
. Maybe something likeparticlePositioningTransform
would make its purpose more clearBAAConstants
that are related to this and are not actually shared with other classes, such asNUCLEON_ENERGY_LEVEL_ARRAY_MVT
, intoParticleNucleus
.If I've misunderstood this, then I'd suggest thinking about why it's confusing and what could be done to make it less so.