1
\$\begingroup\$

I wrote classes to model a playing card, a deck, and a player to be used in multiple playing card games. I originally posted a Card class here, and now extended the project by adding a deck and player class and am really looking for some feedback. This is my first real object oriented design project and I feel unsure about the current way the deck and player classes are implemented.

class Card(object):
    """Models a playing card, each Card object will have a suit, rank, and weight associated with each.

    possible_suits -- List of possible suits a card object can have
    possible_ranks -- List of possible ranks a card object can have
    Suit and rank weights are initialized by position in list.
    If card parameters are outside of expected values, card becomes joker with zero weight
    """

    possible_suits = ["Clubs", "Hearts", "Diamonds", "Spades"]
    possible_ranks = [2, 3, 4, 5, 6, 7, 8, 9, 10, "J", "Q", "K", "A"]

    def __init__(self, suit, rank):
        if suit in Card.possible_suits and rank in Card.possible_ranks:
            self.suit = suit
            self.rank = rank
            self.suit_weight = Card.possible_suits.index(suit)
            self.rank_weight = Card.possible_ranks.index(rank)
        else:
            self.suit = "Joker"
            self.rank = "J"
            self.suit_weight = 0
            self.rank_weight = 0

    def __str__(self):
        """Returns abbreviated name of card

        Example: str(Card('Spades', 'A') outputs 'AS'
        """
        return str(self.rank) + self.suit[0]

    def __eq__(self, other):
        """Return True if cards are equal by suit and rank weight"""
        return self.suit_weight == other.suit_weight and self.rank_weight == other.rank_weight

    def __gt__(self, other):
        """Returns true if first card is greater than second card by weight"""
        if self.suit_weight > other.suit_weight:
            return True
        if self.suit_weight == other.suit_weight:
            if self.rank_weight > other.rank_weight:
                return True
        return False

    def modify_weight(self, new_suit_weight = None, new_rank_weight = None):
        """Modifies weight of card object"""
        if new_suit_weight:
            self.suit_weight = new_suit_weight
        if new_rank_weight:
            self.rank_weight = new_rank_weight

    def get_suit(self):
        return self.suit

    def get_rank(self):
        return self.rank

    def get_suit_weight(self):
        return self.suit_weight

    def get_rank_weight(self):
        return self.rank_weight

from itertools import product
from random import shuffle

class Deck(object):
    """Models a deck of playing cards, each instance will be a list of 52 Card objects"""

    def __init__(self):
        self.cards = [Card(card[0], card[1]) for card in product(Card.possible_suits, Card.possible_ranks)]

    def __str__(self):
        return str([str(card) for card in self.cards])

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

    def draw(self, end = None):
        """Return a card object or a set of card objects from the zeroth index of the deck, while removing it"""
        card = self.cards[0: end]
        del self.cards[0: end]
        return card

    def shuffle_deck(self):
        """Shuffle the deck of cards in place"""
        shuffle(self.cards)

    def get_suit(self, suit):
        """Return all cards of suit in the deck"""
        cards_of_suit = []
        for card in self.cards:
            if card.get_suit() == suit:
                cards_of_suit.append(card)
            if len(cards_of_suit) == 13:
                break
        return cards_of_suit

    def find(self, card_to_find):
        """Return position of card in deck, if card not in deck return None"""
        print(type(card_to_find), "; ", card_to_find)
        if card_to_find in self.cards:
            return self.cards.index(card_to_find)
        return None

    def remove(self, card_to_remove):
        """Remove a card from the deck"""
        position = self.find(card_to_remove)
        if position:
            del self.cards[position]

class Player(object):
    """Models a Player in a playing card game, each instance will have an id,
    can be initialized with a hand of cards
    and can be initialized with a score
    """

    def __init__(self, id, hand = None, score = None):
        self.id = id
        self.hand = hand
        self.score = score

    def __str__(self):
        return str(self.id)

    def set_hand(self, cards):
        """Append cards to hand"""
        self.hand = cards

    def find_card(self, card):
        """Finds a card in hand"""
        if card in self.hand:
            return self.hand.index(card)
        return None

    def play(self, card_to_be_played):
        """Return card_to_be_played from hand while removing it, return None if card not in hand"""
        position = self.find_card(card_to_be_played)
        if position:
            del self.hand[position]
            return card_to_be_played
        return None

    def update_score(self, update):
        """Updates the current score of the instance by adding the update"""
        self.score += update

    def get_hand(self):
        return [str(i) for i in self.hand]

    def get_score(self):
        return self.score 
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

You may want to make the suits and ranks Enums, so that you can make cards in a more readable and less error prone way when needed:

Card(Suit.Spade, Rank.King)  # readable, clear
Card("Spade", "K")  # prone to typos 
Card(Card.possible_suits[0], Card.possible_ranks[11])  # meaning not very clear

It could also be beneficial to add a dict to your Deck implementation to more efficiently query the randomized deck:

# quickly retrieve cards of a suit
suitToCards = { 
    Suits.Spade: filter(card.suit == Suits.Spade, self.cards)
    # repeat for all suits...
} 

This way the Deck.cards are randomized, but Deck.bySuit can be used internally by Deck.get_suit() and Deck.find and Deck.remove so you don't need to iterate over the entire deck when you know you only ever need to work with a portion of the deck for these operations.

One more thing to consider is encapsulation of the cards in the deck. You've handled Deck.draw() well, but it looks like cards in the deck can be altered or deleted if the caller manipulates the cards returned by Deck.get_suit(). You may want to make a deep copy of the list you return in Deck.get_suit() to prevent this.

If this is to model standard deck based games where a deck always has 1 of each card (and optional jokers), and a card will always be in the deck or not, you may want to re-evaluate your design where cards are deleted from the deck. Will you ever want to support shuffling a card or cards back into the deck once drawn? How would you prevent duplicate cards from ending up in the deck? It could be easier to add a Boolean to each card indicating if its in the deck or not. This way the deck always knows about the cards it was initialized with, and these classes will be easier to debug.

\$\endgroup\$
1
  • \$\begingroup\$ Your filter syntax is not valid Python. It should either be [card for card in self.cards if card.suit == Suits.Spade] or filter(lambda card: card.suit == Suits.Spade, self.cards). \$\endgroup\$
    – Graipher
    Commented Mar 2, 2019 at 10:16

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.