Review the 1st screen

[samreid]

@ariel-phet asked if I could do a 1-hour review of the 1st screen and report back on the status. Elevating priority so I can try to get to it before Wednesday.

[samreid]

Missing features from considering the design:

• [ ] salt falls out the wrong way
• [ ] sugar doesn't dispense
• [ ] beaker tick labels aren't shown
• [ ] conductivity tester doesn't drag out of the toolbox
• [ ] water volume readout should be fully submerged but is halfway out
• [ ] when evaporating a solution, sugar and sugar bars appear out of nowhere (even when neither were in the solution)
• [ ] mock-up overlay needs to be removed
• [ ] numerous problems for string test = double

• [ ] bars overlap concentration title

There may be more problems, but that's what I could find in about 10 minutes. I'll look at the code next.

[samreid]

• [ ] incorrect usage of self in MacroCrystal.js and MacroModel.js (probably elsewhere too)
• [ ] missing visibility annotations in methods
• [ ] collision of crystals with wall of beaker seems to be implemented but not working properly
• [x] no code is namespaced, so will not run with ?ea
• [ ] there is a value that says this sim runs at 30 frames per second. It should run at the browser speed (60fps for a laptop or greater)
• [ ] private/public annotations missing in many places
• [ ] complex/suspicious use of derived property subclass as a dependency of another derived property, like new DerivedProperty( [new AirborneCrystalMoles( self.saltList )]
• [ ] property links not using arguments properly, like this:

    self.moreSaltAllowed = new DerivedProperty( [ self.salt.grams, self.airborneSaltGrams ], function() {
      return (self.salt.grams.get() + self.airborneSaltGrams.get()) < 100;
    } );

• [ ] stale commented out Java code in each file (for reference)

Some notes:

• The code seems well-documented
• The code seems like it follows the implementation details from Java

[samreid]

• [ ] It looks like some files in common may be in the wrong directory, such as sphericalparticles/
• [ ] AbstractSugarAndSaltSolutionsModel should probably be deleted (unnecessary)
• [ ] Should the bar charts be in common code?
• [ ] Some constructors have too many required parameters, like

function MacroSugarDispenser( x, y, beaker, moreAllowed, sugarDispenserName, distanceScale, selectedType, type, model ) {

• [ ] Why does the submerged in water node contain the conductivity tester toolbox?

self.submergedInWaterNode.addChild( self.conductivityToolboxNode );

[samreid]

To summarize, I do not believe that the above list of problems is complete--it is more of a sampling of the kind of problems we may find as we work on bringing the simulation to production. Furthermore, it is certain that new problems will be uncovered as we work through the original list of known problems.

A note about my methodology, a more thorough following of the code review checklist will reveal other trouble. (This review was more "freehand").

I would also recommend having a QA member or simulation design member spend some time looking at the functionality of the simulation and comparing it to the published Java version, to see what I missed in https://github.com/phetsims/sugar-and-salt-solutions/issues/30#issuecomment-357409959

@ariel-phet I'm sure I could find more trouble given more time but this feels like a reasonable stopping point for a first pass. Over to you.

[ariel-phet]

@veillette has agreed to help mentor the student working on this project. Assigning to him so he can see @samreid's very quick review.

[samreid]

I fixed the namespaces and got it running with ?ea