13
\$\begingroup\$

I've only been learning Python for a few days after a Humble Bundle book sale, but I made a functional Blackjack game on which I'd like some constructive criticism as far as my coding structure and any suggestions for improvement from more experienced coders.

Edit: This is coded in Python 3.8.2

import random
import time

deck = []
firstDraw = 0
usedCards = []
playerHand = []
playerHandValue = 0
dealtCard1 = ''
dealtCard1Number = 0
dealtCard2 = ''
dealtCard2Number = 0
dealerHand = []
dealerHandValue = 0



def dealHand():

    global deck
    global playerHand
    global playerHandValue
    global dealerHand
    global dealerHandValue
    global firstDraw
    firstDraw = 0
    # The starting deck is created #
    deck = ['2sp', '3sp', '4sp', '5sp', '6sp', '7sp', '8sp', '9sp', '10sp', 'Jsp', 'Qsp', 'Ksp', 'Asp', '2cl', '3cl', '4cl', '5cl', '6cl', '7cl', '8cl', '9cl', '10cl', 'Jcl', 'Qcl', 'Kcl', 'Acl', '2he', '3he', '4he', '5he', '6he', '7he', '8he', '9he', '10he', 'Jhe', 'Qhe', 'Khe', 'Ahe', '2di', '3di', '4di', '5di', '6di', '7di', '8di', '9di', '10di', 'Jdi', 'Qdi', 'Kdi', 'Asp']

    playerHand = []
    playerHandValue = 0
    dealerHand = []
    dealerHandValue = 0

# Two cards are dealt to the player #
    dealtCard1Number = random.randint(0, len(deck)-1)
    playerHand.append(deck[dealtCard1Number])
    del deck[dealtCard1Number]

    dealtCard2Number = random.randint(0, len(deck)-1)
    playerHand.append(deck[dealtCard2Number])
    del deck[dealtCard2Number]

# Two cards are dealt to the dealer #
    dealerCard1Number = random.randint(0, len(deck)-1)
    dealerCard1 = deck[dealerCard1Number]
    dealerHand.append(dealerCard1)
    del deck[dealerCard1Number]

    dealerCard2Number = random.randint(0, len(deck)-1)
    dealerCard2 = deck[dealerCard2Number]
    dealerHand.append(dealerCard2)
    del deck[dealerCard2Number]



# The player's starting hand is revealed to the player #
    print('\n' + 'Your current hand is ' + str(playerHand) + '\n')
    time.sleep(1)
    findHandValue()

def findHandValue():
    global playerHand
    global playerHandValue
# Resets the player's hand value to 0 for new deals #
    playerHandValue = 0
# The value of the player's cards is determined #
    if '2sp' in playerHand:
        playerHandValue = playerHandValue + 2
    if '3sp' in playerHand:
        playerHandValue = playerHandValue + 3
    if '4sp' in playerHand:
        playerHandValue = playerHandValue + 4
    if '5sp' in playerHand:
        playerHandValue = playerHandValue + 5
    if '6sp' in playerHand:
        playerHandValue = playerHandValue + 6
    if '7sp' in playerHand:
        playerHandValue = playerHandValue + 7
    if '8sp' in playerHand:
        playerHandValue = playerHandValue + 8
    if '9sp' in playerHand:
        playerHandValue = playerHandValue + 9
    if '10sp' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Jsp' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Qsp' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Ksp' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Asp' in playerHand:
        playerHandValue = playerHandValue + 11
    if '2cl' in playerHand:
        playerHandValue = playerHandValue + 2
    if '3cl' in playerHand:
        playerHandValue = playerHandValue + 3
    if '4cl' in playerHand:
        playerHandValue = playerHandValue + 4
    if '5cl' in playerHand:
        playerHandValue = playerHandValue + 5
    if '6cl' in playerHand:
        playerHandValue = playerHandValue + 6
    if '7cl' in playerHand:
        playerHandValue = playerHandValue + 7
    if '8cl' in playerHand:
        playerHandValue = playerHandValue + 8
    if '9cl' in playerHand:
        playerHandValue = playerHandValue + 9
    if '10cl' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Jcl' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Qcl' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Kcl' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Acl' in playerHand:
        playerHandValue = playerHandValue + 11
    if '2he' in playerHand:
        playerHandValue = playerHandValue + 2
    if '3he' in playerHand:
        playerHandValue = playerHandValue + 3
    if '4he' in playerHand:
        playerHandValue = playerHandValue + 4
    if '5he' in playerHand:
        playerHandValue = playerHandValue + 5
    if '6he' in playerHand:
        playerHandValue = playerHandValue + 6
    if '7he' in playerHand:
        playerHandValue = playerHandValue + 7
    if '8he' in playerHand:
        playerHandValue = playerHandValue + 8
    if '9he' in playerHand:
        playerHandValue = playerHandValue + 9
    if '10he' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Jhe' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Qhe' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Khe' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Ahe' in playerHand:
        playerHandValue = playerHandValue + 11
    if '2di' in playerHand:
        playerHandValue = playerHandValue + 2
    if '3di' in playerHand:
        playerHandValue = playerHandValue + 3
    if '4di' in playerHand:
        playerHandValue = playerHandValue + 4
    if '5di' in playerHand:
        playerHandValue = playerHandValue + 5
    if '6di' in playerHand:
        playerHandValue = playerHandValue + 6
    if '7di' in playerHand:
        playerHandValue = playerHandValue + 7
    if '8di' in playerHand:
        playerHandValue = playerHandValue + 8
    if '9di' in playerHand:
        playerHandValue = playerHandValue + 9
    if '10di' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Jdi' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Qdi' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Kdi' in playerHand:
        playerHandValue = playerHandValue + 10
    if 'Adi' in playerHand:
        playerHandValue = playerHandValue + 11
# Allows Aces to convert from 11 points to 1 point if the hand value is over 21 #
    if playerHandValue > 21:
        if 'Asp' in playerHand:
            playerHandValue = playerHandValue - 10
        if playerHandValue > 21:
            if 'Acl' in playerHand:
               playerHandValue = playerHandValue -10
            if playerHandValue > 21:
                if 'Adi' in playerHand:
                   playerHandValue = playerHandValue -10
                if playerHandValue > 21:
                    if 'Ahe' in playerHand:
                         playerHandValue = playerHandValue -10
# Displays the player's hand value to the player #
    print("Player hand value = " + str(playerHandValue) + '\n')
    hitOrStay()


def hitOrStay():
    global dealtCard1
    global firstDraw
# The dealer's first card is revealed to the player #
    if firstDraw == 0:
        print('The dealer draws 2 cards and reveals ' + str(dealerHand[0]) + '\n')
        firstDraw = 1
        time.sleep(2)
# If the player's hand value is less than or equal to 21, the player has the choice to hit or stay #
    if playerHandValue <= 21:
        hitOrStayChoice = ''
        while hitOrStayChoice != 'hit' or 'stay':
            hitOrStayChoice = input('Do you \'hit\' or \'stay\'?' '\n')
            if hitOrStayChoice == 'hit':
                dealtCard1Number = random.randint(0, len(deck)-1)
                dealtCard1 = deck[dealtCard1Number]
                playerHand.append(dealtCard1)
                del deck[dealtCard1Number]
                print('You were dealt ' + dealtCard1)
                findHandValue()
            if hitOrStayChoice == 'stay':
                revealDealerHand()
# If the player's hand value is over 21, the player loses automatically #
    elif playerHandValue > 21:
        loseGame()

def revealDealerHand():
    global playerHand
    global playerHandValue
    global dealerHand
    global dealerHandValue

    dealerHandValue = 0
# The value of the dealer's cards is determined in the same manner as the player's cards #
    if '2sp' in dealerHand:
        dealerHandValue = dealerHandValue + 2
    if '3sp' in dealerHand:
        dealerHandValue = dealerHandValue + 3
    if '4sp' in dealerHand:
        dealerHandValue = dealerHandValue + 4
    if '5sp' in dealerHand:
        dealerHandValue = dealerHandValue + 5
    if '6sp' in dealerHand:
        dealerHandValue = dealerHandValue + 6
    if '7sp' in dealerHand:
        dealerHandValue = dealerHandValue + 7
    if '8sp' in dealerHand:
        dealerHandValue = dealerHandValue + 8
    if '9sp' in dealerHand:
        dealerHandValue = dealerHandValue + 9
    if '10sp' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Jsp' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Qsp' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Ksp' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Asp' in dealerHand:
        dealerHandValue = dealerHandValue + 11
    if '2cl' in dealerHand:
        dealerHandValue = dealerHandValue + 2
    if '3cl' in dealerHand:
        dealerHandValue = dealerHandValue + 3
    if '4cl' in dealerHand:
        dealerHandValue = dealerHandValue + 4
    if '5cl' in dealerHand:
        dealerHandValue = dealerHandValue + 5
    if '6cl' in dealerHand:
        dealerHandValue = dealerHandValue + 6
    if '7cl' in dealerHand:
        dealerHandValue = dealerHandValue + 7
    if '8cl' in dealerHand:
        dealerHandValue = dealerHandValue + 8
    if '9cl' in dealerHand:
        dealerHandValue = dealerHandValue + 9
    if '10cl' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Jcl' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Qcl' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Kcl' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Acl' in dealerHand:
        dealerHandValue = dealerHandValue + 11
    if '2he' in dealerHand:
        dealerHandValue = dealerHandValue + 2
    if '3he' in dealerHand:
        dealerHandValue = dealerHandValue + 3
    if '4he' in dealerHand:
        dealerHandValue = dealerHandValue + 4
    if '5he' in dealerHand:
        dealerHandValue = dealerHandValue + 5
    if '6he' in dealerHand:
        dealerHandValue = dealerHandValue + 6
    if '7he' in dealerHand:
        dealerHandValue = dealerHandValue + 7
    if '8he' in dealerHand:
        dealerHandValue = dealerHandValue + 8
    if '9he' in dealerHand:
        dealerHandValue = dealerHandValue + 9
    if '10he' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Jhe' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Qhe' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Khe' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Ahe' in dealerHand:
        dealerHandValue = dealerHandValue + 11
    if '2di' in dealerHand:
        dealerHandValue = dealerHandValue + 2
    if '3di' in dealerHand:
        dealerHandValue = dealerHandValue + 3
    if '4di' in dealerHand:
        dealerHandValue = dealerHandValue + 4
    if '5di' in dealerHand:
        dealerHandValue = dealerHandValue + 5
    if '6di' in dealerHand:
        dealerHandValue = dealerHandValue + 6
    if '7di' in dealerHand:
        dealerHandValue = dealerHandValue + 7
    if '8di' in dealerHand:
        dealerHandValue = dealerHandValue + 8
    if '9di' in dealerHand:
        dealerHandValue = dealerHandValue + 9
    if '10di' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Jdi' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Qdi' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Kdi' in dealerHand:
        dealerHandValue = dealerHandValue + 10
    if 'Adi' in dealerHand:
        dealerHandValue = dealerHandValue + 11
# this section is to allow Aces to convert from 11 points to 1 point if the hand value is over 21 #
    if dealerHandValue > 21:
        if 'Asp' in dealerHand:
            dealerHandValue = dealerHandValue - 10
        if dealerHandValue > 21:
            if 'Acl' in dealerHand:
               dealerHandValue = dealerHandValue -10
            if dealerHandValue > 21:
                if 'Adi' in dealerHand:
                   dealerHandValue = dealerHandValue -10
                if dealerHandValue > 21:
                    if 'Ahe' in dealerHand:
                         dealerHandValue = dealerHandValue -10
    # The dealer's hand is revealed #
    print('\n' + 'The dealer\'s hand is ' + str(dealerHand) + ' with a value of ' + str(dealerHandValue) + '\n')
    time.sleep(2)
    if dealerHandValue <= 16:
        dealerHit()
    if dealerHandValue > 16:
        dealerStay()

def dealerHit():
    global dealerHitCard1Number
    global dealerHitCard1
    global dealerHand
    global dealerHitCard1
    global dealerHitCard1Number
    dealerHitCard1Number = random.randint(0, len(deck)-1)
    dealerHitCard1 = deck[dealerHitCard1Number]
    dealerHand.append(dealerHitCard1)
    del deck[dealerHitCard1Number]
    print('The dealer hits and draws ' + dealerHitCard1)
    time.sleep(2)
    revealDealerHand()


def dealerStay():
    if playerHandValue <= dealerHandValue:
        if dealerHandValue <= 21:
            loseGame()
        if playerHandValue > 21:
            loseGame()
        if dealerHandValue >21 and playerHandValue <= 21:
            winGame()
    if playerHandValue > dealerHandValue:
        if playerHandValue <= 21:
            winGame()
        if playerHandValue >21:
            loseGame()

def loseGame():
    global playerHandValue
    if playerHandValue <= 21:
        print('You lose! Your hand value was ' + str(playerHandValue) + ', while the dealer\'s was ' + str(dealerHandValue) + '\n')
    elif playerHandValue > 21:
        print('You busted!' + '\n')
    askNewGame()

def winGame():
    global playerHandValue
    global dealerHandValue
    print('You won! Your hand value was ' + str(playerHandValue) + ', while the dealer\'s was ' + str(dealerHandValue) + '\n')
    newGame = ''
    while newGame != 'yes' or 'no':
        askNewGame()

def askNewGame():
    newGame = input('Do you want to play another game? \'Yes\' or \'No\'.')
    while newGame != 'yes' or 'no':
        if newGame == 'yes' or 'Yes' or 'y' or 'Y':
            dealHand()
    if newGame == 'no' or 'No' or 'n' or 'N':
        print('Goodbye!')


print('Welcome to Blackjack' + '\n')

input("Press Enter to deal your first hand.")
dealHand()
\$\endgroup\$
  • \$\begingroup\$ What version of python is this written in? \$\endgroup\$ – Linny Apr 5 at 15:46
  • \$\begingroup\$ It is Python version 3.8.2 \$\endgroup\$ – Joel V Apr 5 at 15:47
  • \$\begingroup\$ This might be useful: stackoverflow.com/questions/41970795/… \$\endgroup\$ – Right leg Apr 6 at 16:44
11
\$\begingroup\$

I'll try and address @Sam's second point since I agree with him. How you have it now is an incredible amount of work and is unsustainable. Lets break it down. Look at these chunks of code:

if '2sp' in playerHand:
    playerHandValue = playerHandValue + 2
if '3sp' in playerHand:
    playerHandValue = playerHandValue + 3
if '4sp' in playerHand:
    playerHandValue = playerHandValue + 4

. . .

if '2sp' in dealerHand:
    dealerHandValue = dealerHandValue + 2
if '3sp' in dealerHand:
    dealerHandValue = dealerHandValue + 3
if '4sp' in dealerHand:
    dealerHandValue = dealerHandValue + 4

There's multiple problems with this setup:

  • Really, the suit of the card doesn't matter. You only actually care about the value of the card.

  • Those two discrete chunks are nearly identical. The only difference between them is the first takes the player's hand and returns the player's total, and the second takes the dealer's hand and returns the dealer's total.

Lets deal with the first point, then expand it to fix the second.

Whenever you have nearly exactly the same code repeated over and over, you probably want to use a function and/or loop to reduce the duplication. I'd start with a function that takes a card and returns its value:

# This could be simplified since all values are the same
face_values = {"K": 10, "Q": 10, "J": 10}

def card_value(card):
    raw_value = card[:-2]  # Chop off the suit

    if raw_value in face_values:
        return face_values[raw_value]

    else:
        return int(raw_value)

>>> card_value("Ksp")
10

>>> card_value("8he")
8

Then, just loop:

player_hand_value = 0
for card in player_hand:
    player_hand_value += card_value(card)

There's still the problem though that you'd need to duplicate this code for the dealer. The solution is to make a function:

def hand_value(hand):
    total = 0
    for card in hand:
        total += card_value(card)

    return total

>> hand_value(["Ksp", "2he"])
12

Then just use that function:

def find_hand_value():
    global player_hand
    global player_hand_value

    playerHandValue = hand_value(player_hand)

    . . .

Then, to clean it up further:

  • revealDealerHand is also mostly the same as the player version. They could be generalized further by creating a function that handles the identical bits.

  • You'd probably want to represent cards using a cleaner method. Even a tuple of ("K", "Spade") would be better than what you have now. Needing to parse a string to gets its value as needed isn't very clean

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestion. I still have a lot to learn! \$\endgroup\$ – Joel V Apr 5 at 17:19
  • \$\begingroup\$ Why are you wrapping the face_values[raw_value] in a call to int if the dictionary holds integers? \$\endgroup\$ – RGS Apr 5 at 18:23
  • 1
    \$\begingroup\$ @RGS Oops. I did this review as a warm-up before I started working on an essay... but apparently I needed a warm-up before my warm-up. \$\endgroup\$ – Carcigenicate Apr 5 at 18:51
  • \$\begingroup\$ @Carcigenicate I just wanted to be sure I wasn't missing anything :P \$\endgroup\$ – RGS Apr 5 at 18:56
10
\$\begingroup\$

Use augmented assignments

Because people do things like var = var + updateValue Python supports writing this as var += updateValue. This means you don't have to write the name of the variable twice.

What is even cooler, this works for lots of binary operations, like var *= multiplier is the same as var = var * multiplier and var %= mod is the same as var = var % mod.

If you are copying and pasting your code, something is wrong

  • If you have code that is too repetitive, you can probably do it in a smarter way.

  • If you have duplicate code, you can probably factor it in a function.

I'm talking particularly about your if trains to update hand values. First of all, you should have a function that computes the value of a given hand and call it twice, instead of writing the trains of ifs twice; this reduces the probability of making a mistake!

After factoring your ifs into a function, you still have way too many if statements that are too similar... so there might be a better way to do it!

Some suggestions come to mind. Instead of checking if each card is in the hand, use a for loop to go over the cards in the hand:

for card in hand:
  pass

then, we need to check the value of each card. Note that the suit doesn't count for the value, so you can ignore the suits and only focus on the first character:

for card in hand:
  cardValue = card[0]
  # ...

and then check the value of the card in a smarter way. You could, for example, check if the card is a number or an ace; all the other cards are worth 10.

handValue = 0
for card in hand:
  cardValue = card[0] # a 10 will be "1"
  if cardValue in "23456789":
    handValue += int(cardValue)
  elif cardValue == "A":
    handValue += 11
  else:
    handValue += 10

if handValue > 21:
  # check if there are aces, etc

Probably in the first loop you can even count the aces, so that latter on it is easier to handle the case where the hand busts the 21 cap.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This assigns Q the value 11, K, the value 12, and A the value 13, so is completely wrong. Plus, it will fail when a 10 card is encountered with an Exception. \$\endgroup\$ – AJNeufeld Apr 5 at 16:37
  • \$\begingroup\$ @AJNeufeld you are most certainly right... I fixed that problem \$\endgroup\$ – RGS Apr 5 at 18:21
9
\$\begingroup\$

Two big ones:

  1. Don't use globals. There are lots of good reasons why globals are generally bad practice, and you can either read about them, you can discover them for yourself through painful experience over the course of years, or you can trust all the people who have been there and will tell you the same thing, and just nip that habit in the bud when you're starting out.
  2. Come up with an easier way to represent your cards. Having separate if statements to check the values of the 6 of spades vs the 6 of diamonds (and so on for every value and every suit) is a lot of unnecessary work.

Check out this answer I wrote a while back on representing a deck of cards in Python code; this is far from the only way to do it, but pay attention to how the suit is separated from the rank, and how simply defining all the possible suits and ranks lets us use the product function to generate a complete deck automatically. The goal is to give yourself less work to do and to not need to copy+paste lots of code to do relatively simple things.

Blackjack / 21 in Python3

| improve this answer | |
\$\endgroup\$
7
\$\begingroup\$

Dealing from the Middle of the Deck

    dealtCard1Number = random.randint(0, len(deck)-1)
    playerHand.append(deck[dealtCard1Number])
    del deck[dealtCard1Number]

Variations of this code is repeated many times.

First, you could simplify this code slightly by using random.randrange(len(deck)).

Second, it could be made into a function:

def deal():
    card_number = random.randrange(len(deck))
    card = deck[card_number]
    del deck[card_number]
    return card

And then using statements like:

    playerHand.append(deal())

But selecting and dealing a random card from the middle of the deck just seems wrong. You want to shuffle the deck once, and then deal cards from the top of the deck:

    deck = [...]
    random.shuffle(deck)

Then:

def deal():
    card = deck[0]
    del deck[0]
    return card

Or, as pointed out by MT_Head in the comments:

def deal():
    return deck.pop(0)

PEP-8

Follow the Python Style Guidelines in PEP-8. For instance, variables should be snake_case, not mixedCase.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The book I'm learning from has everything in mixed case/camel case, but I'll take your suggestion and start doing snake case with the underscores. \$\endgroup\$ – Joel V Apr 5 at 16:32
  • \$\begingroup\$ @JoelV While usually style is up to the individual, python is very strict about its styling conventions, and all python programmers follow it. If your book is not following these conventions, then it's a bit of a red flag for the value of the book. \$\endgroup\$ – Cruncher Apr 6 at 20:20
  • 1
    \$\begingroup\$ Try the built-in pop() method: card = deck.pop(index). It returns an item AND removes it from the list in one operation; index defaults to 0. \$\endgroup\$ – MT_Head Apr 7 at 2:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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