I am looking for some feedback on my blackjack Python code. Any feedback is much appreciated. Docstring, code etc.
Other than that, I need to test it, and I am not sure how to make this code more testable. Any tips on that would also be appreciated.
from random import shuffle
class Card():
"""Class handles cards in a deck
Attributes:
suit -- The possible suits in a deck of cards
card_value -- The possible values in a deck of cards
"""
suits = [('Heart',1), ('Diamond',2), ('Spade',3), ('Club',4)]
values = [('Ace',11),('Two',2),('Three',3),('Four',4),('Five',5),
('Six',6),('Seven',7), ('Eight',8), ('Nine',9), ('Ten',10),
('Jack',10), ('Queen',10),('King',10)]
def __init__(self, card_value = 0, suit = 0):
"""Inits Card class with card_value and suit """
self.card_value = Card.values[card_value]
self.suit = Card.suits[suit]
class Entity():
"""Class handles entities and game logic
Attributes:
bet_account: int -- holds the players account amount
entity_name: string -- holds the name of the player
cards: list -- holds the cards
"""
def __init__(self, bet_account = 0, entity_name = 'name'):
"""Inits Enitity class with bet_account, entity.name and cards """
self.bet_account = bet_account
self.entity_name = entity_name
self.cards = []
def deposit(self, amount):
"""deposit momey into players account
Parameters:
amount: int -- amount to deposit
"""
self.bet_account += amount
def calc_card_value(self):
"""calculates the total value of a players cards, and handles aces
Returns:
the total value of player or house cards.
"""
total_value = 0
for card in self.cards:
total_value += card.card_value[1]
#checks for aces, and adjust accordingly
if total_value > 21:
for card in self.cards:
if card.card_value[0] == "Ace":
total_value -= 10
if total_value < 21:
break
return total_value
def print_current_cards(self):
"""prints the current cards on the table to the terminal"""
print('---------------------------------')
print(f'{self.entity_name.capitalize()}''s cards:')
for card in self.cards:
print(f'Card: {card.card_value[0]}, of {card.suit[0]}')
print(f'Total card value: {self.calc_card_value()}')
print('---------------------------------')
def generate_deck():
"""Generate a deck of cards.
Returns:
a new deck containing 52 cards
"""
deck = []
order = list(range(1,53))
shuffle(order)
for i in order:
card = Card(i % 13, i % 4)
deck.append(card)
return deck
def deal_card(player, deck):
"""gets a card out of the deck, to hand over to player
Parameters:
player: obj -- object of player
deck: list -- list of the deck
"""
player.cards.append(deck.pop())
def check_winner(player, house, bet):
"""Check who won the game by going through the scores and dertimining who won """
if house.calc_card_value() == 21:
print("House got blackjack!")
if player.calc_card_value() == 21:
print(player.entity_name + " got blackjack!")
if house.calc_card_value() > 21:
print(player.entity_name + " won")
player.deposit(bet)
elif player.calc_card_value() > house.calc_card_value():
print(player.entity_name + " won")
player.deposit(bet)
elif player.calc_card_value() == house.calc_card_value():
print("Tie!")
else:
print('House won')
player.deposit(-bet)
def play_game(player, house, deck, bet):
"""
Game functionality; deals cards,
handles hit and pass,
checks if player busts
Parameters:
player: obj -- player object
house: obj -- house object
deck: list -- list of deck
bet: int -- placed bet
"""
#deals 2 cards to the player, and one for the dealer
deal_card(house, deck)
deal_card(player, deck)
deal_card(player, deck)
#prints the current card on the table
player.print_current_cards()
house.print_current_cards()
bust = False
#get user input.
#if user busts, bust is set to True, and the player looses their bet
#if the user decides to hit, they are dealt another card.
while True:
action = input('(h (hit) or s (stand)?')
if action == 'h':
deal_card(player, deck)
player.print_current_cards()
elif action == 's':
player.print_current_cards()
break
if player.calc_card_value() > 21:
player.print_current_cards()
print(player.entity_name + ' busts')
bust = True
break
if bust:
player.deposit(-bet)
#computers turn if the user decides to stand
else:
while house.calc_card_value() < 17:
deal_card(house, deck)
house.print_current_cards()
if not bust:
check_winner(player, house, bet)
print(f'{player.entity_name} you now have {player.bet_account} in your account')
def main():
"""Initial setup. Gets player name and how much they wants to deposit, starts game """
print()
name = input('What is your name?').capitalize()
if name == "":
print("You need to type a name")
main()
try:
money = int(input('How much do you want to deposit?'))
if money <= 0:
print("deposit must be bigger than 0, starting over..")
main()
except ValueError:
print("Not a valid deposit. Starting over..")
main()
#creates objects of player and house
player = Entity(bet_account = money, entity_name = name)
house = Entity(bet_account = 10000000, entity_name = "House")
stop = False
while not stop:
deck = generate_deck()
player.cards = []
house.cards = []
try:
bet = int(input('How much do you want to bet?'))
if bet <= player.bet_account and bet > 0:
# starts the game
play_game(player,house,deck,bet)
else:
print("Bet cannot be bigger than what you have, and cannot be 0")
except ValueError:
print("Not a valid bet")
want_to_stop = input('To stop write ̈s ̈, to try again press enter')
if want_to_stop == "s":
stop = True
if __name__ == '__main__':
main()
1 Answer 1
Card:
Consider renaming
card_value
to justvalue
. Since it belongs to an instance of theCard
class, there is no point in restating the "card" part.What purpose does the
suits
list of name-value pairs serve? It seems it's only used once in your script. In fact, it doesn't make sense to associate a suit with a number, especially since you just rely on indices later ingenerate_deck
and never actually use the number portion of the pairs.Consider changing the
values
list to a dictionary that maps a numerical value to its corresponding string name. For example:
values = { 1: 'Ace', 2: 'Two', 3: 'Three', 4: 'Four', ... }
This would eliminate your reliance on passing in indices to initialize a card with a particular value, since that logic is a bit convoluted as it requires decrementing a card's value by one (or, in the case of Ace = 11, simply doesn't make sense).
Refactor your code for this accordingly.
Entity:
Why not rename this to
Player
? In game dev, you usually only give something the generic name ofEntity
if that class is going to be an abstract class. Otherwise, you could argue that anything is an entity, including a card. Technically, the house is also aPlayer
.You are currently using
deposit
for both positive and negative values. That works and is fine from a technical standpoint, but it's not from a logical one. Consider using two separate methods,deposit
andwithdraw
, that serve opposite purposes.calc_card_value
isn't informative—what you're really doing is computing ahand_value
. Moreover, your logic/efficiency can be simplified quite a bit, especially if you use my dictionary suggestion above forCard
:
total_value = 0
for card in self.cards:
if total_value > 21 and card.value == 'Ace':
total_value -= 10
else:
total_value += card.value
return total_value
This eliminates unnecessarily looping over the cards twice and is a bit more compact. A downside/tradeoff is that you're checking the conditional on each iteration.
EDIT: This is a classic example of trying to optimize something and breaking the code in the process. I am wrong—you do need two loops here. My code doesn't do what you'd expect.
Instead, to keep the code more compact while still using two loops, I'd use a list comprehension:
total_value = sum([card.value for card in self.cards])
if total_value > 21:
for card in self.cards:
if card.card_value == "Ace":
total_value -= 10
if total_value <= 21:
break
Global methods
generate_deck
is good.deal_card
makes more sense as part of aBlackjackGame
class that manages the game (more on this later/throughout the rest of the review).Let's take a look at
check_winner
:a. The logic would be much cleaner if this were part of a class like
BlackjackGame
that has members for the house and the player.b. You should define a class constant in
BlackjackGame
for the "magic number" of 21:BLACKJACK = 21
. That way, everyone can reference it asBlackjackGame.BLACKJACK
(however, this isn't that big of a deal; 21 is well known as Blackjack in... Blackjack!).play_game
would also be much simpler if it were part ofBlackjackGame
. You could then just rename it toplay
, deal the cards to the membersself.house
andself.player
, and so on.
The code is also a bit long for this function, mainly because you're doing two things: taking input for the player, and then taking "input" for house/the computer. Consider splitting those two code segments into their own methods and calling them here one after the other. The method for the player's scenario could return a boolean indicating whether the player bust or not. So something like:
#deals 2 cards to the player, and one for the dealer
deal_card(house, deck)
deal_card(player, deck)
deal_card(player, deck)
#prints the current card on the table
player.print_current_cards()
house.print_current_cards()
#runs player and house scenarios
player_bust = self.determine_player_outcome()
if player_bust:
player.withdraw(bet)
else:
cycle_dealing_to_house()
self.check_winner(bet)
Notice how your conditional for if not bust
was also redundant—that's already covered by the else
after if bust
.
Main
Your logic for getting the player's name is really convoluted. You're calling main from within main... Don't you end up with two instances of the game running? Instead, do this:
a. Split this into a global function called
get_player_name()
:while True: name = input('What is your name?').capitalize() if name is not "": return name else: print("You need to type a name!")
Then, main doesn't need to care about these details anymore; it just calls
get_player_name
and is guaranteed to either cycle the print statement or get a valid name and continue.Same goes for depositing money—create a function named
ask_player_to_deposit_money
or something shorter, and run a similar loop inside its body. Then call it inside main and store the return value in a variable.Remember to create a
BlackjackGame
class and consolidate the player/house construction in there.The rest of the code is fine.
Testing
I recommend looking into the unittest
framework for Python. Unit testing allows you to isolate functionally independent parts of a program and to test them separately from the rest to ensure that they function as intended.
You can of course also test the game manually by playing it, but it would be tedious to run through every scenario by hand.
-
\$\begingroup\$ in the case of Ace = 11, simply doesn't make sense: I believe that this is so that comparators (
>
,<
, ...) give the ace the highest in-game value. \$\endgroup\$PiCTo– PiCTo2019年05月27日 10:12:49 +00:00Commented May 27, 2019 at 10:12 -
1\$\begingroup\$ Oh, no, to clarify, what I meant is that using indices to get the card values does not consistently make sense. For values 2-10, it's a matter of
values[n-1] = card with value of n
. But the Ace is an exception to this rule, so I wanted to emphasize that a map would make this even clearer. \$\endgroup\$Aleksandr Hovhannisyan– Aleksandr Hovhannisyan2019年05月27日 13:54:52 +00:00Commented May 27, 2019 at 13:54
Explore related questions
See similar questions with these tags.