identify and address unnecessary coupling to model

[pixelzoom]

Related to #2 (code review):

• [ ] Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)

Unnecessary coupling is when a class knows about (or has access to) things that are not related to its responsibilities. And it's one of the biggest factors in writing maintainable code.

In this sim, there are many constructors that take a model parameter. This is a red flag that there is unnecessary coupling. Generally, only a couple of things in the model are needed by a class, not the entire model.

For example SpringNode is passed the entire OneDimensionModel instance:

  class SpringNode extends Node {

    /**
     * @param {Spring} spring
     * @param {ModelViewTransform2} modelViewTransform
     * @param {OneDimensionModel} model
     * @param {Tandem} tandem
     */
    constructor( spring, modelViewTransform, model, tandem ) {

But it uses only model.springsVisibilityProperty. So a better API design would decouple SpringNode from OneDimensionModel like this:

  class SpringNode extends Node {

    /**
     * @param {Spring} spring
     * @param {ModelViewTransform2} modelViewTransform
     * @param {Property.<boolean>} springsVisibleProperty - whether the springs are visible
     * @param {Tandem} tandem
     */
    constructor( spring, modelViewTransform, model, tandem ) {

Based on this use of model parameter, the following constructor should be revisited, to evaluate whether they need the entire model or just a few of its Properties:

// @param {Model} model
OptionsPanel
AmpPhaseAccordionBox
GraphAccordionBox
ModeGraphCanvasNode
StaticModeGraphCanvasNode
AmpSelectorAccordionBox
AmpSelectorRectNode

// @param {OneDimensionModel} model
MassNode
SpringNode
MassNode1D
WallNode

// @param {TwoDimensionsModel} model
MassNode2D

[pixelzoom]

I should also point out that the code-review checklist also has this item:

• [ ] Is there too much unnecessary decoupling? (e.g. by passing all of the properties of an object independently instead of passing the object itself)?

This identifies a trade-off between doupling and number of parameters in a constructor/method. If a class needs to know about most of the API for another class, then you may decide that it's preferable to pass the entire object instance. Use your judgement.

[pixelzoom]

If you're looking for a good introduction to Coupling (and how it contrasts with Cohesion), see https://www.javatpoint.com/software-engineering-coupling-and-cohesion.

[Hyodar]

In the latest commit, I fixed some cases in which model could be easily replaced (or just removed). About this, I also went and counted the number of model attributes that some of the other classes that got model as a constructor parameter used: at least 4. Is it justifiable to pass the entire model instance in these cases?

[pixelzoom]

I don't know if 4 is the magic number, but that sounds reasonable for this sim.

I inspected the remaining occurrences of @param {...} model, and have a few suggestions:

• MassNode, MassNode1D, MassNode2D: Remove constructor param model from MassNode, it's not used by MassNode, its used by subclasses. And subclasses can use their constructor args. Also these fields are not needed in MassNode, you can just use the constructor args:

this.mass = mass;
this.modelViewTransform = modelViewTransform;
this.model = model;

• SpringNode: Replace constructor param model with springsVisibilityProperty -- it's the only model field that is used. Also you don't need these fields, you can just use the constructor args:

this.spring = spring;
this.modelViewTransform = modelViewTransform;
this.model = model;

• OneDimensionalScreenView, TwoDimensionsScreenView: These fields are not needed, you can just use the constructor args:

this.model
this.modelViewTransform

[Hyodar]

Makes sense, those didn't need to be added as attributes as they could be used directly on the constructor. About SpringNode, I didn't change it in the last commit because I was thinking if the property was really necessary, so I waited to take a look at it again today. Anyway, I'll do what you suggested and, if necessary, create another issue about SpringNode. Thanks!

[Hyodar]

Made the discussed changes in the previous commits. On OneDimensionScreenView and TwoDimensionsScreenView, though, this.modelViewTransform wasn't set by a constructor argument; it was created inside the constructor, so this wasn't changed.

[pixelzoom]

Right. But this.modelViewTransform does not need to be a field, it can be a const. See above commit. In general, try to create fields (this.something) only when necessary.

[pixelzoom]

I removed the design:interviews label from this issue. I added it by mistake when the issue was created. That label is typically used for things that need to be tested with users.

[Hyodar]

Understood! It's something to remember. If I come across any more cases of unnecessary fields that could be replaced in this way, I'll do it.

[Hyodar]

Went through all the files looking for fields that were not used or that could be replaced for const. In the latest commit I probably fixed all of them.

[Hyodar]

I think this can already be closed.

What do you think, @pixelzoom? Assigning for review.

[pixelzoom]

Looks great @Hyodar! Closing.