Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Begin abstracting pool source #1543

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

psettle
Copy link
Contributor

@psettle psettle commented Jul 29, 2021

Linked tickets

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
@tooomm tooomm requested a review from ZeldaZach August 3, 2021 12:45
@psettle psettle changed the title [WIP][DNS] Begin abstracting pool source Begin abstracting pool source Aug 14, 2021
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
Copy link
Contributor Author

@psettle psettle Aug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

tooomm commented Aug 14, 2021

@ZeldaZach, @mixmix: Please share your thoughts!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this abstraction, it feels a lot tidier

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

psettle commented Aug 15, 2021

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
Copy link
Member

mixmix commented Aug 15, 2021

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
Copy link
Member

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

@psettle
Copy link
Contributor Author

psettle commented Mar 4, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants