phetsims / pendulum-lab

"Pendulum Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 10 forks source link

Avoid assignment in Node constructor calls #61

Closed aaronsamuel137 closed 9 years ago

aaronsamuel137 commented 9 years ago

During code review #56, I noticed this code in GravityWithSliderListNode:

new Node( {
            children: [
              new Rectangle( 0, 0, PendulumLabConstants.TRACK_SIZE.width - 2 * arrowButtonMinus.width - 2 * VALUE_LABEL_SPACING, arrowButtonMinus.height, 3, 3, {
                centerY: -1,
                centerX: 0,
                fill: '#FFF',
                stroke: 'black',
                lineWidth: 1
              } ),
              valueLabel = new SubSupText( StringUtils.format( pattern_0gravityValue_gravityUnitsMetric, Util.toFixed( gravityProperty.value, PendulumLabConstants.TWEAKERS_PRECISION ) ), {
                centerX: 0,
                centerY: -1,
                font: FONT
              } )
            ]
          } ),

In this code, the new SubSupText is being assigned to valueLabel at the same time as it is added into the children array. My recommendation is to avoid this pattern in PhET code as it is harder to read and maintain.

If there are other places in the code that use this pattern, it would be good to change those as well. Thanks.

andrey-zelenkov commented 9 years ago

Avoided assignment in Node constructor calls. Reassign to @aaronsamuel137 for verification.

aaronsamuel137 commented 9 years ago

This looks much better to me, thanks! Closing