8
\$\begingroup\$

I am new to programming in Python and wrote a simple Blackjack project. I am looking for experienced peers to provide a high level code review about the overall design patterns and proper usages. The code performs as expected and passes all my unit tests so I am mainly looking for feedback on how to make the code more clean, modular, and conforming to some of the most common best practices. I am especially nervous about all the variables I am passing and the use (or misuse) of global variables.

P.S. On a side note, for the blackjack players out there I did not implement the "double down" feature yet.

import sys
import os
import random

deck = None
stats = {'player':{'win':0,'lose':0,'tie':0,'blackjack':0},'dealer'{'win':0,'lose':0,'tie':0,'blackjack':0}}
history = {'bets':[]} #use later to log playing history

def NewBlackjackGame():
    player = Player("Player 1")
    dealer = Player("Dealer")
    game = Game(player, dealer,[])
    round_number = 1
    while player.credits > 1:
        print('### Round ' + str(round_number) + ' ###')
        game.play_round()
        round_number = round_number + 1
    if player.credits < 1:
        print('You are out of credits. Game over.')

class Hand():
    def __init__(self, owner):
        self.owner = owner
        self.cards =[]
        self.values=[]
        self.total = self.get_total()

    def show_hand(self,dealer_turn):
        if self.owner == "Player 1":
            print(self.owner + ' current hand: ' + str(self.cards) + ' for a total of: ' + str(self.get_total()))
        if self.owner == "Dealer" and dealer_turn==0:
            print('Dealer shows: ' + self.cards[0] + ' and <card face down>') #don't show the card in the hole
        if self.owner == "Dealer" and dealer_turn==1:
            print(self.owner + ' current hand: ' + str(self.cards) + ' for a total of: ' + str(self.get_total()))

    def draw_card(self):
        global deck
        new_card = deck.draw()
        self.cards.append(new_card)
        self.values.append(deck.values_lookup[new_card])
        #automatically take care of the "soft" "hard" business
        if "A" in self.cards:
            self.adjust_ace_value()
        self.total = self.get_total()

    def adjust_ace_value(self):
        global deck
        total_of_non_ace_cards = sum(deck.values_lookup[i] for i in self.cards if i != 'A')
        if total_of_non_ace_cards <= 10:
            self.values[self.cards.index('A')]=11
        else:
            self.values[self.cards.index('A')]=1

    def clear_hand(self):
        del self.cards[:]
        del self.values[:]

    def get_total(self):
        return sum(c for c in self.values)

class Game():
    def __init__(self, player, dealer, stats):
        self.player = player
        self.dealer = dealer
        self.stats = stats

    def hit_or_stand(self):
        #choice = raw_input('Continue or stop? You have a ' + str(self.get_bust_probability(self.player.hand,self.dealer.hand)) + ' percent probability of busting') #use this later when I help the player if they need help
        choice = raw_input('Press any key to Hit, or "s" to [s]tand > ')
        if choice == "s":
            return 0
        else:
            return 1

    def increment_stats(self,player,cat):
        global stats
        if player == 'player' and cat == 'win':
            stats['player']['win'] = stats['player']['win'] +1
            stats['dealer']['lose'] = stats['dealer']['lose'] +1
        if player == 'player' and cat == 'lose':
            stats['player']['lose'] = stats['player']['lose'] +1
            stats['dealer']['win'] = stats['dealer']['win'] +1
        if player == 'player' and cat == 'blackjack':
            stats['player']['blackjack'] = stats['player']['blackjack'] +1
            stats['dealer']['lose'] = stats['dealer']['lose'] +1
        if player=='dealer' and cat == 'blackjack':
            stats['player']['lose'] = stats['player']['lose'] +1
            stats['dealer']['blackjack'] = stats['dealer']['blackjack'] +1
        if player == 'player' and cat == 'tie':
            stats['player']['tie'] = stats['player']['tie'] +1
            stats['dealer']['tie'] = stats['dealer']['tie'] +1
    def play_round(self):
        global deck
        global history
        deck = Deck()
        deck.shuffle()
        self.player.hand.clear_hand()
        self.dealer.hand.clear_hand()
        initial_bet = 0
        dealer_turn =0 #is it the dealer's turn?
        hit = None # 1 is player and 0 is dealer
        winner = None# -1 for dealer, 1 for player, 0 for tie
        ##player turn##
        while initial_bet < 1 or initial_bet > self.player.credits:
            try:
                initial_bet = int(raw_input('How much would you like to bet? You have ' + str(self.player.credits) + ' credits. '))
                if initial_bet < 1:
                    print('Please bet at least 1 credit')
                if initial_bet > self.player.credits:
                    print('You do not have sufficient credits to make this wager. You have ' + str(self.player.credits) + ' credits left.')
        except ValueError:
                print('That was an invalid number. Please enter a value >= 1')  
        print('You bet ' + str(initial_bet))
        self.player.change_credits(-initial_bet)
        history['bets'].append(initial_bet)

    for i in range(2):
        self.player.hand.draw_card()
        self.dealer.hand.draw_card()
    self.player.hand.show_hand(dealer_turn)
    self.dealer.hand.show_hand(dealer_turn)
    if self.player.hand.total <  21:
        hit = self.hit_or_stand()
    if self.player.hand.total == 21:
        print('Player Blackjack!')
        self.increment_stats('player', 'blackjack')
        self.player.change_credits(initial_bet*2.5) #3:2 returns for blackjack
        winner = 1

    while self.player.hand.total < 21 and hit and winner == None:
        self.player.hand.draw_card()
        self.player.hand.show_hand(dealer_turn)
        if self.player.hand.total > 21:
            print('Player bust!')
            self.increment_stats('player', 'lose')
            winner = -1
            break
        hit = self.hit_or_stand()
    #player stands

    if hit == 0 and winner == None: 
        print('Player stands. Dealer turn')
        dealer_turn = 1
        self.dealer.hand.show_hand(dealer_turn)
        #two cases where dealer wins/ties right away
        if self.dealer.hand.total == 21 and self.player.hand.total < 21:
            print('Dealer Blackjack!')
            self.increment_stats('dealer', 'blackjack')
            winner = -1
        if self.dealer.hand.total == 21 and self.player.hand.total == 21 and len(self.player.hand.card) ==2:
            print('Push! You have tied. You will get back your initial wager.')
            self.player.change_credits(int(initial_bet))
            self.increment_stats('player', 'tie')   
            winner = 0
        if self.dealer.hand.total > 17 and self.dealer.hand.total > self.player.hand.total:
            print('Dealer wins!')
            self.increment_stats('player', 'lose')
            winner = -1
        #if not keep playing...
        while self.dealer.hand.total < 17 and winner == None:
            print('Dealer draws card...')
            self.dealer.hand.draw_card()
            self.dealer.hand.show_hand(dealer_turn)
        if self.dealer.hand.total < 21 and winner == None:
            if self.dealer.hand.total > self.player.hand.total:
                print('Dealer wins!')
                self.increment_stats('player', 'lose')
                winner = -1
            if self.dealer.hand.total == self.player.hand.total:
                print('Push! You have tied. You will get back your initial wager.')
                self.player.change_credits(int(initial_bet))
                self.increment_stats('player', 'tie')
                winner = 0
            if self.dealer.hand.total < self.player.hand.total:
                print('Player 1 wins!') 
                self.player.change_credits(2*int(initial_bet))
                self.increment_stats('player', 'win')
                winner = 1
        if self.dealer.hand.total>21 and winner == None:
            print('Dealer bust. Player wins!')
            self.player.change_credits(2*int(initial_bet))
            self.increment_stats('player', 'win')
            winner = 1
    print('Your current credit is: ' + str(self.player.credits))

def get_bust_probability(self,player_hand,dealer_hand):
    global deck
    margin = 21 - player_hand.total
    deck.card_values.append(deck.values_lookup[dealer_hand.cards[1]]) #we need to put back the dealer's hidden card since we cannot account for it in the probabilities
    over_margin = len([c for c in deck.card_values if c > margin]) 
    deck.card_values.remove(deck.values_lookup[dealer_hand.cards[1]]) #remove the dealer's hidden card that we had inserted to compute accurate probabilities
    return round((over_margin/len(Deck().cards))*100.0)

class Deck():
    def __init__(self):
        self.values_lookup = {'A':1,'2':2,'3':3,'4':4, '5':5,'6':6,'7':7,'8':8,'9':9,'10':10,'J':10,'K':10,'Q':10}
        self.cards = list(self.values_lookup.keys())*4
        self.card_values = list(self.values_lookup.values())*4

    def shuffle(self):
        random.shuffle(self.cards)

    def draw(self):
        self.card_values.remove(self.values_lookup[self.cards[0]]) #update the values list
        return self.cards.pop(0)

    def cards_left(self):
        return len(self.cards)

class Player():
    def __init__(self, name):
        self.credits = 100
        self.hand = Hand(name)

    def get_credits(self):
        return self.credits

    def change_credits(self,value):
        self.credits = self.credits + value

def main():
    NewBlackjackGame()

if __name__ == '__main__':
    main()
\$\endgroup\$
  • 1
    \$\begingroup\$ Couple minor points on blackjack rules - Dealer usually offers insurance for a visible ace, and I don't see a method for hitting a "soft" 17 (17 with an ace involved) for the dealer. \$\endgroup\$ – JohnP Jul 10 '14 at 21:56
  • \$\begingroup\$ While being able to support a rule such as "dealer must hit soft 17" is certainly a good thing, it is merely a variant rule. It is a very common rule, but by no means universal. Offering insurance when dealer shows an ace is universal, though. There are bigger problems with the rules in this program; for example, a player total of 21 should not be blackjack and should not pay 3:2 unless it is the player's starting hand (he has not hit). The program also does not permit doubling down or splitting, though the latter would greatly complicate the program. \$\endgroup\$ – Kef Schecter Mar 19 '15 at 9:43
  • \$\begingroup\$ Since nobody else mentioned it, I've noticed that "== None" appears a few times in the program. The preferred way to write it is "is None". The "is" operator is used because None is supposed to be a singleton. \$\endgroup\$ – Kef Schecter Mar 19 '15 at 9:51
7
\$\begingroup\$

You should follow the style guide. I was thinking that NewBlackjackGame was an odd name for a class, but it turns out it's a function; it should therefore be new_blackjack_game. (That being said, I think all of the logic in that function should really be in Game, see below.)


There is no point having a main that just calls one other function. Either rename that function to main, or call the function directly if __name__ == "__main__":.


You only need to hold the values for face cards in the values_lookup, which should be a class attribute (it's the same for all cards/decks). The card's value can just be an integer for non-face cards, then you can dict.get either the face card value or use it directly. For example:

class Card():

    FACES = {'Ace': 1, 'Jack': 10, 'Queen': 10, 'King': 10}

    def __init__(self, face, suit):
        self.face = face
        self.suit = suit

    def __str__(self):
        return "{0.face} of {0.suit}".format(self)

    @property
    def value(self):
        return self.FACES.get(self.face, self.face)

Note the use of @property, which is a Pythonic way to implement the getters and setters used in other languages. In use:

>>> c = Card("Ace", "spades")
>>> str(c)
'Ace of spades'
>>> c.value
1
>>> c = Card(9, "diamonds")
>>> str(c)
'9 of diamonds'
>>> c.value
9

As already pointed out, don't hard-code the name of the Players. If you keep the Dealer separate, you can have as many others as you like:

class Game():

    def __init__(self, *players, start_credit=100):
        self.dealer = Dealer()
        self.deck = Deck()
        self.players = [Player(player, start_credit) for player in players]
        ...

Then play like:

game = Game("Anna", "Bob", "Chris")
game.play() # replaces `NewBlackjackGame`

I think the main issue is having logic in odd places. I would suggest a structure like the following:

  • Card - holds the value and suit of an individual card;
  • Deck - holds the cards and the logic for shuffling and dealing;
  • Hand - another collection of cards, with the logic for adding up scores. The hand doesn't need to know its owner, the logic for which Cards in the Hand to show should be in the Player. It shouldn't access a global deck - implement e.g. def draw_from(self, deck):;
  • Player - a player has a Hand of Cards, the input logic (hit_or_stand, which should incorporate validation), the rules for showing the cards, their credit and stats, etc.;
    • Dealer - a subclass of Player holding the dealer's playing logic and the different rules for showing its hand;
  • Game - holds the deck and the players and the logic for progressing through the game, including e.g. history, which should be Game instance attributes rather than global variables.

Even within the existing classes, your logic is all over the place. Consider this simplified implementation of Hand:

class Hand():

    def __init__(self):
        # initialise the empty list of cards

    def draw_from(self, deck):
        # draw a card from the deck and add to list

    def return_to(self, deck):
        # empty list of cards and return to deck

    @property
    def value(self):
        # all the value logic goes here

Now there is only one attribute the Hand instance needs - the cards it holds. You don't have to do everything in value (you could have "private" _helper_functions), but you don't need to recalculate a total or hold a separate list of values.

Here is a rough UML diagram showing what I mean:

UML class diagram showing relationships

\$\endgroup\$
4
\$\begingroup\$

Alright, I myself am a Python beginner too. So I'll see what I can review and leave the rest to someone with more experience!

First thing I notice, is how much easier reading your code would be if the classes were in separate files. It'd be much more modular, and in fact it would help separate different parts of the code.

I'll break this up into section based on class/function.

NewBlackjackGame()

  • I noticed you set the name of your player to "Player 1". Do you intend on having more than one player (besides the dealer)? If so, the rest of your code will need to depend on the addition of players to the game, which is a whole new level!
  • After your while loop, which only loops if the player has enough credits, you call this:

    if player.credits < 1:
    

    I find this redundant because the code wouldn't have gotten there if it the player had more than 1 credit!

Hand()

  • Here's a flaw if you're looking to expand the allowed player amount:

    if self.owner == "Player 1":
    

    Can you always count on that being the player's name? Probably not. I say ditch the names and owner strings, or allow for compatibility.

  • In draw_card(), I see your run in with globals. I too am not 100% on how to handle these in Python, but if it's like other languages I know, there's a time and place for them, and this doesn't look like either! I don't see why you can't pass the deck as a parameter for either the function or the class.
  • Regarding your ace checking in adjust_ace_value(), I know this will sound stupid, but let the player choose if they want 1 or 11. They may have some crazy strategy and having the value assigned may just mess them up!
  • I think you are repeating a bit of code here. I see you counting up the cards for total_of_non_ace_cards and again in get_total(). The only difference being, the first one excludes aces. I suppose you could add a parameter in get_total() to exclude a card type, and that way you can use the function for total_of_non_ace_cards.

Game()

  • I do think you could solve the problem of having globals by passing an appropriate parameter. I'm sorry I don't exactly have the solution! I know there's a way though!
  • I suggest extending cat into category. I don't see a reason to shorten it. It only makes things harder to skim.
  • Before all of the conditionals in increment_stats(), I suggest you add something such as if player != 'player':. This way you can eliminate all those checks in the following ifs.

Overall, I think your code is easy to read, and from what I can see, it follows standards. Up next you'll have to make a GUI and add the actual suite names! :)

\$\endgroup\$
4
\$\begingroup\$

This list is not exhaustive:

  • There is no need for global
  • There is confusion between (for example) global stats and Game.stats
  • You need docstrings throughout
  • NewBlackjackGame is a confusing name
  • In NewBlackJackGame the cases player.credits > 1 and < 1 are handed but not player.credits == 1
  • It is not obvious that increment_stats covers all cases (or even what the cases are) and which stats is being used
  • Game.play_round is so complicated that I can't tell what it is doing or attempting to do
  • Why do Deck.cards and Deck.card_values both exist?
  • is_dealer_turn and similar should be booleans not integers
  • get_bust_probability is dead code (maybe?)
  • Is there a difference between sum(self.values) and sum(c for c in self.values)?

I recommend finding any other implementation of blackjack (or a couple of them) and see how they are structured. It appears you've bitten off more than your current skills afford.

\$\endgroup\$
  • 1
    \$\begingroup\$ I upvoted this but removed my vote on reading the last sentence. How is @Rich to improve his programming skills without "biting off more than [his] current skills afford"? I have found that experimenting outside of my comfort zone and skill level is the best way to learn. \$\endgroup\$ – Adam Jun 7 '16 at 23:19

protected by Simon Forsberg Jun 17 '16 at 14:16

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.