Begin abstracting pool source #1543

psettle · 2021-07-29T06:04:52Z

Linked tickets

Starts on Improve interface for new game modes #1029
Related discussion in Feature Request: 2 Player Cube Format #1540

Description of your changes

Pulls the data related to the card pool into a new class. I'm not really sure if I'm on the right track, but I think this is a bit of an improvement?

Also this change enabled bots on decadent drafts, mostly as a side effect of some of the abstractions I introduced around the idea of 'burn cards'. I think this is just an improvement? I can't think of why you'd want to disable bots explicitly on decadent drafts.

My hope here is that this is a base to start separating the cards in the pool from the format being played.

I think these changes can get us going in the right direction in future patches:

change PoolBuilder.createPool() to PoolBuilder.createPacks(size, number), and move the logic about what size and number of packs out of the /pool module into a new "/format" or "/gamemode" module
Change PoolBuilder into an abstract interface, and implement it for [cube, regular, chaos] pool types. Then a poolBuilder can be injected into a Game() object as it is created.

Testing

I tested each type of draft manually and it seems OK.

Please let me know what you think!

One of the unit tests was assuming a forward slash seperator, even though the implementation uses path.join(). This meant the test would fail on Windows platforms because on Windows path.join() would use a back slash path seperator. Type: Bug Fix

Add a new class that wraps data related to what cards and how many cards will be needed for a game. Type: Code Improvement

psettle · 2021-08-14T07:11:49Z

backend/game.js

    return this.players[(index % length + length) % length];
  }
+
+  // Accessor required for unit test "can make a draft with 4 sets"
+  // (Note: It's weird that the unit tests evaluate private class members instead of


The test that is validating this struck me as very weird for how I am used to thinking about and designing unit tests.

For a class/project like this I would have either expected to see unit tests that are testing the entire backend as a unit
(i.e. the unit tests would send fake messages that we expect to receive from the front-end, and validate that the response messages meet the expectations)

and/or

I would expect to see each class/module in the backend treated as a unit
(i.e. injecting mocks of the classes these components depend on, then testing the public API of the class/module while setting expectations on the mocks to stress the expected behaviors of each component in isolation)

(That said my unit testing experience is all on much lower level languages (C/C++) and more safety critical applications, so I'm not sure what the conventions are up here in web land.)

I'm not proposing any major overall of the unit tests. I don't see that as a very practical option; but I think the 2nd approach I listed there can work pretty well on this class with the poolBuilder abstraction I've proposed in the push request going forward? Curious for others' thoughts on this.

tooomm · 2021-08-14T09:08:58Z

@ZeldaZach, @mixmix: Please share your thoughts!

mixmix · 2021-08-15T21:10:25Z

backend/pool/pool.js

@@ -1,6 +1,6 @@
-const {sample, shuffle, random, range, times, constant, pull} = require("lodash");
+const { sample, shuffle, random, range, times, constant, pull } = require("lodash");


this file rename is a little confusing to me.
what is pool/pool? should it be pool/index.js?

I moved this into /pool because I am hoping to make /pool a component (module? package? not sure on the terminology here) that exposes PoolBuilder as an interface.

I'm happy to go with whatever the convention is for node.js (I don't really have any node.js experience).

Re: pool/pool, please see my notes about where I see this change going in the pull request description.

I'm hoping to see PoolBuilder being split into 3 different implementations of an abstract interface (cube, standard, chaos) and all the logic currently in pool/pool would be split up and included directly in those 3 interface implementations.

So I did a bit more research here and I see that index.js serves as the entry point for passing a folder into require(). I don't think this file makes sense as an entry point to the 'pool' abstraction (indeed it is not included by anything external to this folder anymore).

I can't think of anything useful an index file would do for this folder, I think it would just make the import in game.js more confusing probably? Please let me know what you think :)

mixmix · 2021-08-15T21:11:06Z

backend/pool/poolBuilder.js

+/**
+ * @brief Manages parameters that control the type, grouping and quantity of cards in a pool
+ */
+module.exports = class PoolBuilder {


I love this abstraction, it feels a lot tidier

mixmix · 2021-08-15T21:14:32Z

backend/game.js

 const uuid = require("uuid");
 const jsonfile = require("jsonfile");
 const Bot = require("./player/bot");
 const Human = require("./player/human");
-const Pool = require("./pool");
+const PoolBuilder = require("./pool/poolBuilder");


my gut feeling is this should require pool and that poolGenerator or whatever it's called should be required inside pool so you can just go pool.create or pool.generate and then that calls out to the booster and pool creating functions.

I think we have a pretty similar idea of the target design here, but that we are being caught up on naming a little.

I think that PoolBuilder would be more appropriately called 'Pool', but I chose PoolBuilder because I didn't want to create confusion and conflicts with the existing usage of 'pool' in this project (where pool is an array of cards, as in Game.pool).

In future patches I see removing the current usage of 'pool' completely:

the pool/pool component would be integrated into various implementations of PoolBuilder that would be injected into Game externally

the current use of 'pool' in Game.js (this.pool) could be more appropriately called (this.packs) or some such

(Which would signal that this.packs is a concrete representation of the exact cards to be used, instead of the more abstract idea of a Pool as the rules that define the bounds of the possible cards to be used that PoolBuilder is currently capturing.)

At that point I think we could (and should) rename PoolBuilder to Pool in a way that matches your intuition here.

psettle · 2021-08-15T21:39:51Z

Thanks @mixmix for the review!

A lot of this patch is doing setup for other architecture changes related to #1029.

I'm not sure if you would prefer I do all the changes I am suggesting here in a single pull request, or if doing it in a few steps is easier to review/discuss, I'm happy to go either way, please let me know :)

mixmix · 2021-08-15T22:27:52Z

Probably small PRs.
Some context is that this repo doesn't get much active development, and there's only a couple of us reviewing things. So easy PRs are most welcome!

ZeldaZach · 2022-02-26T06:42:14Z

Hey there! Any word on if this is still compatible/finished? :)

psettle · 2022-03-04T06:24:42Z

I think this change is okay if you want it, I don't really have a personal interest in continuing work on the initial feature I proposed (#1540). The main motivation for me was gathering restrictions related to the pandemic, and they haven't been active in my area for a while, so we've just been playing in person for a while now. Feel free to close.

psettle added 3 commits July 18, 2021 00:43

Wrap pool related data in new poolBuilder class

094a20c

Add a new class that wraps data related to what cards and how many cards will be needed for a game. Type: Code Improvement

Merge branch 'dr4fters:master' into master

8059dd9

tooomm requested a review from ZeldaZach August 3, 2021 12:45

psettle added 2 commits August 14, 2021 00:44

Fix eslint errors

cb95df9

Fix unit tests after recent refactor

e178394

psettle changed the title ~~[WIP][DNS] Begin abstracting pool source~~ Begin abstracting pool source Aug 14, 2021

psettle commented Aug 14, 2021

View reviewed changes

mixmix reviewed Aug 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Begin abstracting pool source #1543

Begin abstracting pool source #1543

psettle commented Jul 29, 2021 •

edited by tooomm

psettle Aug 14, 2021 •

edited

tooomm commented Aug 14, 2021

mixmix Aug 15, 2021

psettle Aug 15, 2021

psettle Aug 16, 2021

mixmix Aug 15, 2021

mixmix Aug 15, 2021

psettle Aug 15, 2021

psettle commented Aug 15, 2021

mixmix commented Aug 15, 2021

ZeldaZach commented Feb 26, 2022

psettle commented Mar 4, 2022 •

edited

		@@ -1,6 +1,6 @@
		const {sample, shuffle, random, range, times, constant, pull} = require("lodash");
		const { sample, shuffle, random, range, times, constant, pull } = require("lodash");

Begin abstracting pool source #1543

Are you sure you want to change the base?

Begin abstracting pool source #1543

Conversation

psettle commented Jul 29, 2021 • edited by tooomm

Linked tickets

Description of your changes

Testing

psettle Aug 14, 2021 • edited

Choose a reason for hiding this comment

tooomm commented Aug 14, 2021

mixmix Aug 15, 2021

Choose a reason for hiding this comment

psettle Aug 15, 2021

Choose a reason for hiding this comment

psettle Aug 16, 2021

Choose a reason for hiding this comment

mixmix Aug 15, 2021

Choose a reason for hiding this comment

mixmix Aug 15, 2021

Choose a reason for hiding this comment

psettle Aug 15, 2021

Choose a reason for hiding this comment

psettle commented Aug 15, 2021

mixmix commented Aug 15, 2021

ZeldaZach commented Feb 26, 2022

psettle commented Mar 4, 2022 • edited

psettle commented Jul 29, 2021 •

edited by tooomm

psettle Aug 14, 2021 •

edited

psettle commented Mar 4, 2022 •

edited