search

phetsims / expression-exchange

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

centralize decision making for expression and coin term join behavior #66

Closed jbphet closed 7 years ago

jbphet commented 7 years ago

This sim has some fairly complex behavior for making decisions about when expressions combine with one another, when coin terms combine, when expressions combine with coin terms, when collection areas have priority over joining expressions, and so on. This logic is currently distributed between four locations:

  1. In ExpressionManipulationModel.js, where listeners are added when expressions and coin terms are added to the model
  2. In the ExpressionManipulationModel.js step function, where determinations are made about what is hovering over what
  3. In several functions in ExpressionManipulationModel.js that determine which thing is overlapping the most with another thing, which often includes logic about whether it is eligible for combining
  4. In ExpressionManipulationView.js, where decisions are made about removing coin terms from the model if they are released over the creator box

This seems to work reasonably well, but would be hard to maintain since there is a lot of interdependency and implicit assumptions about the order in which various notifications will proceed. I think it would be best to move the bulk of this logic into one location - the listeners that are added to coin terms and expressions when the are added to the model - and to rename/refactor the methods such as getExpressionMostOverlappingWithExpression (and there are a number of these sorts of functions) to something more like getExpressionMostEligibleToCombineWithExpression and to have some of the logic about the eligibility to combine in these methods (it's currently there, but the method names don't really reflect this).

jbphet commented 7 years ago

I spent some time working on this, and I've realized that items 1 and 2 above don't really lend themselves to simplification because some of the code is base on events, and so was written using listeners, and the other code was written to looks at the state at a given moment in time, so it lends itself to being in the step function. I've documented this but largely left it as is.

jbphet commented 7 years ago

This is done, the results seem like a substantial improvement. Closing.