2
\$\begingroup\$

I built a very basic blackjack program a couple of weeks ago, but I've learned a lot since then, and would like some input on a couple of basic functions for card games: a build_deck function and a draw_hand function.

It seems pretty efficient use of code (at least compared to my previous attempt) but I'm sure there are some ways I can improve on it.

import random

def build_deck():
# The deck is a list of tuples, with each tuple representing a card.
# Each card has a name, face, suit, and value.
    deck = []
    value = 0
    face = ''
    for value in range(13):
        value += 1
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:
            if value == 1:
                face = 'Ace'
            elif value == 11:
                face = 'Jack'
            elif value == 12:
                face = 'Queen'
            elif value == 13:
                face = 'King'
            else:
                face = str(value)
            name = 'the %s of %s' % (face, suit)
            # The deck is shuffled only once, when the deck is first built.
            random.shuffle(deck)
            deck.append((name, face, suit, value))
    return deck

def draw_hand(hand_size):
    # The player receives a card into their hand while the same card is removed from the deck.
    player_hand = []
    for draw in range(int(hand_size)):
        selected_card = (random.choice(deck))
        print('You drew %s.' % (selected_card[0]))
        player_hand.append(selected_card)
        deck.remove(selected_card)
    return player_hand

deck = build_deck()
player_hand= draw_hand(input('How many cards do you want to draw? There are %s cards in the deck.' % (str(len(deck)))))
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Shuffling the Deck

Your comment # The deck is shuffled only once, when the deck is first built is wrong. You are shuffling the deck 52 times; once after before each card is added:

    deck = []
    for value in range(13):
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:
            ...
            random.shuffle(deck) # This line is executed 13 * 4 times!
            deck.append((name, face, suit, value))
    return deck

Additionally, you are grabbing cards from a random location in the middle of the deck, so it doesn't matter if you shuffled the deck or not:

        selected_card = (random.choice(deck))
        ...
        deck.remove(selected_card)

You should build the complete deck, and then shuffle it once:

    deck = []
    for value in range(13):
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:
            ...
            deck.append((name, face, suit, value))

    random.shuffle(deck)
    return deck

And then, deal from the top of the deck:

        selected_card = deck.pop(0)

Suits

    for value in range(13):
        value += 1
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:

The suit_list doesn't change. It doesn't need to be recreated for each of the 13 card ranks. You should move it out of the inner loop.

There it is still a local variable. Code which wants to use your deck of cards might want/need to know the what all the suits in your deck are. Is it a normal playing card deck, or a deck of Tarot cards? Is the spade suit represented as "Spades", "SPADES", "S", "♠" or "♤"?

An enum would be a better entity to use as a card suit:

from enum import Enum

Suit = Enum('Suit', 'Clubs, Diamonds, Hearts, Spades')

Then a program could safely refer to the spade suit as Suit.Spades, and iinstead of:

        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:

You'd simply iterate over the Suit enum:

        for suit in Suit:

You can still use suit.name to get a nice string for the suit.

range(start, end, step)

Instead of writing:

    for value in range(13):
        value += 1
        ...

use the fact that a range can start at any value, so you don't need to add 1 to the card's rank at each iteration; just iterate over a different range:

    for value in range(1, 13+1):
        ...

Each tuple represents a card ...

Ok ... lemme think. The card[0] is the card's rank, and card[1] is the card's suit? Or do have have that backwards ; card[0] is the suit and card[1] is the rank?

With a tuple, it is easy to forget which member is stored in which field. It is way easier to use a named tuple:

from collections import namedtuple

Card = nametuple('Card', 'name, face, suit, value')

Then we wouldn't have to remember; card.rank is the the card's rank, and card.suit is the card's suit.

The only downside is ... uhh ... no, sorry, there are no downsides. A namedtuple is just as efficient time and space wise. You'd create the cards like this:

        deck.append(Card(name, face, suit, value))

And instead of

    print('You drew %s.' % (selected_card[0]))

you'd write:

    print('You drew %s.' % selected_card.name)

Formatting

The inner parenthesis are unnecessary here:

    print('You drew %s.' % (selected_card[0]))

They take the value "the Ace of Spaces", and ... return that string unaltered. So we end up with the expression str % str, and since the first string contains only one %s code, the argument is directly used.

if you had used (selected_card[0], ), that would have constructed a tuple of 1 values, a string, which could also be applied to that format string. Without the trailing comma, you don't have a tuple.

It is easier to use f-strings. There, the format arguments are placed directly in the format codes, instead of at the end where they have to be match up by positions. Instead of:

            name = 'the %s of %s' % (face, suit)

you could write:

            name = f'the {face} of {suit}'

which is slightly more compact, and much easier to tell where the values are going in the resulting string.

Hand size

def draw_hand(hand_size):
    player_hand = []
    for draw in range(int(hand_size)):

Wait ... what is that int(...) doing there? What are you expecting to pass to draw_hand, if not an integer? Why would you allow it to be a string?

It should be the caller's responsibility for any string to integer conversions. This function should only expect an integer for hand_size.

Improved code

Using NamedTuple instead of namedtuple, a Rank enumeration, and reducing Card to just a tuple of rank and suit, with str(card) corresponding to the card's name, and card.rank.value for the card's value ... along with some other structural modifications:

from random import shuffle
from enum import Enum
from typing import NamedTuple, List
from itertools import product

Suit = Enum('Suit', 'Clubs, Diamonds, Hearts, Spades')
Rank = Enum('Rank', 'Ace, 2, 3, 4, 5, 6, 7, 8, 9, 10, Jack, Queen, King')

class Card(NamedTuple):
    rank: Rank
    suit: Suit

    def __str__(self):
        return f"{self.rank.name} of {self.suit.name}"

def build_deck() -> List[Card]:
    deck = [Card(rank, suit) for rank, suit in product(Rank, Suit)]
    shuffle(deck)
    return deck

def draw_hand(hand_size: int) -> List[Card]:
    return [deck.pop(0) for _ in range(hand_size)]

def print_hand(hand: List[Card]) -> None:
    print("You drew:", ", ".join(str(card) for card in hand))

if __name__ == '__main__':
    deck = build_deck()
    n = int(input(f"How many cards? There are {len(deck)} cards in the deck: "))
    hand = draw_hand(n)
    print_hand(hand)

Result:

How many cards? There are 52 cards in the deck: 5
You drew: 2 of Hearts, 9 of Clubs, 5 of Hearts, 7 of Hearts, Queen of Clubs

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for providing the enum and namedtuple code, I haven't seen those before and will read up on them. As far as the int for hand size, I had thought that input() always returned a string, but I might have been wrong in that regard. \$\endgroup\$
    – Joel V
    Apr 20, 2020 at 2:14
  • \$\begingroup\$ input() does return a string, which is why I have int(input)). The point is draw_hand() shouldn’t handle hand_size given as a string. It should expect an integer. \$\endgroup\$
    – AJNeufeld
    Apr 20, 2020 at 5:37
  • 1
    \$\begingroup\$ Using an IntEnum or OrderedEnum, enables card ranks to be compared. Because the Rank Enum uses numbers for member names, the ranks should be accessed using Rank['4'] or Rank['Queen']. While Rank.Queen works, Rank.4 will give a syntax error. \$\endgroup\$
    – RootTwo
    Apr 20, 2020 at 18:36
  • \$\begingroup\$ @RootTwo If you have a number, and need the corresponding card Rank, you should use Rank(4), not Rank['4'], to get the Rack enum who's value is 4. Ordering cards by rank was not important in the OP's previous post (Blackjack), so I stopped before introducing OrderedEnum. It also opens a can or worms about whether ace's should be high or low, and perhaps ordering should be imposed by the game played with the cards and not by the cards themselves. \$\endgroup\$
    – AJNeufeld
    Apr 20, 2020 at 19:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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