phetsims / molecule-polarity

"Molecule Polarity" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 6 forks source link

Confusing extra variable #102

Closed samreid closed 3 years ago

samreid commented 3 years ago

In https://github.com/phetsims/molecule-polarity/commit/6473bfdd7b3abde27753f1cbbbb47ab702dfbbef, this code was added:

    // put a horizontal separator between each sub-panel
    const children = [ subPanels[ 0 ] ];
    let separatorCount = 0;
    for ( let i = 1; i < subPanels.length; i++ ) {
      separatorCount++;
      children.push( new HSeparator( separatorWidth, {
        tandem: options.tandem.createTandem( `separator${separatorCount}` )
      } ) );
      children.push( subPanels[ i ] );
    }

I saw this because I was referred to Molecule Polarity as an example for instrumenting separators for https://github.com/phetsims/gravity-and-orbits/issues/384.

You probably already know that separatorCount and i have the same value in the loop body (after the increments), but it was confusing to me to see them both and I had to think through whether they differed. You may wish to use something like this to stave off my future confusion:

    // put a horizontal separator between each sub-panel
    const children = [ subPanels[ 0 ] ];
    for ( let i = 1; i < subPanels.length; i++ ) {
      children.push( new HSeparator( separatorWidth, {
        tandem: options.tandem.createTandem( `separator${i}` )
      } ) );
      children.push( subPanels[ i ] );
    }
pixelzoom commented 3 years ago

Thanks, fixed in the above commit. And closing this issue.

... I was referred to Molecule Polarity as an example for instrumenting separators ...

@samreid I should point out that there is no naming convention for separator tandems. And I'm not convinced that a convention is needed. I have recently instrumented 2 different sims, for 2 different designers, using 2 different separator-naming approaches.

In Molecule Polarity, MPControlPanel is a base class that needs to support a variable number of separators. It knows nothing about the content, only that the layout is vertical, and a separator needs to be added between each subpanel. The separator tandems are therefore named using a top-to-bottom index - separator1, separator2, etc.

In Graphing Quadratics, there is no base class for panels, and separators are created in a specific context. The separator tandem names therefore reflect information about the context. In GQEquationAccordionBox, the separators are topSeparator and bottomSeparator. Other control panels have 1 separator, named separator.