Closed pixelzoom closed 1 year ago
pixelzoom
commented
1 year ago Coupling to CAVModel (and its subclasses) seems to be a general problem. I'm finding it in other classes. So I'll re-title this issue, and list the places where I think it should be addressed.
pixelzoom
commented
1 year ago There is excessive coupling to CAVModel in the following places:
... and I gave up from there, because there are so many other cases for subclasses of CAVModel.
Search for "model:" (case insensitive) to locate and inspect.
pixelzoom
commented
1 year ago Here's an example of the problem created by excessive coupling. In CAVPlotNode this ugliness is caused by passing in model: CAVModel:
const visibleProperty = model instanceof MeanAndMedianModel && options.parentContext === 'accordion' ? model.isMeanVisibleProperty :
model instanceof VariabilityModel ? DerivedProperty.valueEqualsConstant( model.selectedVariabilityMeasureProperty, VariabilityMeasure.MAD ) :
new BooleanProperty( true );Why not just pass in the propert value for visibileProperty?
Also noting that these are the only occurrences of instanceof in the sim, and it would nice to eliminate them.
samreid
commented
1 year ago I'll work on this
samreid
commented
1 year ago I worked on this a bit, and wanted to check in before I continue. Here is my patch so far:
I wanted to raise this for discussion with @marlitas and @matthew-blackman because I'm concerned this opens up opportunity to pass the incorrect Property at the call sites. For example, one diff is:
So I'm wondering if we would prefer a Pick at the declaration sites.
Also, I'm not convinced this is better for readability at the implementation sites:
pixelzoom
commented
1 year ago ... I'm concerned this opens up opportunity to pass the incorrect Property at the call sites
Yes, that's certainly possible. But it's much less of a concern than passing in an entire CAVModel when you only need a couple of its properties.
samreid
commented
1 year ago Would a Pick be the "best of both worlds"?
pixelzoom
commented
1 year ago In general, I'm not a fan of using Pick for this. It does prevent the implementation from accessing fields that it shouldn't be using. But it still creates unnecessary coupling, because the caller is required to provide an object of a wider type than is really needed. If you want to use Pick here in sim-specific code (center-and-variability) that's up to you. But I definitely recommend against it in common code.
samreid
commented
1 year ago But it still creates unnecessary coupling, because the caller is required to provide an object of a wider type than is really needed.
It requires that the client code pass in an object with a named key. Not sure how that could be considered a wider type.
samreid
commented
1 year ago In discussion with @marlitas and @matthew-blackman and myself, we are leaning toward using Pick in circumstances where we don't need flexibility around the key name.
We did discuss, however, that at runtime (during debugging), the entire model would be available in the Pick site implementations.
Also, wanted to clarify with where @pixelzoom remarked:
But it still creates unnecessary coupling
How does the Pick create the unnecessary coupling, or are you referring to coupling of the key name?
pixelzoom
commented
1 year ago How does the Pick create the unnecessary coupling, or are you referring to coupling of the key name?
model: Pick<CAVModel, ...> is coupling. You are now dependent on the API of CAVModel, in a place where you don't need to be. Again it's not as bad as model: CAVModel, but it's unnecessary coupling.
So... Do what you'd like. I'm still not a fan of Pick when there are only a couple of fields to be passed.
pixelzoom
commented
1 year ago As an example, consider PlayAreaCheckboxFactory. getMedianCheckedSoundPlayer. Its implementation requires 1 Property, medianValueProperty: TReadOnlyProperty<number>.
I really don't understand why you would do change this:
public static getMedianCheckedSoundPlayer( model: CAVModel ): TSoundPlayer {... to this:
public static getMedianCheckedSoundPlayer( model: Pick<CAVModel, 'medianValueProperty'> ): TSoundPlayer {... instead of this:
public static getMedianCheckedSoundPlayer( medianValueProperty: TReadOnlyProperty<number> ): TSoundPlayer {Using Pick has the following problems:
medianValueProperty is writable by getMedianCheckedSoundPlayer, when it should be readonly.medianValueProperty, you'd need to pass in an object that has a medianValueProperty. This is not flexibility.Pick is more complicated, not to mention odd and unexpected. And based on the "Pick is better because it prevents you from passing in the wrong Property" argument, you would never have parameters of the form somethingProperty: Property<someType>. So I don't really buy that argument.
pixelzoom
commented
1 year ago Rather than Pick<CAVModel,...>, here's another approach that addresses some of the problems identified in the preceeding comment.
Here's the Pick<CAVModel,...> approach:
class SomeClass {
public constructor( model: Pick<CAVModel, 'isPlayAreaMedianVisibleProperty'> ) {
// reads the value of isPlayAreaMedianVisibleProperty
}
}Instead of involving CAVModel here, define a Model type that includes only the fields needed, with the proper mutable/read-only Property type as needed.
type SomeClassModel = {
isPlayAreaMedianVisibleProperty: TReadOnlyProperty<boolean>;
};
class SomeClass {
public constructor( model: SomeClassModel ) {
// reads the value of isPlayAreaMedianVisibleProperty
}
}So... am I advocating this? No. After writing the above, I realize that it doesn't prevent you from writing to the fields in SomeClassModel, because there's no notion of readonly in a type.
samreid
commented
1 year ago I am also not advocating this, but if someone wanted to follow that approach, they could mark the attribute readonly
type SomeClassModel = {
readonly isPlayAreaMedianVisibleProperty: TReadOnlyProperty<boolean>;
};OK this is implemented as @pixelzoom recommended and ready for spot-check. Please close if all is well.
UPDATE: Did not change CAVSceneView or even use Pick there since it uses 3 properties locally and several more in the parent.
samreid
commented
1 year ago UPDATE: I'm seeing subclasses of CAVModel that could get the same pattern. Self-assigning to work on those.
samreid
commented
1 year ago OK I addressed several other cases where it was better to pass through individual Property instances. Ready for review again this time, I think.
pixelzoom
commented
1 year ago Much better! A couple of remaining cases, up to you whether you want to address them. Close when done.
[x] CAVPlotNode is getting the entire model: CAVModel as a constructor parameter only so that it can do model instanceof VariabilityModel. Replace with a boolean param`?
[ ] CardModel is gettng the entire cardContainerModel: CardContainerModel as a constructor parameter only so that it can do cardContainerModel.parentContext === 'accordion'. Can you just pass in the parentContext or a boolean as the param?
samreid
commented
1 year ago Thanks, I fixed the first case as recommended.
Regarding the 2nd case:
CardModel is gettng the entire cardContainerModel: CardContainerModel as a constructor parameter only so that it can do cardContainerModel.parentContext === 'accordion'.
That is not the only reason. It is also a constructor parameter declared via public constructor( public readonly cardContainerModel: CardContainerModel so it is a public class attribute. It is accessed 3x in CardNode:
model.cardContainerModel.getCardsInCellOrder()model.cardContainerModel.getCardPositionX( cardCells.length - 1 )model.cardContainerModel.parentContextSo I feel that occurrence should remain. @pixelzoom OK to close?
pixelzoom
commented
1 year ago 👍🏻
For code review #447 ...
PlayAreaCheckboxFactory has too much coupling to CAVModel in almost every method. Each method only needs 1 Property, but takes the entire CAVModel as a parameter. For example:
This should be:
Not recommended to use (for example)
model: Pick<VariabilityModel, 'isIntervalToolVisibleProperty'>in these cases.