Fix deck generation to match regulation card counts #37
Conversation
The way adding 0 cards back worked out, it's generating nearly twice as many cards now, so no need to quadruple the generated deck. Doubling it suffices.
Since there is code that depends on 0 always being first, declaring it first and then never sorting the list is better than depending on the sorting algorithm to always put 0 first no matter what values may be added.
| @@ -462,14 +463,14 @@ def get_card(self): | |||
| @staticmethod | |||
| def create_deck(): | |||
| new_deck = [] | |||
| for card in COLORED_CARD_NUMS: | |||
| for card in (COLORED_CARD_NUMS + COLORED_CARD_NUMS[1:]): | |||
There was a problem hiding this comment.
With this in place, it would be good to prove that the deck is now correct according to the rules. Should be proven by debug output as well as code analysis.
There was a problem hiding this comment.
Code analysis:
4(0-9 + R + S + D2 + 1-9 + R + S + D2) + 2(W) + 2(WD4) =
4(10 + 1 + 1 + 1 + 9 + 1 + 1 + 1) + 2 + 2 =
4(25) + 4 = 104†
Not even gonna bother with debug output yet, because that's wrong. There aren't enough W/WD4 cards.
† Actually 208 because new_deck *= 2 before return new_deck, but that's not how many cards are actually generated.
There was a problem hiding this comment.
Solved by adding twice as many W/WD4 cards in c26a696. That adds 4 more cards for a total of 108, which is the regulation deck size.
| @@ -109,7 +109,8 @@ | |||
| "use color codes at all."], | |||
| 'PLAY_SYNTAX': "Command syntax error. You must use e.g. %pplay r 3 or %pplay w y.", | |||
| } # yapf: disable | |||
| COLORED_CARD_NUMS = ['1', '2', '3', '4', '5', '6', '7', '8', '9', 'R', 'S', 'D2'] | |||
| # don't sort card values to ensure 0 is ALWAYS first | |||
| COLORED_CARD_NUMS = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'R', 'S', 'D2'] | |||
There was a problem hiding this comment.
0 belongs in the special cards by virtue of the fact that it appears less often than the other numbers, but I'd need to review the rest of the code to see if there would be any unintended consequences of putting it there (besides eliminating the list slice and concatenation below).
unobot.py
Outdated
| for color in CARD_COLORS: | ||
| new_deck.append(color + card) | ||
| for card in SPECIAL_CARDS: | ||
| new_deck.append(card) | ||
| new_deck.append(card) | ||
|
|
||
| new_deck *= 4 | ||
| new_deck *= 2 |
There was a problem hiding this comment.
Theoretically the right thing to do because nearly twice as many cards are generated now. Verify count though.
Commit backdated 24 hours because GitHub decided updating my code review of the associated PR on 3/13 made it no longer count as a contribution for 3/12... Really, GitHub?
|
Finally got around to putting this code in production and testing it with some debug statements. Deck size is indeed 108. 🚀 |
Will close #28 when finished and tested/merged.