unnecessary coupling in AtomPropertiesPanel and RutherfordAtomNode

[pixelzoom]

Related to #30 (code review).

Constructor for AtomPropertiesPanel is currently:

 /**
   * Constructor for a Atom Properties control panel.
   *
   * @param {RSBaseModel} model - The model controlled by this panel.
   * @param {Object} [options]
   * @constructor
   */
  function AtomPropertiesPanel( model, options ) {

There's unnecessary coupling with the model here, since AtomPropertiesPanel only needs 3 Properties from model: userInteractionProperty, protonCountProperty, neutronCountProperty.

Recommended to change constructor to:

 /**
   * Constructor for a Atom Properties control panel.
   *
   * @param {Property.<boolean>} userInteractionProperty
   * @param {Property.<boolean>} protonCountProperty
   * @param {Property.<boolean>} neutronCountProperty
   * @param {Object} [options]
   * @constructor
   */
  function AtomPropertiesPanel( model, options ) {

[pixelzoom]

Ditto in RutherfordAtomNode, same 3 Properties are used from the model.

[pixelzoom]

More thoughts on this issue...

(1) The userInteraction Property shouldn't be in RSBaseModel, it's only applicable to the Rutherford atom, so should be specific to RutherfordAtomModel.

(2) While it's nice to reduce coupling by passing in only the parts of the model that are needed, it's a drag to have a lot of constructor parameters. In the case of these 2 constructors ( AtomPropertiesPanel and RutherfordAtomNode), instead of passing in 3 Properties as I proposed in https://github.com/phetsims/rutherford-scattering/issues/52#issue-138632221, we should be passing in an "atom" object that encapsulates those 3 Properties. But we don't currently have such a type, probably didn't exist in the Java implementation. How about if we create one? Something like this (untested code):

function RutherfordAtom( options ) {
  PropertySet.call( this, {
    protonCount: RSConstants.DEFAULT_PROTON_COUNT, // number of protons in the atom
    neutronCount: RSConstants.DEFAULT_NEUTRON_COUNT, // number of neutrons in the atom
    userInteraction: false // {boolean} is the user interacting with the simulation?
  } );
}

return inherit( PropertySet, RutherfordAtom );

Remove the userInteraction Property from RSBaseModel, and change RutherfordAtomModel constructor to:

  function RutherfordAtomModel( ) {
    RSBaseModel.call( this );
    this.atom = new RutherfordAtom();
  }

AtomPropertiesPanel constructor becomes:

/**
   * Constructor for a Atom Properties control panel.
   *
   * @param {RutherfordAtom} atom
   * @param {Object} [options]
   * @constructor
   */
  function AtomPropertiesPanel( atom, options ) {

Similar change for for RutherfordAtomNode constructor; pass in {RutherfordAtom} atom instead of model.

Then any call site that uses one of these Properties needs to be changed. For example, 'model. protonCountbecomesmodel.atom.protonCount`.

[schmitzware]

I'm very reluctant to change this. The userInteraction property is also used in the AlphaParticlesPropertiesPanel which doesn't care about protons or neutrons. Do I then create another abstraction for just the userInteraction property for that case?

To me, this feels like over abstracting. The app is maybe 3000 lines of code (common excluded). While technically, I can see your point, I wouldn't want to see the 'model' become a dumping ground but the benefit of these type of things is IMHO negligible - with this, I add a new file that is, what 10 lines of code..? If it supplied some other set of functionality/features I think it would make more sense but it doesn't seem to really be fixing anything, it's just a PropertySet.

Let me know what you think. I'm going to wait on #51 also until I hear back.

[pixelzoom]

Ah sorry, I missed the fact that userInteraction is also relevant to Plumb Pudding. So fine not to create type RutherfordAtom.

Up to you on whether to decouple as described in https://github.com/phetsims/rutherford-scattering/issues/52#issue-138632221. There are only 7 public fields in the model, 3 of which are relevant to AtomPropertiesPanel and RutherfordAtomModel (userInteraction, protonCount, neutronCount). If there was a lot more stuff accessible in the model, I'd probably push back - especially since we know that the sim will be enhanced, that the enhancement involves modifying the model, and that the work will be done by a different developer. But like you said, not a big deal in the current state.

[pixelzoom]

Ditto for #51.

[schmitzware]

AtomPropertiesPanel fix is in 5218563a4eeed2da111fcda4f80e6d53aa665df0 (which has been merged into master)

[pixelzoom]

Look good.

Two type expressions incorrect in both AtomPropertiesPanel and RutherfordAtomNode, these should be number instead of boolean:

   * @param {Property.<boolean>} protonCountProperty
   * @param {Property.<boolean>} neutronCountProperty

Feel free to close after fixing those.