two-way coupling
Closed this issue · 3 comments
Related to code review #231.
- Is there any unnecessary coupling? (e.g., by passing large objects to constructors, or exposing unnecessary properties/functions)
This sim doesn't pass large objects or expose unnecessary properties/functions. But what it does do is create undesirable 2-way (aka circular) relationships. Specifically, Vector, VectorSet, and Graph all have knowledge of each other.
The undesirable relationships are:
(1) Vector has knowledge of its associated VectorSet and Graph:
class Vector extends RootVector {
/**
* @param {Vector2} initialTailPosition - starting tail position of the vector
* @param {Vector2} initialComponents - starting components of the vector
* @param {Graph} graph - the graph the vector belongs to
* @param {VectorSet} vectorSet - the vector set the vector belongs to
* @param {string|null} symbol - the symbol for the vector (i.e. 'a', 'b', 'c', ...)
* @param {Object} [options] - not propagated to super
*/
constructor( initialTailPosition, initialComponents, graph, vectorSet, symbol, options ) {(2) VectorSet has knowledge of its associated Graph:
class VectorSet {
/**
* @param {Graph} graph - the graph the VectorSet belongs to
* @param {EnumerationProperty.<ComponentVectorStyles>} componentStyleProperty - component style for all vectors
* @param {BooleanProperty} sumVisibleProperty - controls whether the sum vector is visible
* @param {VectorColorPalette} vectorColorPalette - color palette for vectors in this set
* @param {Object} [options]
*/
constructor( graph, componentStyleProperty, sumVisibleProperty, vectorColorPalette, options ) {This type of 2-way coupling makes it difficult to change things. In general, we want to keep general things (e.g. Vector) from having knowledge of the things that use/contain them (e.g. VectorSet).
I discussed this issue several weeks ago with @ariel-phet, when I first started running into changes that were unexpectedly difficult/costly because of coupling. Eliminating this coupling would require fundamental changes, would be costly (my WAG was ~60 hours) and would undoubtedly be destabilizing -- not the kind of change we want to make this late in the game, when the sim is polished and (currently) has no known functional problems. So the decision was to leave things "as is", and incur the costs during future maintenance/enhancement.
So... No code changes are anticipated as the result of this issue. It will be discussed as part of implementation postmortem to inform implementation of future sims, then closed.
@brandonLi8 and I discussed this in 10/21/19 postmortem. A good rule of thumb going forward is to look at container/contents relationships. Generally you don't want the contents to have (or require) knowledge of the container.
@veillette and I discussed this in 10/22/19 postmortem. No changes needed, closing.