Eliminate GamePhaseProperty

[samreid]

Discovered in phetsims/axon#213, BaseGameModel has this inner class:

  /**
   * Property used for the game phase.
   * It has a 'hook' function that is called before the value is changed.
   * This is useful for setting the various state parameters of the game before
   * notifying observes that the game phase has changed.
   * TODO: Shouldn't this be accomplished by adding the first listener to a Property?
   * @param {GamePhase} value
   * @param {function} hook function with one parameter of type {GamePhase}
   * @constructor
   */
  class GamePhaseProperty extends Property {
    constructor( value, hook ) {
      super( value );
      this.hook = hook; // @private
    }

    /**
     * @param value
     * @public
     * @override
     */
    set( value ) {
      this.hook( value );
      super.set( value );
    }
  }

  graphingLines.register( 'BaseGameModel.GamePhaseProperty', GamePhaseProperty );

  return BaseGameModel;

It seems the whole purpose of this class is making sure one listener is called before the other listeners are called. Axon Property and Emitter guarantee that listeners will be invoked in the order they are added and there is no API for changing the ordering of listeners. Therefore, a better solution would be to link or lazyLink a listener directly to the this.gamePhaseProperty and create it as a Property rather than introducing a new subtype for this.

[pixelzoom]

@samreid said:

Axon Property and Emitter guarantee that listeners will be invoked in the order they are added

Looking at both Property and Emitter, searching for "order', and reading the docs for addListener... There is currently nothing in the public contract that states that. If you are basing that statement on the fact that the implementation currently uses an array and iterates in the order that listeners were added, that is not a "guarantee" and it's certainly not something that clients should rely on. If you want to add that to the public contract (via documentation), I support that.

[samreid]

I added documentation to clarify that listeners are called in the order they were added.

[pixelzoom]

It seems the whole purpose of this class is making sure one listener is called before the other listeners are called.

This has nothing to do with the order that gamePhaseProperty listeners are called. The issue is that a change to the game phase requires some setup work that must be done before any observers are notified.

I am inclined to close this as "won't fix". I don't really see the point in changing this because it works, and it's a totally valid approach.

@samreid please comment.

[pixelzoom]

I discovered that this TODO was added (with no issue number) and is apparently related to this issue:

TODO: Shouldn't this be accomplished by adding the first listener to a Property?

Yes, I could accomplish this same thing by registering a first listener to initialize things for the game state before calling other listeners. But that's a more fragile approach, and I don't see the point in introducing an order dependency (and potential maintenance issue) where there currently is no problem. So I'm not going to change this.

Closing.

[samreid]

Can we discuss further and possibly get a second opinion? I'm wondering if I'm the only developer who would come across that code and think it should be changed.

If I understand correctly, this patch would have the same behavior, but with 28 fewer lines of code and no need for a newly introduced class.

Index: js/linegame/model/BaseGameModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/linegame/model/BaseGameModel.js	(revision 48e6effb4e9c0f5814e10cf5a9e8e6d2b2be0fba)
+++ js/linegame/model/BaseGameModel.js	(date 1547960875000)
@@ -82,11 +82,11 @@
     }
 
     // @public
-    this.gamePhaseProperty = new GamePhaseProperty( GamePhase.SETTINGS,
-      /*
-       * This function will be called prior to setting the Property value.
-       * Updates fields so that they are accurate before Property listeners are notified.
-       */
+    this.gamePhaseProperty = new Property( GamePhase.SETTINGS );
+
+    // This function will be called prior to setting the Property value. Updates fields so that they are accurate before
+    // Property listeners are notified.
+    this.gamePhaseProperty.link(
       function( gamePhase ) {
         if ( gamePhase === GamePhase.SETTINGS ) {
           self.playStateProperty.set( PlayState.NONE );
@@ -279,33 +279,5 @@
     }
   } );
 
-  /**
-   * Property used for the game phase.
-   * It has a 'hook' function that is called before the value is changed.
-   * This is useful for setting the various state parameters of the game before
-   * notifying observes that the game phase has changed.
-   * @param {GamePhase} value
-   * @param {function} hook function with one parameter of type {GamePhase}
-   * @constructor
-   */
-  class GamePhaseProperty extends Property {
-    constructor( value, hook ) {
-      super( value );
-      this.hook = hook; // @private
-    }
-
-    /**
-     * @param value
-     * @public
-     * @override
-     */
-    set( value ) {
-      this.hook( value );
-      super.set( value );
-    }
-  }
-
-  graphingLines.register( 'BaseGameModel.GamePhaseProperty', GamePhaseProperty );
-
   return BaseGameModel;
 } );
\ No newline at end of file

there currently is no problem

This simulation will one day be instrumented for accessibility and PhET-iO and it seems valuable to make sure it is in a streamlined state. Introducing a new Property subclass for something that is already supported by axon feels like an anti-pattern.

I don't see the point in introducing an order dependency (and potential maintenance issue)

Taking this to its logical conclusion, are you suggesting that any time we need to enforce a particular order of notifications, we would introduce a subclass that hard-codes the required order?

there currently is no problem

GamePhaseProperty would require maintenance. For instance, at the moment it does not match the signature of the method it overrides and IDEA highlights it as such:

Is your primary concern is that one day AXON will change the order of notifications? Or that another listener will "sneak" in before this one? If the former, would a qunit test that checks for callback order help? If the latter, would an assertion or graphing-lines qunit test help?

[pixelzoom]

Can we discuss further and possibly get a second opinion? I'm wondering if I'm the only developer who would come across that code and think it should be changed.

That's fine with me. I'd also like to understand why you think this is important.

If I understand correctly, this patch would have the same behavior, but with 28 fewer lines of code and no need for a newly introduced class.

You have introduced an order dependency (a code smell) that I prefer not to have in my code. Please explain why having an order dependency is preferable to my solution that has no ordering dependency.

This simulation will one day be instrumented for accessibility and PhET-iO and it seems valuable to make sure it is in a streamlined state. Introducing a new Property subclass for something that is already supported by axon feels like an anti-pattern.

For subclassing Property in general... If you're inferring that we should have no sim-specific subclasses of Property to "streamline" PhET-iO, I strongly disagree. PhET-iO should be able to easily accommodate subclasses of Property.

For GamePhaseProperty specifically... I doesn't see why overriding set requires GamePhaseProperty to have any special instrumentation, any more than overriding Node setVisible would require any special instrumentation. Please describe what your specific concern is with instrumenting GamePhaseProperty, and what PhET-iO-specific problem overriding set causes.

Taking this to its logical conclusion, are you suggesting that any time we need to enforce a particular order of notifications, we would introduce a subclass that hard-codes the required order?

No. I'm saying (1) don't write code that has order dependencies, and (2) don't change code that doesn't have a problem. In this case, it made sense to encapsulate the "setup" work for a game-state change in GameStateProperty (and this in fact may have been ported directly from Java). I don't see any advantage (PhET-iO or otherwise) to changing it.

GamePhaseProperty would require maintenance. For instance, at the moment it does not match the signature of the method it overrides and IDEA highlights it as such: Incompatible override, should have type Property

Thanks for pointing that out. I should have been returning the Property, and it's fixed in the above commit. Good to address this, but I don't see how it invalidates the approach of subclassing Property or would have affected PhET-iO operation.

Is your primary concern is that one day AXON will change the order of notifications?

No, that's not my primary concern. My primary concern is writing code with order dependency, the real anti-pattern here, which is what you're promoting.

For this specific case... Even if AXON's order of notification never changes, there is no support for guaranteeing that the "first listener" in your patch is (and remains) first. Future changes/maintenance could add an earlier listener, breaking the order dependency.

In general... The Observer pattern does not specify order of notification. java.util.Observable in fact specifies that "the order in which notifications will be delivered is unspecified". And writing code that relies on order is generally a bad practice. Order of notifications for Property and Emitter was unspecified until you decided otherwise in phetsims/axon@1239bb6. Against my better judgement, I gave my blessing to that, but it didn't receive general discussion. So up until this point, I have been writing code based on the Observer pattern, and have not been relying on order of notification. Regardless of what you've decide for Property and Emitter, I plan to continue that practice.

[samreid]

I'd also like to understand why you think this is important.

It seems nice to eliminate 28 lines of code. Less to maintain, understand, instrument, etc. Less places for bugs to creep in.

You have introduced an order dependency (a code smell) that I prefer not to have in my code.

Should we go through all the rest of PhET's code and identify and eliminate order dependencies? Should we add code review checklist entries for it? Do you think other PhET code has buggy order dependencies?

Please describe what your specific concern is with instrumenting GamePhaseProperty, and what PhET-iO-specific problem overriding set causes.

It seems nice to eliminate 28 lines of code. Less to maintain, understand, instrument, etc. Less places for bugs to creep in.

don't change code that doesn't have a problem.

The code runs correctly, that is not the problem. The problem is that it has 28 extra lines of code to manage an order dependency. That's a maintenance and understandability problem.

I don't see any advantage (PhET-iO or otherwise) to changing it.

You don't see any advantage to removing 28 lines of code?

Good to address this, but I don't see how it invalidates the approach of subclassing Property

The point is that there is extra code that needs to be read, understood and maintained.

My primary concern is writing code with order dependency, the real anti-pattern here, which is what you're promoting.

If it is sometimes important to manage order of callbacks, perhaps AXON should support this directly instead of managing a one-off solution in a simulation?

Even if AXON's order of notification never changes, there is no support for guaranteeing that the "first listener" in your patch is (and remains) first.

In #109 (comment) I proposed using assertions for guaranteeing that the listener remains first. Do you think that wouldn't work for some reason?

[pixelzoom]

Should we go through all the rest of PhET's code and identify and eliminate order dependencies? Should we add code review checklist entries for it? Do you think other PhET code has buggy order dependencies?

I would guess that there are order dependencies in PhET code, and they may or may not have manifested as bugs. I'm not interested in spending my time proactively identifying them.

Avoiding ordering dependencies is a best practice that I thought every programmer was familiar with. If you think it would be valuable to explicitly state that in the code-review checklist, feel free to add it.

You don't see any advantage to removing 28 lines of code?

I don't understand why you think that introducing an ordering dependency (a cardinal sin of programming where I come from) is acceptable in order to eliminate a mere 28 lines of working code that is trivial to understand, well documented, has no known problems, and no ordering dependency. But since you said "28 lines of code" 4 times in the above comment, I guess you think that's more important.

In #109 (comment) I proposed using assertions for guaranteeing that the listener remains first. Do you think that wouldn't work for some reason?

You proposed a qunit test or assertion. Both of those add complication (and lines of code, reducing your 28-lines savings), and are imo inferior to the current solution.

[samreid]

If it is sometimes important to manage order of callbacks, perhaps AXON should support this directly instead of managing a one-off solution in a simulation?

[pixelzoom]

That sounds like undesirable complication in AXON, it may encourage using expedient-but-inferior solutions, and I think it's preferable to find solutions that don't require order dependencies. But if you want to write code that requires order dependencies, then you'll probably need something in AXON to make that robust.

[samreid]

Graphing Lines apparently requires that the hook is called before listeners are notified. Hence a new type GamePhaseProperty was introduced in order to manage this ordering requirement. But hook has exactly the same signature as every other listener. So the essence of GamePhaseProperty is that it has one callback hook that is given preferential treatment. Why not generalize GamePhaseProperty and move it to common code so that other repos can leverage that same strategy? Or do you think this pattern of enforcing a specific order will be rare and hence should only be codified in Graphing Lines?

What would go wrong if hook was called after listeners were notified? Would the values in the RewardNode be wrong?

[pixelzoom]

Why not generalize GamePhaseProperty and move it to common code so that other repos can leverage that same strategy?

I would wait until there is a general need. One instance is hardly general, and it seems like we're spending a strange amount of time on this, given all of the other more high-priority tasks that are not completed.

What would go wrong if hook was called after listeners were notified? Would the values in the RewardNode be wrong?

I may be wrong... But I seriously doubt that my past self would have done this if there hadn't been a legitimate need to do setup before gamePhaseProperty changes. And I don't have the luxury of spending time to investigate why working code is working.

[pixelzoom]

I see at least one error that will be introduced if hook isn't called before notifying listeners. The score will not have been updated and the clock will still be running. So ResultsNode will display the wrong score, time, and best time.

[pixelzoom]

Another way to eliminate GamePhaseProperty would be:

// @public (read-only) set this via setGamePhase
this.gamePhaseProperty = new Property( GamePhase.SETTINGS );

/**
 * Sets the game phase. Call this instead of setting gamePhaseProperty directly, 
 * because there is setup that needs to be done before listeners are notified.
 * @param {GamePhase} gamePhase
 */
setGamePhase: function( gamePhase ) {
  // do the setup that's currently done in hook, then...
  this.gamePhaseProperty.set( gamePhase );
}

... and change all occurrences of model.gamePhaseProperty.set to model.setGamePhase. But again, I don't see the point in making such a change, and I have yet to hear why this really would have any impact other than reducing lines of code.

[samreid]

The proposal in the preceding comment seems preferable to me. Extending Property to add something that is like a listener but not a listener seems odd. It also had odd semantics that hook is called before any of the other logic in Property.set is called, including validation, isDeferred and regardless of whether equalsValue is going to pass.

Some possible ways to proceed:

1. Close this issue. Reasoning: Even though I still think the code could be improved by using Axon's existing built-in facility for calling listeners in the specified order or the proposal in Eliminate GamePhaseProperty #109 (comment) , I think we understand each other's points. Furthermore, others that come across GamePhaseProperty in the future and ask the same question may benefit from reading this conversation.
2. Invite another developer such as @jbphet or @jonathanolson to review and discuss. This may not be optimal at the moment since everyone is really busy.
3. Leave the issue open and deferred until the sim is instrumented for PhET-iO or a11y. At that point, another developer could chime in or we could discuss further.
4. Bring up for discussion at developer meeting. I recall discussions where we have previously talked about not depending on the order of notifications in the listener pattern, so may not warrant discussing again.
5. Write up more notes in code review about observer pattern and dependency order.
6. Apply the patch from Eliminate GamePhaseProperty #109 (comment) that eliminates the Property subclass, add hook as the 1st listener note that it depends on axon invoking listeners in the given order.
7. Apply the proposed fix from the preceding comment.

To me, 7 makes the most sense as a compromise that reaches many of our shared objectives. I'm also happy with 6 or 3.

[pixelzoom]

Here's the patch for the solution I described in the previous commit. It seems to work, but it will take me at least 30 minutes to properly regression test. That would make close to 2 hours that I've spent on what is imo a non-issue.

Index: js/linegame/view/SettingsNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/linegame/view/SettingsNode.js	(revision a5965d3dfcb1b91e086a8f0841a7be924db5376f)
+++ js/linegame/view/SettingsNode.js	(date 1548045877000)
@@ -149,7 +149,7 @@
       },
       listener: function() {
         model.levelProperty.set( level );
-        model.gamePhaseProperty.set( GamePhase.PLAY );
+        model.setGamePhase( GamePhase.PLAY );
       }
     } );
   };
Index: js/linegame/model/BaseGameModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/linegame/model/BaseGameModel.js	(revision a5965d3dfcb1b91e086a8f0841a7be924db5376f)
+++ js/linegame/model/BaseGameModel.js	(date 1548046133000)
@@ -31,6 +31,7 @@
   var StringProperty = require( 'AXON/StringProperty' );
 
   // constants
+  var INITIAL_GAME_PHASE = GamePhase.SETTINGS;
   var CHALLENGES_PER_GAME = 6;
   var DUMMY_CHALLENGE = new GraphTheLine( '', Line.createSlopeIntercept( 1, 1, 1 ),
     EquationForm.SLOPE_INTERCEPT, ManipulationMode.SLOPE, GLConstants.X_AXIS_RANGE, GLConstants.Y_AXIS_RANGE );
@@ -81,31 +82,8 @@
       this.bestTimeProperties.push( new Property( null ) ); // null if a level has no best time yet
     }
 
-    // @public
-    this.gamePhaseProperty = new GamePhaseProperty( GamePhase.SETTINGS,
-      /*
-       * This function will be called prior to setting the Property value.
-       * Updates fields so that they are accurate before Property listeners are notified.
-       */
-      function( gamePhase ) {
-        if ( gamePhase === GamePhase.SETTINGS ) {
-          self.playStateProperty.set( PlayState.NONE );
-          self.timer.stop();
-        }
-        else if ( gamePhase === GamePhase.PLAY ) {
-          self.initChallenges();
-          self.playStateProperty.set( PlayState.FIRST_CHECK );
-          self.scoreProperty.set( 0 );
-          self.timer.start();
-        }
-        else if ( gamePhase === GamePhase.RESULTS ) {
-          self.playStateProperty.set( PlayState.NONE );
-          self.updateBestTime();
-        }
-        else {
-          throw new Error( 'unsupported game phase: ' + gamePhase );
-        }
-      } );
+    // @public (read-only) {GamePhase} set this using setGamePhase
+    this.gamePhaseProperty = new Property( INITIAL_GAME_PHASE );
 
     this.initChallenges();
 
@@ -128,7 +106,7 @@
 
         if ( isLastChallenge ) {
           // game has been completed
-          self.gamePhaseProperty.set( GamePhase.RESULTS );
+          self.setGamePhase( GamePhase.RESULTS );
           if ( score > self.bestScoreProperties[ level ].get() ) {
             self.bestScoreProperties[ level ].set( score );
           }
@@ -152,7 +130,40 @@
 
   graphingLines.register( 'BaseGameModel', BaseGameModel );
 
-  inherit( Object, BaseGameModel, {
+  return inherit( Object, BaseGameModel, {
+
+    /**
+     * Sets the game phase. Call this instead of setting gamePhaseProperty directly,
+     * because there are tasks that needs to be done before listeners are notified.
+     * @param {GamePhase} gamePhase
+     * @public
+     */
+    setGamePhase: function( gamePhase ) {
+      if ( gamePhase !== this.gamePhaseProperty.get() ) {
+
+        // Do tasks that need to be done before notifying listeners.
+        if ( gamePhase === GamePhase.SETTINGS ) {
+          this.playStateProperty.set( PlayState.NONE );
+          this.timer.stop();
+        }
+        else if ( gamePhase === GamePhase.PLAY ) {
+          this.initChallenges();
+          this.playStateProperty.set( PlayState.FIRST_CHECK );
+          this.scoreProperty.set( 0 );
+          this.timer.start();
+        }
+        else if ( gamePhase === GamePhase.RESULTS ) {
+          this.playStateProperty.set( PlayState.NONE );
+          this.updateBestTime();
+        }
+        else {
+          throw new Error( 'unsupported game phase: ' + gamePhase );
+        }
+
+        // Change the Property, which notifies listeners
+        this.gamePhaseProperty.set( gamePhase );
+      }
+    },
 
     // @override @public
     reset: function() {
@@ -166,7 +177,7 @@
       this.challengesPerGameProperty.reset();
       this.playStateProperty.reset();
 
-      this.gamePhaseProperty.reset();
+      this.setGamePhase( INITIAL_GAME_PHASE );
       this.resetBestScores();
       this.resetBestTimes();
 
@@ -278,35 +289,4 @@
       console.log( 'end: verify creation of challenges' );
     }
   } );
-
-  /**
-   * Property used for the game phase.
-   * It has a 'hook' function that is called before the value is changed.
-   * This is useful for setting the various state parameters of the game before
-   * notifying observes that the game phase has changed.
-   * @param {GamePhase} value
-   * @param {function} hook function with one parameter of type {GamePhase}
-   * @constructor
-   */
-  class GamePhaseProperty extends Property {
-    constructor( value, hook ) {
-      super( value );
-      this.hook = hook; // @private
-    }
-
-    /**
-     * @param value
-     * @returns {Property}
-     * @public
-     * @override
-     */
-    set( value ) {
-      this.hook( value );
-      return super.set( value );
-    }
-  }
-
-  graphingLines.register( 'BaseGameModel.GamePhaseProperty', GamePhaseProperty );
-
-  return BaseGameModel;
 } );
\ No newline at end of file
Index: js/linegame/view/ResultsNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/linegame/view/ResultsNode.js	(revision a5965d3dfcb1b91e086a8f0841a7be924db5376f)
+++ js/linegame/view/ResultsNode.js	(date 1548045877000)
@@ -64,7 +64,7 @@
           model.bestTimeProperties[ model.levelProperty.get() ].get(),
           model.isNewBestTime,
           function() {
-            model.gamePhaseProperty.set( GamePhase.SETTINGS );
+            model.setGamePhase( GamePhase.SETTINGS );
           }, {
             starDiameter: 45,
             buttonFill: LineGameConstants.BUTTON_COLOR,
Index: js/linegame/view/PlayNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/linegame/view/PlayNode.js	(revision a5965d3dfcb1b91e086a8f0841a7be924db5376f)
+++ js/linegame/view/PlayNode.js	(date 1548045877000)
@@ -52,7 +52,7 @@
         xMargin: 10,
         yMargin: 5,
         listener: function() {
-          model.gamePhaseProperty.set( GamePhase.SETTINGS );
+          model.setGamePhase( GamePhase.SETTINGS );
         }
       }
     } );

[pixelzoom]

I'm going to apply the above patch and test.

[pixelzoom]

Patch applied and tested. 1.3.0 will be published in the near future (see #107), and this change will require a full regression test of the game by QA. So labeling this issue as "fixed-pending-testing", and please DO NOT close this issue until QA has verified.

@samreid anything else to do here?

[pixelzoom]

@samreid said:

Extending Property to add something that is like a listener but not a listener seems odd.

I couldn't disagree more. This is the reason that methods are overridden and chained to the superclass - to extend a method's tasks with things that are done before and/or after the superclass' tasks. It is a core pattern in OO programming. And this is the pattern that was used in GamePhaseProperty.

[samreid]

@samreid anything else to do here?

I think this issue can be closed after QA has verified it.

UPDATE: Please also see my relevant notes in phetsims/gravity-and-orbits#276 (comment)

[samreid]

Before QA tests this, should we discuss whether Property should have before and after options, like we discussed for Emitter? If Property had before, would it be leveraged for this use case?

[pixelzoom]

Yes, I could revert everything I've changed here and use Property with before option. But there was nothing wrong with my original solution, and there's nothing wrong with the compromise solution (other than the fact that I prefer my original solution). And I don't think that there's any benefit in changing this yet again, or holding up the RC process.

@ariel-phet your opinion?

[samreid]

Yes, I could revert everything I've changed here and use Property with before option.

I believe we have thus far only discussed and implemented before for Emitter.

But there was nothing wrong with my original solution, and there's nothing wrong with the compromise solution (other than the fact that I prefer my original solution).

I'm fine with any of the proposed solutions, including the original solution. The different solutions have minor pros and cons but they don't seem too important. Up to you or @ariel-phet how to proceed.

[ariel-phet]

I see no clear benefit to making any more changes. Let's continue with the RC as is.

[pixelzoom]

Agreed. QA, please proceed.

And if it's not clear what needs to be done to verify this issue... Please give the entire game screen, in both Graphing Lines and Graphing Slope-Intercept, a thorough test on 1 platform, and spot check on all platforms. There were no platform-specific changes here, so any collateral damage would be universal.

[KatieWoe]

Haven't seen any problems here in either RC. Closing