Closed pixelzoom closed 4 years ago
This has become a liability over in phetsims/tandem#180, where a PhetioGroup refactor is in progress. I can't just search for "Group.array". So I'll take care of this.
I confirmed with the iO team on Slack that this.chargedParticles
should be renamed to this.chargedParticleGroup
, to follow the convention of naming groups with "Group" suffix, and to follow the convention of using the same name for a field and its tandem.
Just to increase the level of difficulty, this sim has both {PhetioGroup} chargedParticles
and {ObservableArray} chargedParticles
.
Done in the above commit. I also added assertions for the cases where there is {ObservableArray} chargedParticles
.
@jonathanolson is the responsible dev, but commit history seems to indicate that @zepumph did the PhetioGroup instrumentation. I'll assign to both of them -- one of you please review and close.
Changes look good to me! Thanks for the extra assertions for clarity. Closing
Noted while working on https://github.com/phetsims/beers-law-lab/issues/244, where this sim was recommended as an exemplar for PhetioGroup. Same problem as https://github.com/phetsims/projectile-motion/issues/221.
In ChargesAndFieldsModel.js:
The name
this.chargedParticles
is confusing. It's not charged particles at all, its a group that's used to instantiate charged particles. And it doesn't match the tandem name. Recommended to rename tothis.chargedParticlesGroup
.Also worth noting:
There are several other PhetioGroups created in this sim, and their field names have "Group" suffix (
this.electricFieldSensorGroup
,this.electricPotentialLineGroup
,...)I find the pattern used in projectile-motion for
PhetioGroup
instantiation to be much nicer. That is, add acreateGroup
method to the class that needs to be instantiate via a PhetioGroup, rather than havingnew PhetioGroup
throughout the code as is done in charges-and-fields.