phetsims / build-an-atom

"Build an Atom" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/build-an-atom
GNU General Public License v3.0
11 stars 10 forks source link

Instance variables should be declared in the constructor #116

Closed samreid closed 8 years ago

samreid commented 8 years ago

the inherit call in ToElementProblemView.js begins like so:

  return inherit( ProblemView,
    CountsToElementProblemView,
    {
      periodicTableAtom: new NumberAtom(),
      neutralOrIon: new Property( 'noSelection' ),
      checkAnswer: function() {

The problem is that periodicTableAtom and neutralOrIon are not instance variables, but class variables. It seems like the intention for these to be instance variables. This sim has passed QA so this is probably a latent bug.

I only saw 2 other files with this pattern, and they may be singletons: RewardNode.js and BuildAnAtomModel.js

The declaration of the periodicTableAtom and neutralOrIon will impact tandemization (and that is how I ran into this issue).

jbphet commented 8 years ago

Looks like this was fixed for ToElementProblemView.js in 3d7e1761b096cbc19238a2ecbf76feba8db1235e by @samreid while working on 'tandemization'.

jbphet commented 8 years ago

I fixed an instance of this anti-pattern in BuildAnAtomModel, and it looks like @samreid fixed one in ToElementProblemView, and the common code RewardNode is now used in the sim instead of the previous unique one. That means that everything listed above should new be addressed. Assigning to @samreid for verification.

samreid commented 8 years ago

BAAGameModel still has this instance variable declared in inherit:

    // Set of external functions that the model will step.
    _stepListeners: [],

and the author probably intended these constants to be static, not instance variables:

    // Public constants.
    MAX_POINTS_PER_GAME_LEVEL: PROBLEMS_PER_LEVEL * POSSIBLE_POINTS_PER_PROBLEM,
    PROBLEMS_PER_LEVEL: PROBLEMS_PER_LEVEL

I left the latter marked in this issue because they are technically instances variables which are not declared in the constructor, but for this case, the more appropriate solution seems to move them to the static declarations in the 4th arg of inherit().

All other inherit calls look good.

jbphet commented 8 years ago

I fixed the two issues pointed out in the comment above. Since @samreid said that the other inherit calls look good, I'm closing this one.