Substantial refactor of command logic #39
Conversation
| var playerState = state.players[playerIdx]; | ||
| if (playerState == null) { | ||
| throw new GameException('Unknown player'); | ||
| const playerIsReady = (command, playerIdx) => { |
There was a problem hiding this comment.
I would prefer to be consistent with the existing style, rather than have a mixture of styles:
function playerIsReady(command, playerIds) {
Unless there is any reason for this syntax?
There was a problem hiding this comment.
Nope, no reason. Habit
| if (!bs) { | ||
| throw new GameException('Unknown command or invalid state'); | ||
| } | ||
| const skipEmit = bs |
There was a problem hiding this comment.
You define skipEmit then you don't use it.
| throw new GameException('Unknown player'); | ||
| const playerIsReady = (command, playerIdx) => { | ||
| if (state.players[playerIdx].isReady !== true) { | ||
| throw new GameException('You cannot start the game'); |
There was a problem hiding this comment.
You are calling this function in many other cases, not just when someone tries to start the game. Might be better to use a message related to not being ready. To be honest, I cannot remember why it is important to check that a player is ready before he starts a game.
| // You can always leave, even if your state id is old. | ||
| playerLeft(playerIdx); | ||
| }; | ||
| const playerIsUndead = (command, playerIdx) => { |
There was a problem hiding this comment.
To me, all these functions would be a lot clearer if they were named, e.g., checkPlayerIsAlive, checkItIsOurTurn, etc.
| } | ||
| action = actions[command.action]; | ||
| }; | ||
| const invalidPlayer = (stateField) => (command, playerIdx) => { |
There was a problem hiding this comment.
For clarity, rename to something like checkPlayerDoesNotActOnSelf, and the message should end in on himself or similar.
| ]; | ||
| behaviors[['play-action', stateNames.START_OF_TURN]] = [ | ||
| playersTurn, | ||
| (command, playerIdx) => { |
There was a problem hiding this comment.
I like how the behaviours are all very readable at a glance now. Maybe factor all of them out into named functions?
| @@ -234,7 +234,8 @@ module.exports = function createGame(options) { | |||
| if (onlyAiLeft()) { | |||
| destroyGame(); | |||
| } | |||
| emitState(true); | |||
| emitState(); | |||
| game.emit('statechange'); | |||
There was a problem hiding this comment.
Is it possible for statechange to be emitted twice now? Would that cause any issues?
There was a problem hiding this comment.
Since the emitting of "statechange" was such a rare usecase for emitState I just pulled it out (removing the need for emitState to take a parameter) and call it explicitly here.
| if (!state.players[playerIdx]) { | ||
| throw new GameException('Unknown player'); | ||
| } | ||
| if (command.stateId != state.stateId) { |
There was a problem hiding this comment.
The original file had a comment You can always leave, even if your state id is old - this is important
| influence.revealed = true; | ||
| state.players[playerIdx].influenceCount--; | ||
| addHistory(state.state.reason, curTurnHistGroup(), '%s; {%d} revealed %s', state.state.message, playerIdx, command.role); | ||
| const reasonMap = { |
There was a problem hiding this comment.
Is this really better than an if statement??
There was a problem hiding this comment.
I think I overdid it here in my pursuit of removing all else's
| for (var i = 0; i < state.players[playerIdx].influence.length; i++) { | ||
| var influence = state.players[playerIdx].influence[i]; | ||
| if (influence.role != command.role || influence.revealed) { | ||
| continue |
|
I like what you've done here. Some of my comments are nit picking, sorry about that. But hey, when you open a pull request just to improve my code, I expect perfection :) Thanks for taking the time. |
|
Any progress? |
|
Yeah I'm tracking down one bug still, somewhere I'm being inconsistent on
where an emission is necessary after a given command. Will try to sort it
out there next few days
…On Mon, May 4, 2020, 3:27 AM Chris Brown ***@***.***> wrote:
Any progress?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#39 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKQ7SX6RTK62PM2PIBCFDRP2KCHANCNFSM4MFET36Q>
.
|
Resolves #35