[CODE REVIEW] Half working version of Coup: Reformation - Githubissues

octachrome / treason

A clone of the card game Coup written in Node.js

Other

139 stars 79 forks source link

[CODE REVIEW] Half working version of Coup: Reformation #30

Closed NobleUplift closed 4 years ago

NobleUplift commented 4 years ago

EDIT: Sorry! I just saw the very hidden "Create draft pull request" option after I created this pull request. It's really stupid that GitHub hides that functionality. I should have made this pull request a draft.

I'm creating this Pull Request with the intent that it will either linger here until I can fully implement Coup: Reformation, or if you create a coup-reformation branch, I could move my pull request to that branch instead (or if I can't update this pull request, reject it so that I can create a new one).

Long story short, I've implemented half of Coup: Reformation. I based this on the original version of Coup: Reformation which had Catholic and Protestant Teams. Here are my changes:

Added Apostatize, Conversion, and Embezzlement actions. The "apostatize" and "conversion" actions are both "conversion", but the way this code is written, I had to separate out the 1 coin convert yourself, from the 2 coin convert someone else
Increased maximum players to 10. I could not find an easy way to switch between 6 players for ambassador/inquisitor (could not remove excess AI players from game), so all gamemodes can support 10 players now
Added treasury reserve logic to the game. Treasury reserve shows up in the bubble next to embezzlement
Added Catholic/Protestant teams to the game. Players will be set to a dead team when they lose all influences
Allow players to apostatize and convert alongside coup when they have >$9
Players on the same team cannot target each other (steal, assassinate), but they can still challenge each other
AI players can use and bluff embezzlement
Added deck support for players greater than 6. Can theoretically support an infinite number of players as it will continue to add cards to the deck for each 2 players in the game

Not implemented yet:

Players on the same team can still block foreign aid
AI logic for apostatizing and converting
Cannot challenge embezzlement

Here are my thoughts on the four current issues I see with this code:

What should the apostatize and conversion icons be?
Should Coup: Reformation have two gamemodes, one for ambassadors, and one for inquisitors?
Instead of "Start X game", the buttons should set the gamemode, which will set the roles and players, and then a second button starts the game. This will allow toggling between a maximum of 6 players, 8 players, 10 players, etc.
I looked into foreign aid, but couldn't find where I'd need to put the code to prevent team members from blocking foreign aid from each other
We will need two versions of the strongestPlayer function, one that finds strongest player on team, and another that finds strongest player in-game. Another function will have to aggregate the strength of each team, and if the other team is stronger than your own power, convert. Otherwise, if the other team is only as strong as your own power, apostatize.

octachrome commented 4 years ago

It's nice that you have spent some time on this, thanks. Sorry I haven't had a chance to look at your code yet. I will try to find time at the weekend.

octachrome commented 4 years ago

This all seems pretty good to me. I like the idea of using "!duke" to encode that the embezzle action requires a player not to have a duke. I agree about having a single start button. I'm not sure we really need a ui to set the maximum number of players. It could just be 10. I think instead of a game mode, you just have radio buttons to choose between inquisitor and ambassador, and a checkbox to enable teams.

octachrome commented 4 years ago

couldn't find where I'd need to put the code to prevent team members from blocking foreign aid from each other

You need to add a check near throw new GameException('Cannot block your own action') in game.js, to make sure the person you are blocking is on a different team to you (or everyone is on the same team).

NobleUplift commented 4 years ago

Happy New Year! Back from my vacation and back to work. I should be able to pick this back up in my free time. There's a paragraph of the rules that needs to be considered and I'll copy it here when I remember.

octachrome commented 4 years ago

I'd like to make some progress on this. Are you still working on it? Or should I pick it up myself?

octachrome commented 4 years ago

I like this icon for apostatize, like signing a confession: Icons: https://game-icons.net/1x1/delapouite/scroll-quill.html

And I like this one for convert, a crusader shield: https://game-icons.net/1x1/delapouite/templar-shield.html

octachrome commented 4 years ago

It would be nice to show a team icon next to each player, as well as a color stripe. How about these:

https://game-icons.net/1x1/skoll/fist.html https://game-icons.net/1x1/lorc/white-tower.html

octachrome commented 4 years ago

Allow players to apostatize and convert alongside coup when they have >$9

I don't see anything about this in the rules, so I think you should still be forced to coup in this case. Unless you have a convincing reason why not?

octachrome commented 4 years ago

I have created a branch with your changes merged into my latest master: https://github.com/octachrome/treason/tree/coup-reformation

Please merge this into your own branch, and if you do any further work, please raise a new pull request into my coup-reformation branch, so we can work together until this is ready to ship.

octachrome commented 4 years ago

I have made a lot of progress on this:

Embezzlement now requires you to not have a duke, and can be challenged
Prevented reformation actions when it is not a reformation game (AI was embezzling in normal games)
Prevented players challenging and blocking their teammates (including AI)
Updated the rules page
Fixed several bugs and failing unit tests

I tidied up a few other things too, such as the action names. I renamed "apostatize" to "change team" - while I think it is a cool word, it is not a familiar one, especially to people whose first language is not English.

octachrome commented 4 years ago

And now we have team icons. Also, you can now challenge your teammates, since this seems to be the generally accepted interpretation of the game rules. I will make this live soon and get some feedback.

NobleUplift commented 3 years ago

So sorry for the late reply!!! I wasn't getting GitHub notifications for this pull request. I have been getting notifications for issues... it looks like I was getting GitHub email notifications but they were being improperly/inconsistently sorted by Focused Inbox, some in Updates and some in Forums. Well, time to disable those.

Glad to see how productive you have been during the pandemic. I haven't had time to work on my GitHub projects. I think the website is down right now, but I can't wait to play 10-player Coup!