Code Review Asked by fames on December 8, 2020
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()
Consider renaming card_value to just value. Since it belongs to an instance of the Card 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 in generate_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.
Why not rename this to Player? In game dev, you usually only give something the generic name of Entity 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 a Player.
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 and withdraw, that serve opposite purposes.
calc_card_value isn't informative—what you're really doing is computing a hand_value. Moreover, your logic/efficiency can be simplified quite a bit, especially if you use my dictionary suggestion above for Card:
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
generate_deck is good.
deal_card makes more sense as part of a BlackjackGame 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 as BlackjackGame.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 of BlackjackGame. You could then just rename it to play, deal the cards to the members self.house and self.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.
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.
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.
Correct answer by AleksandrH on December 8, 2020
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP