PURPOSE:
In order to get comfortable with Python classes and object-oriented programming, I've decided to simulate the Exploding Kittens card game. To make it easier to program, I've greatly simplified the rules of the game.
I'd really appreciate feedback on my code to identify errors in syntax, style, modeling, or expressiveness. Thanks!
RULES:
In this version, as with the original version, the goal of Exploding Kittens is to be the last player standing. (Original version rules here)
Each round, players take turns drawing from a deck filled with different types of cards. In this version of the game, there are only two types of cards: Regular and Exploding Kitten. If the player draws a Regular card, then they've survived that round. But when the player draws an Exploding Kitten card, they're dead! Players will want to avoid drawing an Exploding Kitten card for as long as they can.
Futhermore, in this version of the game, the player has only one possible action they can take each round. They MUST draw the top card from the deck. They cannot shuffle the deck, steal cards, etc. as is possible in the original version of the game. They can only draw the topmost card in the deck. Therefore, the outcome of the game is pure chance.
When only a single player remains, that player wins the game!
import random
class Card:
def __init__(self, name):
self.name = name
def __str__(self):
return self.name
def __repr__(self):
return str(self.name)
class Deck:
def __init__(self, kittens = 4, regulars = 48):
self.kittens = kittens
self.regulars = regulars
self.types = ['Exploding Kitten']*self.kittens + ['Regular']*self.regulars
self.actual_cards = [Card(i) for i in self.types]
def shuffle(self):
random.shuffle(self.actual_cards)
def get_top_card(self):
card_to_be_returned = self.actual_cards.pop(0)
return card_to_be_returned
def get_four_cards(self): #for dealing the initial hand to each player
four_cards = []
for i in range(4):
card_name_list = []
for j in range(len(self.actual_cards)):
card_name_list.append(self.actual_cards[j].name)
regular_card_index = card_name_list.index('Regular')
regular_card = self.actual_cards.pop(regular_card_index)
four_cards.append(regular_card)
return four_cards
class Player:
def __init__(self, ID, hand):
self.player_ID = ID
self.hand = hand
self.alive = True
def __str__(self):
return self.player_ID
def __repr__(self):
return str(self.player_ID)
class ExplodingKittens():
def __init__(self,num_players):
self.num_players = num_players
self.player_list = []
def start_game(self): #set up game for first round of card draws
self.deck_of_cards = Deck(self.num_players - 1, 53 - self.num_players)
for player_ID in range(1, self.num_players + 1): #start w Player 1
cards_for_player = self.deck_of_cards.get_four_cards() #Deal card to player
player_ID = "Player " + str(player_ID)
new_player = Player(player_ID, cards_for_player) #Save player ID and card
self.player_list.append(new_player)
self.deck_of_cards.shuffle()
def play_round(self):
for i in range(len(self.player_list)):
top_card = self.deck_of_cards.get_top_card()
print('{} drew {}'.format(self.player_list[i], top_card))
if str(top_card) == 'Exploding Kitten':
print('{} is dead!'.format(self.player_list[i]))
self.player_list[i].alive = False
alive = [self.player_list[j].alive for j in range(len(self.player_list))]
if sum(alive) == 1:
winner_index = alive.index(True)
return '{} won the game!'.format(self.player_list[winner_index])
player_list = [] #update player_list with only living players
for i in range(len(self.player_list)):
if self.player_list[i].alive:
player_list.append(self.player_list[i])
self.player_list = player_list
def decide_winner(self):
while len(self.player_list) > 1:
outcome = self.play_round()
if outcome is not None:
print(outcome)
break
if __name__=="__main__":
"""run this code when executing module as a program
and not when importing its classes"""
game = ExplodingKittens(5)
game.start_game()
print(game.player_list)
game.decide_winner()
3 Answers 3
Looking at the construction of the deck_of_cards
variable, you actually create the internal variable outside the __init__
function (inside the start_game
function). So, that change would result in (the brackets are redundant too):
class ExplodingKittens:
def __init__(self, num_players):
self.num_players = num_players
self.player_list = []
self.deck_of_cards = None
Also, the constuction of the Deck
, there are some defaults which we all know, 52 cards in a deck, so let's take advantage of that -
class Deck:
def __init__(self, kittens=None):
self.kittens = kittens or 4
self.regulars = 52 - kittens
...
There's a good reason for not specifying a default value in the function defintion of the __init__
, and that's because the value is only evaluated once at definition time. Specifying None as the argument then comparing verses a default value is safer (sorry if I haven't explained that well!).
This of course changes the construction of the deck_of_cards
statement:
self.deck_of_cards = Deck(self.num_players - 1, 53 - self.num_players)
into:
self.deck_of_cards = Deck(self.num_players - 1)
Which raises the questions regarding your design. You're using multiple controls to determine the winner, such as:
alive = [self.player_list[j].alive for j in range(len(self.player_list))]
if sum(alive) == 1:
winner_index = alive.index(True)
return '{} won the game!'.format(self.player_list[winner_index])
and also the:
while len(self.player_list) > 1:
in the decide_winner
function (also you don't need that, when you create the game it should start playing immediately).
As a suggestion, I would update the design to have players in a list, and when a player obtains an Exploding Kitten, remove them from that list, and perform a len(players)
used to determine if the game has finished (also the Player
class no longer needs the self.alive
parameter).
Having your game designed that way would also eliminate code like:
player_list = [] # update player_list with only living players
for i in range(len(self.player_list)):
if self.player_list[i].alive:
player_list.append(self.player_list[i])
self.player_list = player_list
which would become redundant. Also, just a note on that type of code - you're checking for properties on objects via the index, rather than doing a statment like for player in players:
which iterates through each object in the list (do you see the distinction?).
I hope this is enough to begin your rewrite, good luck!
- You can change
Card
andPlayer
intocollections.namedtuple
s. - You don't need to define
self.kittens
,self.regulars
orself.types
onDeck
. - Change your types from raw strings to variables. Or even better an
enum
. - Reduce the size of your variable names
self.actual_cards
can just beself.cards
,self.deck_of_cards
can just beself.deck
. - In
get_top_card
you don't needcard_to_be_returned
. - Your function
get_four_cards
is pretty static. You may want to make this function more generic. - You don't need
self.num_players
onExplodingKittens
. - It'd be better if
start_game
doesn't mutate object state, make it returndeck
andplayers
. In the future you may want to shuffle the deck before taking cards. This will mean people can then draw different cards.
Note: you also want to keep your current shuffle, as
get_four_cards
will create bias.You may want to run
start_game
inExplodingKittens.__init__
.- You can simplify
play_round
if you merge your two loops into onewhile
loop. Do this by manually performing the same asrange
, but don't increment the index if you remove a player. - I'd recommend you don't
return
data fromplay_round
, just use it to exit the function. - I recommend you make most of your class values 'privates', just prefix them with
_
. Sodeck
becomes_deck
.
Below is how I'd improve your code, I also made get_card
and get_cards
more generic. Possibly more than you'd need in your game.
import random
import collections
import enum
Card = collections.namedtuple('Card', 'type')
Player = collections.namedtuple('Player', 'id hand')
class CardTypes(enum.Enum):
EXPLODING_KITTEN = 'Exploding Kitten'
REGULAR = 'Regular'
class Deck:
def __init__(self, kittens=4, regulars=48):
self._cards = [
Card(i) for i in (
[CardTypes.EXPLODING_KITTEN] * kittens
+ [CardTypes.REGULAR] * regulars
)
]
def shuffle(self):
random.shuffle(self._cards)
def card_fn(self, key=None, exclude=None, include=None):
null = object()
def fn(value):
if key:
value = getattr(value, key, null)
return (
(exclude and value in exclude)
or (include and value not in include)
)
return fn
def get_card(self, fn=None):
if fn is None:
fn = lambda card: False
card = None
cards = []
while card is None:
card = self._cards.pop()
if fn(card):
cards.append(card)
card = None
self._cards.extend(reversed(cards))
return card
def get_cards(self, n, fn=None):
return tuple(self.get_card(fn=fn) for _ in range(n))
class ExplodingKittens:
def __init__(self, amount_players):
self._deck, self._players = self.start_game(amount_players)
def start_game(self, amount_players):
deck = Deck(amount_players - 1, 53 - amount_players)
deck.shuffle()
exclude_kittens = deck.card_fn('type', exclude={CardTypes.EXPLODING_KITTEN})
players = [
Player(f'Player {id + 1}', list(deck.get_cards(4, exclude_kittens)))
for id in range(amount_players)
]
deck.shuffle()
return deck, players
def play_round(self):
index = 0
while index < len(self._players):
player = self._players[index]
card = self._deck.get_card()
player.hand.append(card)
print('{} drew {}'.format(player.id, card.type.value))
if card.type is CardTypes.EXPLODING_KITTEN:
print('{} is dead!'.format(player.id))
self._players.pop(index)
else:
index += 1
if len(self._players) == 1:
print('{} won the game!'.format(self._players[0].id))
return
def decide_winner(self):
while len(self._players) > 1:
self.play_round()
if __name__ == "__main__":
game = ExplodingKittens(5)
print(game._players)
game.decide_winner()
Functional approach
My suggestion is to use to use functional style as much as you can while you work with lists, because it will significantly reduce number lines of your code and will make it (code) more concise and readable. The first thing I would suggest to do is getting rid of for
loops. Instead of them you can use list comprehension.
Modular code
In my experience is better to use five small functions or methods instead of one big. I always try to think in terms of atomic operations. One atomic operation is one function or method with single responsibility. To solve complex problem you just need to combine simple solutions in a correct way.
Remarks
class Card
I suggest to use type of cards not names, because you have only two unique names. You can useenum
to represent type of cards.self.actual_cards = ...
Wordactual
from property nameactual_cards
is not giving you any additional information, so you can leave only wordcards
def get_top_card(self)
In my opinion changing state (actual_cards
) in method, which's prefix isget
, is strange. I would suggest to use prefixpop
ortake
. Body of method could be simplified toreturn self.actual_cards.pop(0)
class Deck
I suggest to create some private methods to work with Deck. For example:_take_card(type)
: take first encountered card of given type;_take_cards(amount, type)
: take few cards of given typedef get_four_cards(self)
Number of cards should not be hardcoded in method's name. It's better to have parameter for that. I suggest to rename this method to something likedeal_regular_cards(number_of_cards)
.class Player
Parameterhand
could be default, because logically you can have player without hand and for me (maybe only) is unclear that you need to passhand
as second parameter in constructor. I think it's better to have something like:Player(123, hand=[...])
. Propertyplayer_ID
could be simplified toid
, because you are already inPlayer
contextclass ExplodingKittens
In my opinion is better to initialize all properties in constructor:player_list
(create all players in constructor) anddeck_of_cards
.player_ID = "Player " + str(player_ID)
You can move this logic to method__str__
of classPlayer
def decide_winner
The name of this method is not match what it does. This is actually game loop. Choosing winner is in methodplay_round()
.
Explore related questions
See similar questions with these tags.