phetsims / isotopes-and-atomic-mass

"Isotopes And Atomic Mass" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
3 stars 3 forks source link

Return from forEach is not behaving as expected #104

Closed samreid closed 3 years ago

samreid commented 3 years ago

Similar to the problem in https://github.com/phetsims/gene-expression-essentials/issues/138, discovered in https://github.com/phetsims/chipper/issues/1033. It looks like this code is supposed to short circuit when a match is found, but it iterates over everything anyways.

  checkForParticleOverlap() {
    let overlapCheck = false;

    this.containedIsotopes.forEach( isotope1 => {
      for ( let i = 0; i < this.containedIsotopes.length; i++ ) {
        const isotope2 = this.containedIsotopes.get( i );
        if ( isotope1 === isotope2 ) {
          // Same isotope so skip it!
          continue;
        }

        const distance = isotope1.positionProperty.get().distance( isotope2.positionProperty.get() );
        if ( distance < isotope1.radiusProperty.get() + isotope2.radiusProperty.get() ) {
          overlapCheck = true;
          return overlapCheck;
        }
      }
    } );

    return overlapCheck;
  }
samreid commented 3 years ago

Similar problem in this code:

  getNumericalControllerForIsotope( isotope ) {
    let isotopeController = null;
    this.numericalControllerList.forEach( controller => {
      if ( controller.isotopeConfig.equals( isotope ) ) {
        // Found it.
        isotopeController = controller;
        return isotopeController;
      }
    } );
    return isotopeController;
  }
jbphet commented 3 years ago

I fixed the first bit of code by using standard C-style loops and using the overlap flag to drop out of them. I tested this via explicit overlap testing and fuzz testing.

The second bit of code doesn't seem to be used, so I simply deleted this.

I don't think a follow-up review is necessary so I'm just going to close this. @samreid - If you see this go by, you're welcome to review my changes and reopen if you think it's justified.