Code Review: Kitten and Location Representations

[marlitas]

A review for both the model and view classes that create the Kitten and Location-based representations would be helpful, and I believe was not part of the review in #20

Please feel free to close this issue if that is incorrect.

Classes that should be a part of this review:

• KittenNode
• KittensLayerNode
• LocationCountingObjectNode
• LocationCountingObjectsLayerNode
• TenFrameButton
• CommutativeButton

Additionally, logic was added to the following files due to the implementation of the above:

• CountingObject
• NumberPairsModel
• NumberPairsScreenView

[pixelzoom]

I'm unlikely to get through all of this in one sitting, so here's feedback on what I've reviewed so far.

KittenNode

• When using keyboard focus, there needs to be a moveToFront -- which probably messes with focus. Otherwise the AB switch can sometime be hidden behind a foreground kitten.

• newKittenFocusedEmitter is a somewhat confusing name. It's not really related to focus, it's notifying that the selected kitten has changed. Adding a parameter to emit( kitten ) would allow you to get rid of this undesirble order dependency that is duplicated in 2 places:

newKittenFocusedEmitter.emit();
model.focusedProperty.value = true;

... and then you could do something like:

newKittenFocusedEmitter.addListener( focusedKitten => {
  model.focusedProperty.value = ( focusedKitten === this );
} );

• CountingObject focusedProperty is a little confusing. It's not really about focus. And here it's about whether the kitten is "selected" and its panel is therefore visible. Consider renaming.

• Setting initial value of attributePositionProperty is something that should typically be done in the model. Doing it in the view complicates "Reset All". If you need the counting area dimensions to compute initial positions, consider putting those dimensions in the model.

• const ABSwitchKeyboardListener violates naming conventions. It's also confusing because it’s not added to the ABSwitch, but rather control the Property that it toggles. Consider renaming to something like toggleAddendKeyboardListener.

model.attributePositionProperty.value = options.initialPosition;

KittenLayerNode

See above for KittenNode above renaming newKittenFocusedEmitter and having it emit( kitten ).

See above for KittenNode about initializing attributePositionProperty in the model.

[pixelzoom]

CommutativeButton

• createCommutativeArrowsIcon is 14 lines of unnecessary code, requiring 3 Nodes. Maybe not worth changing since you've already implemented it. But I always consider FontAwesome first. See right-left icon, https://fontawesome.com/icons/right-left?f=classic&s=solid

TenFrameButton

• The current implementation of the icon uses 7 Nodes where 1 would do. Consider reimplementing the icon as a single Path, as in this implementation:

createTenFrameIcon

  private static createTenFrameIcon(): Node {
    const tenFrameWidth = 48;
    const tenFrameHeight = 22;
    const tenFrameLineWidth = 2;

    // outer frame
    const shape = new Shape().rect( 0, 0, tenFrameWidth, tenFrameHeight );
    shape.moveTo( 0, tenFrameHeight / 2 );

    // horizontal line
    shape.lineTo( tenFrameWidth, tenFrameHeight / 2 );

    // vertical lines
    const verticalLineSpacing = tenFrameWidth / 5;
    _.times( 4, i => {
      shape.moveTo( verticalLineSpacing + i * verticalLineSpacing, 0 );
      shape.lineTo( verticalLineSpacing + i * verticalLineSpacing, tenFrameHeight );
    } );

    return new Path( shape, {
      stroke: 'black',
      lineWidth: tenFrameLineWidth
    } );
  }

• You've used a very different pattern for TenFrameButton than you did with CommutativeButton. CommutativeButton is passed its listener, TenFrameButton is responsible for creating its listener. I prefer the CommutativeButton pattern. Consider changing as in this patch:

patch

Subject: [PATCH] fix hBox layout at bottom of SnapshotsAccordionBox, https://github.com/phetsims/equality-explorer/issues/224
---
Index: js/common/view/NumberPairsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/NumberPairsScreenView.ts b/js/common/view/NumberPairsScreenView.ts
--- a/js/common/view/NumberPairsScreenView.ts	(revision ce5dd523244167a04d0eac8e3f903543f4780a08)
+++ b/js/common/view/NumberPairsScreenView.ts	(date 1732311692146)
@@ -140,7 +140,7 @@
     // we have access to the countingAreaBounds which are defined during construction.
     const sumTenFrameBounds = this.countingAreaBounds.erodedX( this.countingAreaBounds.width / 3.5 );
     const tenFrameBounds = options.sumScreen ? [ sumTenFrameBounds ] : NumberPairsScreenView.splitBoundsInHalf( this.countingAreaBounds );
-    const tenFrameButton = new TenFrameButton( tenFrameBounds, model.organizeIntoTenFrame.bind( model ), {
+    const tenFrameButton = new TenFrameButton( () => model.organizeIntoTenFrame( tenFrameBounds ), {
       tandem: options.tandem.createTandem( 'tenFrameButton' ),
       visibleProperty: tenFrameButtonVisibleProperty
     } );
Index: js/common/view/TenFrameButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/TenFrameButton.ts b/js/common/view/TenFrameButton.ts
--- a/js/common/view/TenFrameButton.ts	(revision ce5dd523244167a04d0eac8e3f903543f4780a08)
+++ b/js/common/view/TenFrameButton.ts	(date 1732311692170)
@@ -22,17 +22,14 @@
 export default class TenFrameButton extends RectangularPushButton {
 
   public constructor(
-    tenFrameBounds: Bounds2[],
-    organizeIntoTenFrame: ( bounds: Bounds2[] ) => void,
+    organizeIntoTenFrame: () => void,
     providedOptions: TenFrameButtonOptions
   ) {
     const tenFrameIcon = TenFrameButton.createTenFrameIcon();
     const options = optionize4<TenFrameButtonOptions, SelfOptions, RectangularPushButtonOptions>()( {},
       {
         content: tenFrameIcon,
-        listener: () => {
-          organizeIntoTenFrame( tenFrameBounds );
-        }
+        listener: organizeIntoTenFrame
       }, NumberPairsConstants.RECTANGULAR_PUSH_BUTTON_OPTIONS, providedOptions );
     super( options );
   }

Here's another pattern you might consider for both CommutativeButton and TenFrameButton. Rather than adding construtor parameters, it simply makes listener a required option:

patch

Subject: [PATCH] fix hBox layout at bottom of SnapshotsAccordionBox, https://github.com/phetsims/equality-explorer/issues/224
---
Index: js/common/view/NumberPairsScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/NumberPairsScreenView.ts b/js/common/view/NumberPairsScreenView.ts
--- a/js/common/view/NumberPairsScreenView.ts	(revision ce5dd523244167a04d0eac8e3f903543f4780a08)
+++ b/js/common/view/NumberPairsScreenView.ts	(date 1732312565858)
@@ -140,7 +140,8 @@
     // we have access to the countingAreaBounds which are defined during construction.
     const sumTenFrameBounds = this.countingAreaBounds.erodedX( this.countingAreaBounds.width / 3.5 );
     const tenFrameBounds = options.sumScreen ? [ sumTenFrameBounds ] : NumberPairsScreenView.splitBoundsInHalf( this.countingAreaBounds );
-    const tenFrameButton = new TenFrameButton( tenFrameBounds, model.organizeIntoTenFrame.bind( model ), {
+    const tenFrameButton = new TenFrameButton( {
+      listener: () => model.organizeIntoTenFrame( tenFrameBounds ),
       tandem: options.tandem.createTandem( 'tenFrameButton' ),
       visibleProperty: tenFrameButtonVisibleProperty
     } );
@@ -150,7 +151,8 @@
       tandem: options.tandem.createTandem( 'organizeBeadsButton' ),
       visibleProperty: organizeButtonVisibleProperty
     } );
-    const commutativeButton = new CommutativeButton( model.swapAddends.bind( model ), {
+    const commutativeButton = new CommutativeButton( {
+      listener: () => model.swapAddends(),
       tandem: options.tandem.createTandem( 'commutativeButton' )
     } );
     const countingAreaButtonsVBox = new VBox( {
Index: js/common/view/CommutativeButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CommutativeButton.ts b/js/common/view/CommutativeButton.ts
--- a/js/common/view/CommutativeButton.ts	(revision ce5dd523244167a04d0eac8e3f903543f4780a08)
+++ b/js/common/view/CommutativeButton.ts	(date 1732312565847)
@@ -20,17 +20,16 @@
 type SelfOptions = EmptySelfOptions;
 type CommutativeButtonOptions =
   SelfOptions
-  & PickRequired<RectangularPushButtonOptions, 'tandem'>
-  & StrictOmit<RectangularPushButtonOptions, 'content' | 'listener'>;
+  & PickRequired<RectangularPushButtonOptions, 'tandem' | 'listener'>
+  & StrictOmit<RectangularPushButtonOptions, 'content'>;
 export default class CommutativeButton extends RectangularPushButton {
 
-  public constructor( swapAddends: () => void, providedOptions: CommutativeButtonOptions ) {
+  public constructor( providedOptions: CommutativeButtonOptions ) {
 
     const commutativeArrowsIcon = CommutativeButton.createCommutativeArrowsIcon();
     const options = optionize4<CommutativeButtonOptions, SelfOptions, RectangularPushButtonOptions>()( {},
       {
         content: commutativeArrowsIcon,
-        listener: swapAddends
       }, NumberPairsConstants.RECTANGULAR_PUSH_BUTTON_OPTIONS, providedOptions );
     super( options );
   }
Index: js/common/view/TenFrameButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/TenFrameButton.ts b/js/common/view/TenFrameButton.ts
--- a/js/common/view/TenFrameButton.ts	(revision ce5dd523244167a04d0eac8e3f903543f4780a08)
+++ b/js/common/view/TenFrameButton.ts	(date 1732312565865)
@@ -16,23 +16,16 @@
 import Bounds2 from '../../../../dot/js/Bounds2.js';
 
 type SelfOptions = EmptySelfOptions;
-type TenFrameButtonOptions = SelfOptions & PickRequired<RectangularPushButtonOptions, 'tandem'>
-  & StrictOmit<RectangularPushButtonOptions, 'content' | 'listener'>;
+type TenFrameButtonOptions = SelfOptions & PickRequired<RectangularPushButtonOptions, 'tandem' | 'listener'>
+  & StrictOmit<RectangularPushButtonOptions, 'content'>;
 
 export default class TenFrameButton extends RectangularPushButton {
 
-  public constructor(
-    tenFrameBounds: Bounds2[],
-    organizeIntoTenFrame: ( bounds: Bounds2[] ) => void,
-    providedOptions: TenFrameButtonOptions
-  ) {
+  public constructor( providedOptions: TenFrameButtonOptions ) {
     const tenFrameIcon = TenFrameButton.createTenFrameIcon();
     const options = optionize4<TenFrameButtonOptions, SelfOptions, RectangularPushButtonOptions>()( {},
       {
         content: tenFrameIcon,
-        listener: () => {
-          organizeIntoTenFrame( tenFrameBounds );
-        }
       }, NumberPairsConstants.RECTANGULAR_PUSH_BUTTON_OPTIONS, providedOptions );
     super( options );
   }

[pixelzoom]

• Create a class for the "1" card. There are currently at least 3 separate/different implementations, shown below. There may be others that I didn't identify. Recommended to make the 1-card class extends Node, with children Rectangle and Text. Not extends Rectangle with child Text, as in the current implementations.

// LocationCountingObjectNode.ts

   // Create the one card.
    const oneCard = new Rectangle( 0, 0, IMAGE_WIDTH, ONE_CARD_HEIGHT, {
      fill: 'white',
      stroke: 'black',
      cornerRadius: 5,
      visibleProperty: DerivedProperty.valueEqualsConstant( countingRepresentationTypeProperty, RepresentationType.ONE_CARDS )
    } );
    const numberOne = new Text( '1', {
      font: new PhetFont( 40 ),
      center: oneCard.center,
      visibleProperty: DerivedProperty.valueEqualsConstant( countingRepresentationTypeProperty, RepresentationType.ONE_CARDS )
    } );
    oneCard.addChild( numberOne );


// CountingObjectControl.ts

const createNumberLineIcon = ( fill: TColor ) => {
  const icon = new Rectangle( 0, 0, MAX_ICON_WIDTH, MAX_ICON_HEIGHT, {
    fill: fill,
    cornerRadius: 5
  } );
  const numberOne = new Text( '1', {
    font: new PhetFont( 24 ),
    center: icon.center
  } );
  icon.addChild( numberOne );
  return icon;
};


// RepresentationType.ts

    new Rectangle( 0, 0, ICON_MAX_WIDTH, ICON_MAX_HEIGHT, {
      cornerRadius: 5,
      fill: Color.WHITE,
      stroke: 'black',
      children: [ new Text( '1', { font: new PhetFont( 18 ), center: new Vector2( ICON_MAX_WIDTH / 2, ICON_MAX_HEIGHT / 2 ) } ) ]
    } )

[pixelzoom]

LocationCountingObjectsLayerNode

• updateLocation is unused.

• I’m not clear on how GroupSelectDragInteractionView is supposed to work. Test driving didn’t help. Is it working as expected, or work in progress?

• Document getAvailableGridCoordinates.

• In handleLocationChange the if and else expressions are complicated and could use better documentation. And is it correct that if neither code block is entered, that this method will be a no-op? If so, document when and why there would be a location change that does not require any handling.

• accessibleName requires localization:

      accessibleName: 'Location Counting Objects'

[pixelzoom]

CountingObject

• It looks like StrictOmit<PhetioObjectOptions, 'phetioType' | 'phetioState'> is need for the definition of CountingObjectOptions.

• These constants seem misplaced in CountingObject. Move to KittenNode or NumberPairsConstants?

export const KITTEN_PANEL_WIDTH = 56;
export const KITTEN_PANEL_HEIGHT = 82;
export const KITTEN_PANEL_MARGIN = 3;

• As previously mentioned for KittenNode, consider renaming focusedProperty.

• Consider renaming draggingProperty to isDraggingProperty.

• Will traverseInactiveObjects need to be stateful for PhET-iO? I suspect that it needs to be a Property, with phetioReadOnly: true. I also admit that I don't really understand this field or its documentation.

• For addendTypeProperty did you mean phetioReadOnly: true? It’s currently set to phetioReadOnly: false, which is the default.

• I suspect that all of the Properties defined herein should be phetioReadOnly: true.

• Does addendTypeProperty need to be reset? If not, document why not.

• IOType definitions should always be public static readonly. Relevant to CountingObjectIO and NumberPairsSceneIO.

• IOTypes should include documentation indicating the type of serialization implemented. Your future self will thank you. Relevant to CountingObjectIO and NumberPairsSceneIO. Examples: TargetOrbitInfoIO, BunnyIO, EqualityExplorerSceneIO.

[pixelzoom]

• Use class constants instead of export const.

There are currently 12 occurrences of export const in number-pairs. In general, it's preferrable to use class constants (public static readonly), rather than export constants in this manner. That's the role of class constants, and usage sites are more readable.

For example, in NumberCircle:

Subject: [PATCH] fix hBox layout at bottom of SnapshotsAccordionBox, https://github.com/phetsims/equality-explorer/issues/224
---
Index: js/common/view/NumberCircle.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/NumberCircle.ts b/js/common/view/NumberCircle.ts
--- a/js/common/view/NumberCircle.ts	(revision eb6dd1a63cd7ff5aaca058d71cab655fac1c2e9e)
+++ b/js/common/view/NumberCircle.ts	(date 1732316585885)
@@ -17,8 +17,10 @@
 
 type NumberCircleOptions = StrictOmit<CircleOptions, 'children' | 'radius'>;
 
-export const CIRCLE_RADIUS = 30;
 export default class NumberCircle extends Circle {
+
+  public static readonly RADIUS = 30;
+
   public constructor(
     numberProperty: TReadOnlyProperty<number>,
     numberVisibleProperty: TReadOnlyProperty<boolean>,
@@ -28,7 +30,7 @@
       stroke: 'black',
       excludeInvisibleChildrenFromBounds: true
     }, providedOptions );
-    super( CIRCLE_RADIUS, options );
+    super( NumberCircle.RADIUS, options );
 
     const numberStringProperty = new DerivedProperty( [ numberProperty ], ( number: number ) => number.toString() );
     const numberText = new Text( numberStringProperty, {

[pixelzoom]

NumberPairsModel

• It’s odd to have Animation instances in the model. They are typically in the view. The model changes immediately, then the view animates to the new model state.

• dropCountingObject also seems odd in the model. The model should be unconcerned with dragging. Dragging is a view responsibility that changes model state.

NumberPairsScreenView

• ‘splitBoundsInHalf’ seems like is should live in something like NumberPairsUtils.ts.

[pixelzoom]

• Use countingObject: CountingObject instead of model: CountingObject.

I’ve been ignoring this for awhile, but have changed my mind and decided to bring it up. There are many places with field or parameter model: CountingObject. PhET typically uses model for the top-level model, something that implements TModel, like IntroModel, SumModel, etc. in this sim. And it’s more typical for a field or param name to match its type. So countingObject: CountingObject would be more idiomatic, and more readable than model: CountingObject.

[pixelzoom]

LocationCountingObjectNode

• LocationCountingObjectNodeOptions should omit children.

• Rename apple et.al. to something like appleNode. They currently sounds like model elements.

[pixelzoom]

Review is done. Back to you @marlitas. Let me know if you have any questions.

[marlitas]

It’s odd to have Animation instances in the model. They are typically in the view. The model changes immediately, then the view animates to the new model state.

I looked into this. In previous sims all of my animations have been in the model so this was a bit surprising to hear ( MeanShareandBalance and CenterAndVariability). It feels difficult to separate out the model positionProperty from the animation throughout this sim. The positionProperty is intertwined with the addendType in some scenarios, and we are also saving the positionProperty state for different scenes in the model as well. I'm not sure how to untangle all of that successfully and at this point it feels like it may be too big a lift for what it's worth.

However, I am having to rethink some things for #23. The code is currently making some assumptions that is blocking some animation requests therefore it might coincide to hit this with that work as well.

[marlitas]

Setting initial value of attributePositionProperty is something that should typically be done in the model.

Addressing this point led me to a large refactor that I hope makes reset and construction more straightforward. The benefit is that reset also behaves more closely to phet-io state setting which seems easier to digest and navigate. I'm going to keep an eye on CT for any issues this may have brought up, but I tried to test various scenarios throughout the sim as well.

[marlitas]

Will traverseInactiveObjects need to be stateful for PhET-iO? I suspect that it needs to be a Property, with phetioReadOnly: true. I also admit that I don't really understand this field or its documentation.

I don't believe so because it is mostly needed for user interaction purposes which is not an issue in state setting. I added more documentation to the traverseInactiveObjects class property. @pixelzoom Can you check and see if that helps with understanding the purpose?

[marlitas]

It’s odd to have Animation instances in the model. They are typically in the view. The model changes immediately, then the view animates to the new model state.

The animation is so closely tied to position Properties in the model that I don't think it's worth trying to move to the view. If we did we would also have to move the swap addends logic as well as the ten frame logic to the view which feels much more at home in the model. I think it's best to leave the animation as is.

[pixelzoom]

Re #32 (comment) ...

... @pixelzoom Can you check and see if that helps with understanding the purpose?

Yes, nice documentation, very helpful.

[pixelzoom]

In Slack#DM, @marlitas said:

... it probably wouldn’t be a bad idea for you to review 937c31d

That was a rather large refactor in response to:

Setting initial value of attributePositionProperty is something that should typically be done in the model. Doing it in the view complicates "Reset All".

[pixelzoom]

937c31d is a large commit. I spent ~10 minutes skimming it. While I may have missed some subtlety, I don't see any obvious problems, and it addresses my review comment about initializing and resetting attributePositionProperty.

Very minor feedback... In DecompositionModel.ts this const could benefit from documentation:

const INITIAL_VALUES_DIFFERENCE = 1;

Back to @marlitas for next steps.

[marlitas]

I’m not clear on how GroupSelectDragInteractionView is supposed to work. Test driving didn’t help. Is it working as expected, or work in progress?

I don't remember what state it was in during this review, but I added a bit of documentation and there is refinement work that will need to be done. I created a new issue for that: #77

[marlitas]

accessibleName requires localization:

I need to do accessible name work in general. Issue for that: #78

[marlitas]

Use class constants instead of export const.

@pixelzoom Why is this the convention? With export const I don't have to remember what class the const is connected to and I can get an auto complete. With class constants I generally have to search to find the class first and then can access the const. It's kind of annoying in terms of efficiency.

[pixelzoom]

@pixelzoom Why is this the convention? ...

It's pretty typical of all OO programming languages, and it clearly associates a constant with what it pertains to. If you want to do something else, go for it. Just be aware that you're doing something that you're unlikely to see in other OO languages.

[pixelzoom]

Btw... The same goes for static methods. There's no reason you couldn't define them as plain old functions outside of the class definition and export them separately. But that's not "the OO way".

[pixelzoom]

Here's another example of why static constants are preferrable.

class Proton {
  public static readonly RADIUS = 6;
  ...
}

class Electron {
  public static readonly RADIUS = 5;
  ...
}

So when you see RADIUS in source code, how will you know which radius if refers to? If you used Proton.RADIUS and Electron.RADIUS, it's obvious. If you exported constants that are outside the class definition, you'll need to inspect the imports. You could easily introduce an error by adding the wrong import. And you'll have a namespace collision if you need to use both radii in the same file.

[marlitas]

Thanks for the reply @pixelzoom I will move many to be static constants then. How do you feel about static constants vs. constants that live in NumberPairsConstants? Is there a best practices there?

[pixelzoom]

That's a good question. I tend to put things in {REPO}Constants that are shared by one or more source files, and don't have a clear "owner" class. Shared options values (e.g. MOTHAConstants.ACCORDION_BOX_OPTIONS) are the most obvious example.

[marlitas]

I refactored how constants are organized. most export const were moved to be static members of a class. I think this is ready to close. @pixelzoom feel free to reopen if you want to do some followup before calling it done.