Use layer pattern instead of insertChild

[aaronsamuel137]

During code review #56 I noticed this in ProtractorNode:

self.insertChild( 1, tickNodeLeft );
self.insertChild( 1, tickNodeRight );

Most PhET code uses addChild() instead. Can you either document why this is necessary or use addChild? Thanks.

[andrey-zelenkov]

Added comment for insertChild call. Reassign to @aaronsamuel137 for verification.

[aaronsamuel137]

Looks good. Closing.

[aaronsamuel137]

This pattern occurs in a few places in this sim. We discussed this at our developer meeting on 4/2/2015. We'd prefer a different pattern instead of using the pattern:

this.insertChild( 3, energyGraphNode );

It would be better to create a layer for the node like so:

    this.energyGraphLayer = new Node();
    this.addChild( this.energyGraphLayer );

and then later:

    this.energyGraphLayer.addChild( this.energyGraphNode );

This way adding new nodes won't be as likely to break things in the future if the ordering of the children changes.

[andrey-zelenkov]

Added using of layer pattern instead of insertChild. Reassign to @aaronsamuel137 for verification

[aaronsamuel137]

Looking great, thanks!

There are two other places where insertChild is being used that seem off to me.

1. Could you apply the same layer pattern in ProtractorNode
2. In PendulumSlidersNode, there is a kind of confusing use of insertChild. I like that the code is generalized to handle more pendulums if needed, but it seems less maintainable and more complicated than it needs to be.

This code:

    // add necessary pendulum sliders
    pendulumLabModel.numberOfPendulumsProperty.link( function( numberOfPendulums ) {
      var numberDifference = currentNumberOfSliders - numberOfPendulums;

      // remove extra sliders
      if ( numberDifference > 0 ) {
        for ( ; numberDifference--; ) {
          content.removeChildWithIndex( pendulumSlidersNodeStorage[ currentNumberOfSliders - numberDifference - 1 ], currentNumberOfSliders - numberDifference - 1 );
          currentNumberOfSliders--;
        }
      }
      // add necessary sliders
      else if ( numberDifference < 0 ) {
        for ( ; numberDifference++; ) {
          content.insertChild( currentNumberOfSliders - numberDifference, pendulumSlidersNodeStorage[ currentNumberOfSliders - numberDifference ] );
          currentNumberOfSliders++;
        }
      }
    } );

Could be simplified to this (or something similar):

    content.addChild( pendulumSlidersNodeStorage[0] );

    // add necessary pendulum sliders
    pendulumLabModel.numberOfPendulumsProperty.link( function( numberOfPendulums ) {

      if ( numberOfPendulums === 1 && content.children.length === 2 ) {
        content.removeChildWithIndex( pendulumSlidersNodeStorage[ 1 ], 1 );
      }
      else if ( numberOfPendulums === 2 && content.children.length === 1 ) {
        content.addChild( pendulumSlidersNodeStorage[ 1 ] );
      }

    } );

Do you have any thoughts or objections about simplifying this code?

Feel free to comment on this @jonathanolson if you have another opinion.

Assigning to @andrey-zelenkov.

[andrey-zelenkov]

Fixed insertChild for protractorNode. Layer pattern used instead of insertChild for PendulumSlidersNode. I implemented simplification suggested by @aaronsamuel137 with minor fixes. Thanks! Reassign to @aaronsamuel137 for verification.

[aaronsamuel137]

Looks good to me. Thanks! Closing