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.

Sign up for GitHub

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

Begin abstracting pool source #1543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c74718f
Fix can get data directory test for Windows
psettle Jul 18, 2021
094a20c
Wrap pool related data in new poolBuilder class
psettle Jul 29, 2021
8059dd9
Merge branch 'dr4fters:master' into master
psettle Jul 29, 2021
cb95df9
Fix eslint errors
psettle Aug 14, 2021
e178394
Fix unit tests after recent refactor
psettle Aug 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 36 additions & 165 deletions backend/game.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
const crypto = require("crypto");
const path = require("path");
const {shuffle, truncate} = require("lodash");
const { shuffle } = require("lodash");
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.

const Room = require("./room");
const Rooms = require("./rooms");
const logger = require("./logger");
const Sock = require("./sock");
const {saveDraftStats, getDataDir} = require("./data");
const { saveDraftStats, getDataDir } = require("./data");

module.exports = class Game extends Room {
constructor({ hostId, title, seats, type, sets, cube, isPrivate, modernOnly, totalChaos, chaosPacksNumber, picksPerPack }) {
Expand All @@ -20,69 +20,18 @@ module.exports = class Game extends Room {
title,
seats: parseInt(seats),
type,
cube,
isPrivate,
modernOnly,
totalChaos,
chaosPacksNumber,
picksPerPack: parseInt(picksPerPack),
delta: -1,
hostID: hostId,
id: gameID,
players: [],
round: 0,
bots: 0,
sets: sets || [],
isDecadent: false,
secret: uuid.v4(),
logger: logger.child({ id: gameID })
logger: logger.child({ id: gameID }),
poolBuilder: new PoolBuilder(type, sets, cube, modernOnly, totalChaos, chaosPacksNumber)
});
// Handle packsInfos to show various informations about the game
switch(type) {
case "draft":
case "sealed":
this.packsInfo = this.sets.join(" / ");
this.rounds = this.sets.length;
break;
case "decadent draft":
// Sets should all be the same and there can be a large number of them.
// Compress this info into e.g. "36x IKO" instead of "IKO / IKO / ...".
this.packsInfo = `${this.sets.length}x ${this.sets[0]}`;
this.rounds = this.sets.length;
this.isDecadent = true;
break;
case "cube draft":
this.packsInfo = `${cube.packs} packs with ${cube.cards} cards from a pool of ${cube.list.length} cards`;
if (cube.burnsPerPack > 0) {
this.packsInfo += ` and ${cube.burnsPerPack} cards to burn per pack`;
}
this.rounds = this.cube.packs;
break;
case "cube sealed":
this.packsInfo = `${cube.cubePoolSize} cards per player from a pool of ${cube.list.length} cards`;
this.rounds = this.cube.packs;
break;
case "chaos draft":
case "chaos sealed": {
const chaosOptions = [];
chaosOptions.push(`${this.chaosPacksNumber} Packs`);
chaosOptions.push(modernOnly ? "Modern sets only" : "Not modern sets only");
chaosOptions.push(totalChaos ? "Total Chaos" : "Not Total Chaos");
this.packsInfo = `${chaosOptions.join(", ")}`;
this.rounds = this.chaosPacksNumber;
break;
}
default:
this.packsInfo = "";
}

if (cube) {
Object.assign(this, {
cubePoolSize: cube.cubePoolSize,
packsNumber: cube.packs,
playerPackSize: cube.cards
});
}

this.renew();
Rooms.add(gameID, this);
Expand Down Expand Up @@ -120,15 +69,15 @@ module.exports = class Game extends Room {
// The number of games which have a player still in them.
static numActiveGames() {
return Rooms.getAll()
.filter(({isActive}) => isActive)
.filter(({ isActive }) => isActive)
.length;
}

// The number of players in active games.
static totalNumPlayers() {
return Rooms.getAll()
.filter(({isActive}) => isActive)
.reduce((count, {players}) => {
.filter(({ isActive }) => isActive)
.reduce((count, { players }) => {
return count + players.filter(x => x.isConnected && !x.isBot).length;
}, 0);
}
Expand All @@ -148,7 +97,7 @@ module.exports = class Game extends Room {

static getRoomInfo() {
return Rooms.getAll()
.filter(({isPrivate, didGameStart, isActive}) => !isPrivate && !didGameStart && isActive)
.filter(({ isPrivate, didGameStart, isActive }) => !isPrivate && !didGameStart && isActive)
.reduce((acc, game) => {
const usedSeats = game.players.length;
const totalSeats = game.seats;
Expand All @@ -161,7 +110,7 @@ module.exports = class Game extends Room {
usedSeats,
totalSeats,
name: game.name,
packsInfo: game.packsInfo,
packsInfo: game.poolBuilder.info,
type: game.type,
timeCreated: game.timeCreated,
});
Expand Down Expand Up @@ -207,7 +156,7 @@ module.exports = class Game extends Room {
super.join(sock);
this.logger.debug(`${sock.name} joined the game`);

const h = new Human(sock, this.picksPerPack, this.getBurnsPerPack(), this.id);
const h = new Human(sock, this.picksPerPack, this.poolBuilder.implicitBurnsPerPack, this.id);
if (h.id === this.hostID) {
h.isHost = true;
sock.once("start", this.start.bind(this));
Expand All @@ -222,17 +171,6 @@ module.exports = class Game extends Room {
this.meta();
}

getBurnsPerPack() {
switch (this.type) {
case "decadent draft":
return Number.MAX_VALUE;
case "cube draft":
return this.cube.burnsPerPack;
default:
return 0;
}
}

swap([i, j]) {
const l = this.players.length;

Expand Down Expand Up @@ -266,15 +204,15 @@ module.exports = class Game extends Room {
isHost: h.isHost,
round: this.round,
self: this.players.indexOf(h),
sets: this.sets,
sets: this.poolBuilder.sets,
gameId: this.id
});
h.send("gameInfos", {
type: this.type,
packsInfo: this.packsInfo,
sets: this.sets,
packsInfo: this.poolBuilder.info,
sets: this.poolBuilder.sets,
picksPerPack: this.picksPerPack,
burnsPerPack: this.type === "cube draft" ? this.cube.burnsPerPack : 0
burnsPerPack: this.poolBuilder.explicitBurnsPerPack
});

if (this.isGameFinished) {
Expand Down Expand Up @@ -322,9 +260,9 @@ module.exports = class Game extends Room {
}

uploadDraftStats() {
const draftStats = this.cube
? { list: this.cube.list }
: { sets: this.sets };
const draftStats = this.poolBuilder.isCube
? { list: this.poolBuilder.cubeList }
: { sets: this.poolBuilder.sets };
draftStats.id = this.id;
draftStats.draft = {};

Expand All @@ -345,14 +283,14 @@ module.exports = class Game extends Room {
}
});
const cubeHash = /cube/.test(this.type)
? crypto.createHash("SHA512").update(this.cube.list.join("")).digest("hex")
? crypto.createHash("SHA512").update(this.poolBuilder.cubeList.join("")).digest("hex")
: "";

const draftcap = {
"gameID": this.id,
"players": this.players.length - this.bots,
"type": this.type,
"sets": this.sets,
"sets": this.poolBuilder.sets,
"seats": this.seats,
"time": Date.now(),
"cap": this.players.map((player, seat) => ({
Expand Down Expand Up @@ -407,7 +345,7 @@ module.exports = class Game extends Room {
});
}

if (this.round++ === this.rounds) {
if (this.round++ === this.poolBuilder.rounds) {
return this.end();
}

Expand Down Expand Up @@ -466,64 +404,6 @@ module.exports = class Game extends Room {
return this.players.map((player) => player.getPlayerDeck());
}


createPool() {
switch (this.type) {
case "cube draft": {
this.pool = Pool.DraftCube({
cubeList: this.cube.list,
playersLength: this.players.length,
packsNumber: this.cube.packs,
playerPackSize: this.cube.cards
});
break;
}
case "cube sealed": {
this.pool = Pool.SealedCube({
cubeList: this.cube.list,
playersLength: this.players.length,
playerPoolSize: this.cubePoolSize
});
break;
}
case "draft":
case "decadent draft": {
this.pool = Pool.DraftNormal({
playersLength: this.players.length,
sets: this.sets
});
break;
}

case "sealed": {
this.pool = Pool.SealedNormal({
playersLength: this.players.length,
sets: this.sets
});
break;
}
case "chaos draft": {
this.pool = Pool.DraftChaos({
playersLength: this.players.length,
packsNumber: this.chaosPacksNumber,
modernOnly: this.modernOnly,
totalChaos: this.totalChaos
});
break;
}
case "chaos sealed": {
this.pool = Pool.SealedChaos({
playersLength: this.players.length,
packsNumber: this.chaosPacksNumber,
modernOnly: this.modernOnly,
totalChaos: this.totalChaos
});
break;
}
default: throw new Error(`Type ${this.type} not recognized`);
}
}

handleSealed() {
this.round = -1;
this.players.forEach((p) => {
Expand All @@ -534,7 +414,7 @@ module.exports = class Game extends Room {
}

handleDraft() {
const {players, useTimer, timerLength} = this;
const { players, useTimer, timerLength } = this;

players.forEach((p, self) => {
p.useTimer = useTimer;
Expand All @@ -547,21 +427,14 @@ module.exports = class Game extends Room {
this.startRound();
}

shouldAddBots() {
return this.addBots && !this.isDecadent;
}

start({ addBots, useTimer, timerLength, shufflePlayers }) {
try {
Object.assign(this, { addBots, useTimer, timerLength, shufflePlayers });
this.renew();

if (this.shouldAddBots()) {
if (this.addBots) {
while (this.players.length < this.seats) {
const burnsPerPack = this.type === "cube draft"
? this.cube.burnsPerPack
: 0;
this.players.push(new Bot(this.picksPerPack, burnsPerPack, this.id));
this.players.push(new Bot(this.picksPerPack, this.poolBuilder.implicitBurnsPerPack, this.id));
this.bots++;
}
}
Expand All @@ -570,7 +443,7 @@ module.exports = class Game extends Room {
this.players = shuffle(this.players);
}

this.createPool();
this.pool = this.poolBuilder.createPool(this.players.length);

if (/sealed/.test(this.type)) {
this.handleSealed();
Expand All @@ -580,7 +453,7 @@ module.exports = class Game extends Room {

this.logger.info(`Game ${this.id} started.\n${this.toString()}`);
Game.broadcastGameInfo();
} catch(err) {
} catch (err) {
this.logger.error(`Game ${this.id} encountered an error while starting: ${err.stack} GameState: ${this.toString()}`);
this.players.forEach(player => {
if (!player.isBot) {
Expand All @@ -603,25 +476,23 @@ module.exports = class Game extends Room {
title: ${this.title}
seats: ${this.seats}
type: ${this.type}
sets: ${this.sets}
isPrivate: ${this.isPrivate}
picksPerPack: ${this.picksPerPack}
modernOnly: ${this.modernOnly}
totalChaos: ${this.totalChaos}
chaosPacksNumber: ${this.chaosPacksNumber}
packsInfos: ${this.packsInfo}
players: ${this.players.length} (${this.players.filter(pl => !pl.isBot).map(pl => pl.name).join(", ")})
bots: ${this.bots}
${this.cube ?
`cubePoolSize: ${this.cube.cubePoolSize}
packsNumber: ${this.cube.packs}
playerPackSize: ${this.cube.cards}
cube: ${truncate(this.cube.list, 30)}`
: ""}`;
poolBuilder:
${this.poolBuilder.toString()}`;
}

getNextPlayer(index) {
const {length} = this.players;
const { length } = this.players;
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 •
edited
Loading

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.

// validating behavior, probably worth a look at the approach at some point)
get rounds() {
return this.poolBuilder.rounds;
}
};
Loading