createPropertySet() pattern in ParametricSpringNode

[jessegreenberg]

createPropertySet is used as a static function to initialize all properties for the ParametricSpringNode. This pattern is not used in any other scenery-phet object and requires the user to call

ParametricSpringNode.createPropertySet( options ) 

every time a ParametricSpringNode is instantiated. Why is createPropertySet() used instead of passing properties into the main ParametricSpringNode constructor in an options block?

[pixelzoom]

This pattern is bit of an experiment, trying to address a situation that we run into frequently in common code.

ParametricSpringNode is generalized for migration to scenery-phet, and will be used in other sims. It has many Properties. Sims may want to set the default values of many of these Properties, but may only want to observe a few of them (i.e., only a few things are dynamic). For example, hookes-law configures 7 of the 8 options, only observers 3 of them dynamically.

This pattern was chosen at after discussions with other developers (most recently @jonathanolson.) It allows you to specify default values for the Properties without having to create Properties. For example, hookes-law sets the 'loops' property (number of loops in the coil), but its not something that ever changes, so creating a Property is an odd requirement. Other alternatives that were discussed were: requiring the client to create Properties for options of interest; overloading options to take either a default value or property.

I'll assign this to @jbphet for his opinion, since he may have a fresh perspective.

[jbphet]

I find this approach a little awkward. My understanding is that the factory function is used to create the PropertySet so that clients don't have to create properties for things that don't need to change dynamically for their particular use case. So why not just create this same PropertySet in the constructor and let the user specify the desired default configuration using fixed values in the options argument, and reach into the PropertySet to link as needed? I think that the answer to this question is that in some cases some of the properties that the client will provide to this node are model properties, and the client would like to pass them in directly instead of linking them to the properties in the PropertySet created through createPropertySet. In this case, they would be assembling their own property set, so they'd have to do exactly what I believe you're trying to avoid, i.e. create properties for non-dynamic attributes of the spring. Or maybe they would replace properties in the property set with the ones that they waned to hook to the model.

I would favor the alternative (mentioned in the previous comment) where the options could take a property or fixed value. I believe we've used this pattern before, though I couldn't find any examples in the code base. This, IMO, would be more intuitive from the perspective of someone who was setting out to use this node in a new sim. I realize that this would make the code in the constructor of ParametricSpringNode a bit more complicated, but I think the tradeoff would be worth it.

Having said this, I was able to figure out what the intent was (assuming I've got it right), and any user would likely search for examples in the code base, so they'd be able to figure it out too. It might take a little extra time, but they would get there. In other words, I could live with it if @pixelzoom really likes it this way. Maybe it'll grow on me.

[pixelzoom]

I would favor the alternative (mentioned in the previous comment) where the options could take a property or fixed value. I believe we've used this pattern before, though I couldn't find any examples in the code base.

We've discussed it before (most recently @jonathanolson and me) and have decided each time that it was awkward and probably not a good idea to overload options in this way.

[pixelzoom]

I could live with it if @pixelzoom really likes it this way.

I tried various approaches and this seemed to be the lesser of evils, not something that I like.

[samreid]

Did you consider promoting the propertySet defaults to the main options instance and creating the property set from within the ParametricSpringNode constructor? For instance, then the ParametricSpringNode constructor arguments would change from (propertySet, options) to (options).

[pixelzoom]

Did you consider ...

Yes. I'll probably revert to that since it's more familiar.

[jbphet]

Sounds good to me.

[pixelzoom]

@jessegreenberg @jbphet @samreid and anyone else who is interested... PropertySet is now created in the constructor based on default values specified in options. Run with "exp" query parameter to enable the "Experimental" screen test harness.

The PropertySet is currently an instance field, i.e.:

this.propertySet = new PropertySet( { 
  loops: options.loops,
  radius: 10,
  .... 
} );

So accessing an individual Property looks like (e.g.) springNode.propertySet.loopsProperty or springNode.propertySet.property( 'loops' ).

If you prefer the syntax to be more succinct (e.g. springNode.loopsProperty) then the PropertySet could be replaced with explicit creating of individual properties, i.e.:

this.loopsProperty = new Property( options.loops );
this.radiusProperty = new Property( options.loops );
...

Preferences?

[samreid]

@jonathanolson & @samreid previously discussed an axon feature that would add a property to an instance along with its es5 get/set. It looks like this was implemented in https://github.com/phetsims/axon/issues/42 and available as Property.addProperty which looks like:

      /**
       * Set up a PropertySet-like property on any object (see https://github.com/phetsims/axon/issues/42).
       *
       * @param {Object} object - The object that the property will be placed on
       * @param {string} propertyName - Name of the property
       * @param {*} initialValue - The initial value of the property
       */
      addProperty: function( object, propertyName, initialValue ) {
        // defines the property
        var property = this[ propertyName + 'Property' ] = new axon.Property( initialValue );

        // defines ES5 getter/setter
        Object.defineProperty( this, propertyName, {
          get: function() { return property.get(); },
          set: function( value ) { property.set( value ); },

          // Make it configurable and enumerable so it's easy to override...
          configurable: true,
          enumerable: true
        } );
      }

What do you think of that as an intermediate solution?

[pixelzoom]

I see zero usages of Property.addProperty. So I'm not wild about being the guinea pig. I would probably opt for adding the 8 calls to new Property.

[samreid]

I also see 0 usages and do not want you to have to guinea pig this. @jonathanolson looks like the developer lead on this, not sure if he developed it for a specific case or not. However, having lots of xProperty without get x() and set x() could be confusing, which is why I originally suggested this strategy. Let's discuss further.

[pixelzoom]

I'm not too concerned about the lack of ES5 get/set, because I typically use the someObject.someProperty.get and someObject.someProperty.set forms - imo easier to search for, and easier to see when you're dealing with a Property. But I realized that others prefer the syntactic sugar of get/set.

[pixelzoom]

@samreid If you had a choice between:

(a) springNode.properties.loop = value; (b) springNode.loopProperty.set( value ); or springNode.loopProperty.value = value;

which would you prefer?

[samreid]

It's not just syntactic sugar, it's having a consistent API for our simulations at different levels. Imagine if you had to remember which scenery/sun/simulation classes should do:

myObject.top=7;

and which required:

myObject.topProperty.set(7);

I used top purposefully to demonstrate the convention in scenery.

Or alternatively if you wanted to set a scenery property on line 87 then a ParametricSpringNode-specific property on line 88--it would feel very unbalanced if there were radically different ways of accomplishing the same thing.

[pixelzoom]

myObject.topProperty.set(7); is in fact the common denominator of all the various ways that you can currently create a Property. Which is why I use it.

[pixelzoom]

And myObject.top=7; gives me no indication when reading code that I'm dealing with a Property assignment that will result in side effects.

[pixelzoom]

Not to belabor the point... But having one syntax (e.g. myObject.top=7) that is in fact something different depending on the type of top is in no way a consistent API. Yes the syntax is consistent in that it's the same. But the behavior is different, and the difference is hidden by the syntax. If you wanted a consistent API for dealing with Property, there would be one consistent way of setting and getting a Property's value. Currently there are several. And they are all ultimately implemented by calling Property.set and Property.get. The other forms are syntactic sugar:

In computer science, syntactic sugar is syntax within a programming language that is designed to make things easier to read or to express. It makes the language "sweeter" for humans to use: things can be expressed more clearly, more concisely, or in an alternative style that some may prefer. (https://en.wikipedia.org/wiki/Syntactic_sugar)

You like having a common syntax for assignment; that's your preference and I respect it. It's not my preference, and I have some very good reasons (rooted in defensive programming and maintainability) for my preference.

[samreid]

Perhaps good to discuss at an upcoming developer meeting, I'll add it to tomorrow's agenda. It would be good to hear from @jonathanolson why he introduced Property.addProperty.

But nit-picking notes above:

myObject.topProperty.set(7); is in fact the common denominator of all the various ways that you can currently create a Property. Which is why I use it.

In Spring.js you are using the ES5 set strategy for the applied force:

// if the displacement range was specified, maintain the displacement, change applied force
thisSpring.appliedForce = springConstant * thisSpring.displacement; // F = kx

In this case you could have used property set method like so: thisSpring.appliedForceProperty.set(springConstant*thisSpring.displacement); I am not sure why you used the syntactic sugar here... could be unintentional. To the next point:

And myObject.top=7; gives me no indication when reading code that I'm dealing with a Property assignment that will result in side effects.

You also use side-effect setters when you call scenery es5 setters like this (in XYPointPlot.js):

xValueNode.centerX = xView; // centered on the tick
xValueNode.top = 12; // below the x axis

In cases like those you could have used xValueNode.setCenterX(xView);, for example to cue the reader that many side effects are happening. I am not advocating that you do so, however.

Regarding syntactic sugar: the term is apt but the sugar is in the intention of providing an API that shields the maintainer from having to know or understand the internals of the implementation. If the developer wants to set the height to 7, then I think the best way is to provide obj.height=7 or obj.setHeight(7) rather than obj.somethingInternal.someUnexpectedMethod.makeIt(7). I guess the way that I am defensive about this is that I never assume that a JS property setter has no side effects.

[pixelzoom]

In Spring.js you are using the ES5 set strategy for the applied force:

I was experimenting with how I felt about using the format this.appliedForce = ... where this is the PropertySet (in this case Spring constructor) that creates appliedForceProperty. A problem I've run into repeatedly with using thisSpring.appliedForce = ... is when the method of creating appliedForceProperty changes and call sites then (silently) break. For example, mySpring.appliedForce = ... works only because appliedForceProperty is create via a PropertySet. If the implementation is changed such that appliedForceProperty is create using new Property, then the semantics of mySpring.appliedForce = ... is totally different and the assignment is broken.

And yes, you will find this pattern used in some of my previous code, where I was still forming an opinion about the tradeoffs of using the various forms of the Property API.

[pixelzoom]

I also find it inconsistent that I can do assignment like this:

spring.loops = 5;

...but can't use the same syntax for other operations. Eg, this doesn't work:

spring.loops.link( ... );

It must be done using one of these forms:

spring.loopsProperty.link( ... );
spring.property( 'loops' ).link( ... );

[samreid]

For example, mySpring.appliedForce = ... works only because appliedForceProperty is create via a PropertySet.

Right, this wouldn't work if you created it with new Property but I think this is what Property.addProperty was meant to solve.

Eg, this doesn't work: spring.loops.link( ... );

Are you suggesting it would be more consistent to make calls like this?

spring.loopsProperty.value
spring.loopsProperty.link(...);

So that everything goes through the loopsProperty?

There is actually an alternative to the 2 options you mentioned above (declared in PropertySet.js), but it still inconsistent with the es5 get/set:

spring.link('loops',function(){...});

Just brainstorming, we could always redesign axon to autocreate APIs like this:

spring.getLoops();
spring.setLoops(...);
spring.linkLoops(function(){...});

I brainstormed it because (a) it uses a uniform style for calls (b) since it uses methods, it is clear that there may be side effects.

One reason I prefer es5 get/set is because I find it easier to read:

ball.momentum = ball.mass * ball.velocity;

instead of:

ball.setMomentum(ball.getMass() * ball.getVelocity());

For me, the former is kind of like "factoring out" the set()(get()get()). I know that for such a small example both versions are extremely simple and easy to read but when things get more complex the former seems advantageous. But thought I'd jot down these ideas for discussion.

[samreid]

At today's meeting after @pixelzoom was gone, @jonathanolson pointed out that he is investigating a pattern like the one brainstormed above in Text.js to reduce boilerplate but to leave get/set methods and es5 get/set. See Text.js, copied here for discussion:

  function addFontForwarding( propertyName, fullCapitalized, shortUncapitalized ) {
    var getterName = 'get' + fullCapitalized;
    var setterName = 'set' + fullCapitalized;

    Text.prototype[ getterName ] = function() {
      // use the ES5 getter to retrieve the property. probably somewhat slow.
      return this._font[ shortUncapitalized ];
    };

    Text.prototype[ setterName ] = function( value ) {
      // create a full copy of our font instance
      var ob = {};
      ob[ shortUncapitalized ] = value;
      var newFont = this._font.copy( ob );

      // apply the new Font. this should call invalidateText() as normal
      // TODO: do more specific font dirty flags in the future, for how SVG does things
      this.setFont( newFont );
      return this;
    };

    Object.defineProperty( Text.prototype, propertyName, {
      set: Text.prototype[ setterName ],
      get: Text.prototype[ getterName ]
    } );
  }

[jonathanolson]

Also a somewhat similar pattern in Kite:

  /**
   * Adds getter/setter function pairs and ES5 pairs, e.g. addInvalidatingGetterSetter( Arc, 'radius' ) would add:
   * - segment.getRadius()
   * - segment.setRadius( value )
   * - segment.radius // getter and setter
   *
   * It assumes the following is the internal name: '_' + name
   *
   * @param {Function} type - Should be the constructor of the type. We will modify its prototype
   * @param {string} name - Name of the
   */
  Segment.addInvalidatingGetterSetter = function( type, name ) {
    var internalName = '_' + name;
    var capitalizedName = name.charAt( 0 ).toUpperCase() + name.slice( 1 );
    var getterName = 'get' + capitalizedName;
    var setterName = 'set' + capitalizedName;

    // e.g. getRadius()
    type.prototype[ getterName ] = function() {
      return this[ internalName ];
    };

    // e.g. setRadius( value )
    type.prototype[ setterName ] = function( value ) {
      if ( this[ internalName ] !== value ) {
        this[ internalName ] = value;
        this.invalidate();
      }
      return this; // allow chaining
    };

    Object.defineProperty( type.prototype, name, {
      set: type.prototype[ setterName ],
      get: type.prototype[ getterName ]
    } );
  };

[samreid]

How long has this been in production in Kite? Are there any other disadvantages other than making it difficult to search for getX setX methods (where X is a camelcased string)?

[jonathanolson]

The Kite example is just as of a few days ago.

The main disadvantages are searchability, and potentially a minor performance hit depending on optimization (using this[ name ] instead of this.start in the closure may have a more indirect lookup, and the closure might have its own overhead).

[pixelzoom]

The main disadvantages are search ability

Given that refactoring JavaScript involved primarily search-&-replace, I consider this to be a huge disadvantage.

[pixelzoom]

Proposal:

One of these options:

(A) • Replace the PropertySet in ParametricSpringNode constructor with individual axon.Property instances. • Use springNode.loopsProperty syntax in client code (as in hookes-law, currently the only client) • Revisit in the future if someone finds they can't live without springNode.loops syntax.

(B) • Leave as is, with support for springNode.propertySet.loopsProperty and springNode.propertySet.loops

[pixelzoom]

... or leave as is, with support for springNode.propertySet.loopsProperty and springNode.propertySet.loops.

[pixelzoom]

@jbphet since you reviewed this and found the pattern awkward, it would be nice if you could weigh in on the alternatives proposed,

[jonathanolson]

Given that refactoring JavaScript involved primarily search-&-replace, I consider this to be a huge disadvantage.

For my purposes, it seems much easier to make modifications in one place, instead of the more error-prone "search for all the places that need to be changed, and hope none are missed".

It would also be possible to still have the 4-methods-per-property boilerplate for searchability, but have those forward to a shared implementation.

If the shortcut is used 20 times, it could cut down 300 lines of code (which is also helpful for keeping file sizes down).

[pixelzoom]

If you want to add a utility function for addInvalidatingGetterSetter, go for it. But I'm not going to be the guinea pig here.

Feedback on proposal (https://github.com/phetsims/hookes-law/issues/9#issuecomment-120507909) please?

[samreid]

An alternative to https://github.com/phetsims/hookes-law/issues/9#issuecomment-120508151, but instead of propertySet, call it model or springModel, which would support:

// this
springNode.model.loops
// and
springNode.model.loopsProperty.get()

[jonathanolson]

What about using PropertySet.addProperty() for this? E.g.:

PropertySet.addProperty( this, 'loops', options.loops );

Also I recall discussing supporting being able to pass a direct value OR a property, so is options.loops sometimes a Property itself?

[samreid]

We're going to need more guinea pigs, stat. :hamster: :hamster: :hamster:

[pixelzoom]

@jonathanolson asked:

What about using PropertySet.addProperty() for this?

How does that avoid having to use the springNode.propertySet.loopsProperty syntax? The point is that we want to avoid having springNode.propertySet and access properties directly via springNode.XXX, where XXX is one of the many ways to access a Property.

[samreid]

How does that avoid having to use the springNode.propertySet.loopsProperty syntax?

Property.addProperty would make the loops a property of the springNode. Then you could do:

springNode.loops = springNode.loops+1;
springNode.loopsProperty.link(function(){...});

[pixelzoom]

7/14/15 dev meeting notes:

Best option: Be the guinea pig for Property.addProperty (spend 30 minutes or so)

Fallback: Rename this.propertySet to this.model

[pixelzoom]

I tried the Property.addProperty approach. After 1 bug fix (https://github.com/phetsims/axon/commit/579f8c660b519d9381e4e85833702eb7d370a733) it seemed to work ok. But a major downside is that we lose support for resetting a set of Properties.

With PropertySet, ParametricSpringNode.reset looks like this:

function reset() {
  this.propertySet.reset();
}

With Property.addProperty, it becomes:

function reset() {
  this.loopsProperty.reset();
  this.radiusProperty.reset();
  this.aspectRatioProperty.reset();
  this.pointsPerLoopProperty.reset();
  this.lineWidthProperty.reset();
  this.phaseProperty.reset();
  this.deltaPhaseProperty.reset();
  this.xScaleProperty.reset();
}

(a) Yuck.

(b) This presents a maintenance issue. If additional calls to Property.addProperty are added in the constructor, then the corresponding Properties must be added to reset.

So I'm going to go with the "fallback plan", which is to rename ParametricSpringNode.propertySet to model.

[pixelzoom]

Fallback plan implemented. Closing.