Separate Maze into Controller and Wrapper #20611

Hamms · 2018-02-14T02:24:17Z

Where the MazeController is responsible for handling all the maze stuff
like animating and drawing the map and enforcing all the maze rules like
bumping into walls and only collecting from collectable tiles, and the
MazeWrapper is responsible for all the stuff external to make like
hooking up to the Run button and turning Blockly into actions and
rendering feedback.

The ultimate goal with this PR is to prepare for us to split the Maze directory in half, with the following files all moving into the external repo:

animationsController.js
beeCell.js
beeItemDrawer.js
bee.js
cell.js
collectorDrawer.js
collector.js
constants.js
dirtDrawer.js
drawer.js
drawMap.js
farmer.js
gatherer.js
harvesterCell.js
harvesterDrawer.js
harvester.js
locale.js
mazeController.js
mazeMap.js
mazeUtils.js
planterCell.js
planterDrawer.js
planter.js
redux.js
scrat.js
subtype.js
tiles.js
timeoutList.js
utils.js
wordsearchDrawer.js
wordsearch.js

And the rest staying where they are.

balderdash

This looks great!

balderdash · 2018-02-26T20:44:08Z

apps/src/maze/maze.js

-    const Type = getSubtypeForSkin(config.skinId);
-    this.subtype = new Type(this, config);
+    this.controller = new MazeController(level, skin, config);
+    this.controller.rebindMethods({


Could these just be (optional?) constructor arguments?

joshlory

Awesome refactor! I have some reservations about extracting redux.js along with the other logic (do we need the standalone Maze app to have a Redux dependency?).

Added a few nit comments, nothing blocking this PR.

joshlory · 2018-02-26T20:39:41Z

apps/src/maze/mazeController.js

+
+    this.pegmanD;
+    this.pegmanX;
+    this.pegmanY;


Does this appropriately add these properties to the object shape? My understanding is that without an assignment this is a no-op.

You're correct; I put these here for readability, not functionality. Do you have a preference for a more useful way to call out which properties are declared on a given instance?

I'm not sure... can we set them to null/undefined for clarity?

joshlory · 2018-02-26T20:41:09Z

apps/src/maze/mazeController.js

+    // their own complex syntax. This way is deprecated for new levels,
+    // and only exists for backwards compatibility for not-yet-updated
+    // levels.
+    if (this.level.serializedMaze) {


Can we pass in level as an argument instead of adding it to this?

probably, yes; in fact, the way it currently works is that we pass in level, skin, and config to the constructor despite the fact that level and skin are actually just properties on config.

I did it this way because that's what most closely mirrors how the current version of Maze is initialized, and I wanted this PR to be just a reorganization with as few functional changes as possible, with the intention of doing cleanups like that when things are more cleanly separated.

joshlory · 2018-02-26T20:41:34Z

apps/src/maze/mazeController.js

+  /**
+   * Initialize Blockly and the maze.  Called on page load.
+   */
+


Nit: extra newline.

joshlory · 2018-02-26T20:46:48Z

apps/src/maze/mazeController.js

+    // Play sound and animation for hitting wall or obstacle
+    var squareType = this.map.getTile(targetY, targetX);
+    if (squareType === tiles.SquareType.WALL || squareType === undefined ||
+      (this.subtype.isScrat() && squareType === tiles.SquareType.OBSTACLE)) {


😕 might be good to call into something like this.subtype.handleHitObstacle make the subtype override the behavior if it's different from the default. (Future PR...)

A thousand times yes. I would like in general to remove these subtype identification methods entirely and update everything that's currently using them to instead leverage overridable methods

joshlory · 2018-02-26T20:52:46Z

apps/test/integration/levelSolutions/maze/bee/bee_other.js

@@ -43,7 +43,7 @@ module.exports = {
        testResult: TestResults.ALL_PASS
      },
      // customValidator: function () {
-      //   return Maze.subtype.nectars_.length === 2 && Maze.subtype.honey_ === 2;
+      //   return Maze.controller.subtype.nectars_.length === 2 && Maze.controller.subtype.honey_ === 2;
      // },


Correct, but I figured this PR was already large enough without doing misc cleanup

Hamms · 2018-02-26T21:24:33Z

I'm intending to address the redux dependency separately; I intentionally deferred any changes to that functionality in this PR to prevent it from becoming even larger than it already is

Where the MazeController is responsible for handling all the maze stuff like animating and drawing the map and enforcing all the maze rules like bumping into walls and only collecting from collectable tiles, and the MazeWrapper is responsible for all the stuff external to make like hooking up to the Run button and turning Blockly into actions and rendering feedback.

…o live to Maze where they now actually live

… longer be so, since they are now intended to be public

…ones

Hamms force-pushed the maze-controller branch 3 times, most recently from d5ec6fb to 3a66ef7 Compare February 23, 2018 02:54

Hamms changed the title ~~[WIP] Separate Maze into Controller and Wrapper~~ Separate Maze into Controller and Wrapper Feb 24, 2018

Hamms requested review from joshlory and balderdash February 24, 2018 02:31

balderdash approved these changes Feb 26, 2018

View reviewed changes

joshlory approved these changes Feb 26, 2018

View reviewed changes

Hamms added 14 commits February 27, 2018 13:29

update mazeTest to specifically test mazeController

635abdd

move some instance declarations from mazeController where they used t…

5b03329

…o live to Maze where they now actually live

move redux store onto maze controller

70a98ae

update a few methods that were named as if they were pseudoprivate no…

b03acb6

… longer be so, since they are now intended to be public

move studioapp method binding onto MazeController

9446e99

update a UI test step definition to use new MazeController

43d5178

update a few unnecessarily-bound instance methods to just be regular …

8355211

…ones

update wordsearch drawer to reference its own color constants

887ecff

update integration test references to point to new MazeController

4e1821f

also bind special 'playAudioOnFailure' method

dd58ff0

add controller initialization options as optional constructor arguments

aaf48bc

initialize all instance properties to null

875754b

prettify js

b72f871

Hamms force-pushed the maze-controller branch from 6a0f940 to b72f871 Compare February 27, 2018 21:29

Hamms merged commit a2a3048 into staging Feb 28, 2018

Hamms deleted the maze-controller branch May 31, 2018 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Maze into Controller and Wrapper #20611

Separate Maze into Controller and Wrapper #20611

Hamms commented Feb 14, 2018 •

edited

balderdash left a comment

balderdash Feb 26, 2018

joshlory left a comment

joshlory Feb 26, 2018

Hamms Feb 26, 2018

joshlory Feb 27, 2018

joshlory Feb 26, 2018

Hamms Feb 26, 2018

joshlory Feb 26, 2018

joshlory Feb 26, 2018

Hamms Feb 26, 2018

joshlory Feb 26, 2018

Hamms Feb 26, 2018

Hamms commented Feb 26, 2018

Separate Maze into Controller and Wrapper #20611

Separate Maze into Controller and Wrapper #20611

Conversation

Hamms commented Feb 14, 2018 • edited

balderdash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamms commented Feb 26, 2018

Hamms commented Feb 14, 2018 •

edited