Multiplayer card game "Hearts" with OOP in Python

Question

To practice Object-Oriented Python and learning how to write tests, I found an exercise and solved it as below(all classes are put in one block of code to make the question a little bit more readable):

import sys
import random
from typing import List, Tuple, Dict, Optional
from abc import ABC, abstractmethod

class Card:
    SUITS = "♠ ♡ ♢ ♣".split()
    RANKS = "2 3 4 5 6 7 8 9 10 J Q K A".split()

    def __init__(self, suit: str, rank: str) -> None :
        self.suit = suit
        self.rank = rank

    @property
    def suit(self) -> str:
        return self.__suit

    @property
    def rank(self) -> str:
        return self.__rank

    @suit.setter
    def suit(self, suit):
        if suit not in self.__class__.SUITS:
            raise ValueError("Invalid Card Suit")
        self.__suit = suit

    @rank.setter
    def rank(self, rank):
        if rank not in self.__class__.RANKS:
            raise ValueError("Invalid Card Rank")
        self.__rank = rank

    def __repr__(self) -> str :
        return f"{self.__suit}{self.__rank}"

    def __hash__(self):
        return hash((self.__suit, self.__rank))

    def __eq__(self, second) -> bool:
        return self.__suit == second.suit and self.__rank == second.rank

    def __gt__(self, second) -> bool:
        """
            Specifies whether this card is greater than another card
            NOTICE: if the suits are different, returns False.
        """
        rankNums = {rank: num for (rank, num) in zip(list("23456789")+["10"]+list("JQKA"), range(2,15))} 
        if second.suit == self.__suit:
            if rankNums[self.__rank] > rankNums[second.rank]:
                return True
        return False

class Trick:
        HEARTS_ALLOWED: bool = False

        def __init__(self, cards: Optional[Tuple[Card, ...]]=None):
            self.__cards: Tuple[Card,...] = cards

        @property
        def cards(self) -> Tuple[Card,...]:
            return self.__cards

        def get_points(self):
            points = 0
            for card in self.__cards:
                if card.suit == "♡":
                    points += 1
                elif card.suit == "♠" and card.rank == "Q":
                    points += 13
            return points

        def add_card(self, card: Card):
            if self.cards and len(self.cards) >= 4:
                raise ValueError("More than 4 cards cannot be added to a trick")
            if self.cards and card in self.cards:
                raise ValueError("The same card cannot be added to a trick twice")
            if self.__cards:
                self.__cards = (*self.__cards, card)
            else:
                self.__cards = (card,)

        def get_winCard_idx(self) -> int:
            """ returns the turn number in which the winner card of the trick was played """
            winIdx = 0
            maxCard: Card = self.__cards[0]
            for idx, card in enumerate(self.__cards):
                if card > maxCard:
                    winIdx = idx
                    maxCard = card
            return winIdx

class Deck:
    def __init__(self, **kwargs) -> None :
        """ 
        possible keyword arguments:
            cards: Optional[List[Card]]=None
            shuffle: bool=False
        """
        self.cards_setter(kwargs)

    def cards_getter(self) -> List[Card]:
        return self.__cards

    def cards_setter(self, kwargs):
        cards = kwargs["cards"] if "cards" in kwargs else None
        shuffle = kwargs["shuffle"] if "shuffle" in kwargs else False

        if not cards:
            cards = [Card(s, r) for r in Card.RANKS for s in Card.SUITS]
        if shuffle:
            random.shuffle(cards)
        self.__cards: List[Card] = cards

    cards = property(cards_getter, cards_setter)

    def __iter__(self) -> Card:
        yield from self.cards

    def deal(self) -> Tuple["Deck", "Deck", "Deck", "Deck"] :
        """Deal the cards in the deck into 4 hands"""
        cls = self.__class__
        return tuple(cls(cards=self.__cards[i::4]) for i in range(4))

class Player(ABC):
    def __init__(self, name: str, hand: Deck) -> None:
        self.name: str = name
        self.hand: Deck = hand
        self.tricksPointsSum: int = 0
        self.roundsPointsSum: int = 0

    @property
    def name(self) -> str:
        return self.__name

    @name.setter
    def name(self, name):
        self.__name = name

    @property
    def hand(self) -> Deck:
        return self._hand

    @hand.setter
    def hand(self, cards: Deck):
        self._hand = cards

    @property
    def tricksPointsSum(self) -> int:
        return self.__tricksPointsSum

    @tricksPointsSum.setter
    def tricksPointsSum(self, tricksPointsSum: int):
        self.__tricksPointsSum = tricksPointsSum

    @property
    def roundsPointsSum(self) -> int:
        return self.__roundsPointsSum

    @roundsPointsSum.setter
    def roundsPointsSum(self, roundsPointsSum: int):
        self.__roundsPointsSum = roundsPointsSum

    def play_card(self, trick: Trick) -> Trick:
        if Card("♣","2") in self._hand:
            yield self.__play_this(Card("♣","2"), trick)
        while True:
            playable = self.__get_playable_cards(trick)
            chosen_card = self._prompt_choice(playable)
            yield self.__play_this(chosen_card, trick)

    def __play_this(self, card: Card, trick: Trick) -> Trick:
        trick.add_card(card)
        print(f"{self.__name} -> {card}")
        self._hand.cards.remove(card)
        return trick

    def __get_playable_cards(self, trick: Trick) -> List[Card]:
        if not trick.cards:
            if Trick.HEARTS_ALLOWED:
                return self._hand.cards
            else:
                lst = list(filter(lambda card: card.suit != "♡" , self._hand))
                if lst:
                    return lst
                else:
                    Trick.HEARTS_ALLOWED = True
                    return self.__get_playable_cards(trick)
        else:
            trickSuit = trick.cards[0].suit
            if self.has_card(trickSuit):
                return list(filter(lambda card: card.suit == trickSuit, self._hand))
            else:
                Trick.HEARTS_ALLOWED = True
                return self._hand.cards

    def has_card(self, suit, rank: Optional[str] = None) -> bool:
        if rank:
            if Card(suit, rank) in self._hand:
                return True
        else:
            for card in self._hand:
                if card.suit == suit:
                    return True
        return False

    @abstractmethod
    def _prompt_choice(self, playable: Deck) -> Card:
        pass

class HumanPlayer(Player):
    def _prompt_choice(self, playable: Deck) -> Card:
        rankNums = {rank: num for (rank, num) in zip(list("23456789")+["10"]+list("JQKA"), range(2,15))} 
        sortedPlayable = sorted(playable, key=lambda card: (card.suit, rankNums[card.rank]))

        [print(f"\t{idx}: {card} ", end="") for idx, card in enumerate(sortedPlayable)]
        print("(Rest: ", end="")
        for nonPlayableCard in list(set(self._hand.cards)-set(playable)):
            print(nonPlayableCard, end="")
            print(" ", end="")
        print(")")
        while True:
            print(f"\t{self.name}, choose card: ", end="")
            try:
                choiceCardIdx: int = int(input())
            except ValueError:
                continue
            if choiceCardIdx < len(sortedPlayable):
                break

        return sortedPlayable[choiceCardIdx]

class AutoPlayer(Player):
    def _prompt_choice(self, playable: Deck) -> Card:
        rankNums = {rank: num for (rank, num) in zip(list("23456789")+["10"]+list("JQKA"), range(2,15))} 
        sortedPlayable = sorted(playable, key=lambda card: (card.suit, rankNums[card.rank]))
        return sortedPlayable[0]

class Game:

    def __init__(self, numOfHumans: Optional[int] = 1, *playerNames: Optional[str]) -> None:
        """Set up the deck and create the 4 players"""
        self.__roundNumber: int = 1
        self.__names: List[str] = (list(playerNames) + "P1 P2 P3 P4".split())[:4]
        self.__players: List[Player] = [ HumanPlayer(name, hand) for name, hand in zip(self.__names[:numOfHumans], [None]*numOfHumans)]
        self.__players.extend((AutoPlayer(name, hand) for name, hand in zip(self.__names[numOfHumans:], [None]*(4-numOfHumans)) ))

    def __set_new_round(self):
        deck = Deck(shuffle=True)
        hands = deck.deal()
        for idx, player in enumerate(self.__players):
            player.hand = hands[idx]

    def play(self) -> int:
        """Play the card game"""

        while max(player.roundsPointsSum for player in self.__players) < 100:
            """ while no one has lost the whole game """ 

            self.__set_new_round()

            # continue the round by playing the rest of the tricks
            winnerPlayer: Optional[Player] = None
            while self.__players[0].hand.cards:
                trick = Trick()
                turnOrder = next(self.__player_order(winnerPlayer))
                for player in turnOrder:
                    trick = next(player.play_card(trick))
                winnerIdx = trick.get_winCard_idx()
                winnerPlayer = turnOrder[winnerIdx]
                winnerPlayer.tricksPointsSum += trick.get_points()
                turnOrder = self.__player_order(winnerPlayer)
                print(f"{winnerPlayer.name} wins the trick\n{'*'*17}")
            # end of round

            print(f"{'-'*50}\nEnd Of Round {self.__roundNumber}\n{'-'*50}")

            # save the round scores
            for player in self.__players:
                player.roundsPointsSum += player.tricksPointsSum
                print(f"{player.name}: {player.roundsPointsSum}")
                player.tricksPointsSum = 0

            # finish the round and reset it
            Trick.HEARTS_ALLOWED = False
            self.__roundNumber += 1
            print(f"{'-'*50}\n")

        #end of the whole game
        self.__announce_winner()
        return 0

    def __announce_winner(self) -> None:
        winnerName = min(self.__players, key=lambda player: player.roundsPointsSum).name
        print(f"{'*' * 50}\n{'*'*16} {winnerName} wins the game {'*'*16}\n{'*' * 50}")

    def __player_order(self, startPlayer: Player) -> List[Player]:
        """Rotate player order so that start goes first"""
        if not startPlayer:
            for player in self.__players:
                """ find the player that has the ♣2 card """
                if player.has_card("♣", "2"):
                    start_idx = self.__players.index(player)
                    yield self.__players[start_idx:] + self.__players[:start_idx]
                    break
        while True:
            start_idx = self.__players.index(startPlayer)
            yield self.__players[start_idx:] + self.__players[:start_idx]

if __name__ == "__main__":
    try:
        numOfHumans = int(sys.argv[1])
        if numOfHumans > 4 or numOfHumans < 0:
            raise ValueError("Number of human players cannot be less than 0 or more than 4")
        playerNames = sys.argv[2:2+numOfHumans]
    except (IndexError, ValueError):
        print("Number of human players is automatically set to 1")
        numOfHumans = 1
        playerNames = ()

    try:
        game = Game(numOfHumans, *playerNames)
        game.play()
    except EOFError:
        print("\nGame Aborted")

Example usage(the number of humanPlayers can be from 0 to 4):

python3 hearts.py 0

I know it's so important to learn how to write tests for any software so I chose Pytest as a start and wrote these tests as well for the public methods of the above classes:

import pytest
from french_card_game_oo import Card, HumanPlayer, AutoPlayer, Deck, Trick, Game

SUITS = {suit_title: suit_symbol for (suit_title, suit_symbol) in zip(["spades", "hearts", "diamonds", "clubs"], "♠ ♡ ♢ ♣".split())}

def test_invalid_card_suit():
    with pytest.raises(ValueError):
        Card("*",5)

def test_invalid_card_rank():
    with pytest.raises(ValueError):
        Card(SUITS["diamonds"],13)

def test_card_5_diamonds_eq_card_5_diamonds():
    assert Card(SUITS["diamonds"], "5") == Card(SUITS["diamonds"], "5")

def test_card_A_spades_gt_3_spades():
    assert Card(SUITS["spades"], "A") > Card(SUITS["spades"], "3")

def test_card_A_spades_isnt_gt_10_clubs():
    assert not (Card(SUITS["spades"], "A") > Card(SUITS["clubs"], "10"))

@pytest.fixture
def trick_15_points():
    return Trick(
        (
        Card(SUITS['spades'], "Q"),
        Card(SUITS['hearts'], "2"),
        Card(SUITS['hearts'], "3")
        )
    )

def test_get_points_of_trick_of_15_points(trick_15_points):
    assert trick_15_points.get_points() == 15

def test_add_card_to_trick(trick_15_points):
    trick_15_points.add_card(Card(SUITS['hearts'], "4"))
    assert len(trick_15_points.cards) == 4

def test_cannot_add_5th_card_to_a_trick(trick_15_points: Trick):
    with pytest.raises(ValueError):
        trick_15_points.add_card(Card(SUITS['hearts'], "4"))
        trick_15_points.add_card(Card(SUITS['hearts'], "5"))

def test_cannot_add_repeated_card_to_trick(trick_15_points: Trick):
    with pytest.raises(ValueError):
        trick_15_points.add_card(Card(SUITS['hearts'], "3"))

def test_get_winner_card_idx1(trick_15_points: Trick):
    trick_15_points.add_card(Card(SUITS['spades'], "J"))
    assert trick_15_points.get_winCard_idx() == 0

def test_get_winner_card_idx2(trick_15_points: Trick):
    trick_15_points.add_card(Card(SUITS['spades'], "A"))
    assert trick_15_points.get_winCard_idx() == 3

def test_get_winner_card_idx3(trick_15_points: Trick):
    trick_15_points.add_card(Card(SUITS['clubs'], "A"))
    assert trick_15_points.get_winCard_idx() == 0

def test_deck_creation():
    Deck()
    Deck(shuffle=True)
    Deck(cards=[Card(SUITS['clubs'],"A")])
    Deck(cards=[Card(SUITS['clubs'],"K")],shuffle=True)

@pytest.fixture
def deck() -> Deck:
    return Deck(shuffle=True)

def test_can_iterate_in_deck(deck: Deck):
    for card in deck:
        pass

def test_deal_full_deck(deck: Deck):
    hands = deck.deal()
    assert len(hands) == 4
    assert isinstance(hands[0], Deck)
    assert hands[0].cards

@pytest.fixture
def humanPlayer(deck: Deck):
    return HumanPlayer("Joe", deck)

def test_play_card(humanPlayer: HumanPlayer, monkeypatch):
    trick = Trick()
    assert Card(SUITS["clubs"], "2") in next(humanPlayer.play_card(trick)).cards
    monkeypatch.setattr("builtins.input", lambda: 0)
    len(next(humanPlayer.play_card(trick)).cards) == 2

def test_game_all_auto_player():
    game = Game(0)
    assert game.play() == 0

The exercise is done (at least gives me a minimum satisfaction), but now I'm riddled with more OO questions. Since I'm self-learning, I'll ask them here, but if it is a TLDR for you, just leave a review of my code independently of my questions.

The questions:

• Isn't this bad practice to create classes that are "plural"s of another class, if they have distinct purposes? I'm referring to Deck and Trick that are both kind of Cards. I have reasons for creating the class Trick, there are points in it, it specifies the winner of the Trick, and more importantly, it is needed to hold the state of the game. It also makes the code much more readable(you give a player the trick when they want to play, and you get a trick back as the output when they've finished playing their card). The class Deck is also basically a wrapper of a list of cards as well. (Probably, I could get rid of them both, but I think then I have to use dictionaries which are not IMO as cool as using objects).

• An argument I've seen a lot in object orientation analysis is "How do you know you won't subclass that class one day?", But in real scenario, should we really consider this warning for ALL of the super classes (and start all of them with interfaces/abstract-classes) or just for some of them? I think that sounds reasonable only for some clasess, e.g., Player -> HumanPlayer & AutoPlayer, but in some cases, sounds like an overkill, why would the class "Trick" be abstract ever?

• I'm addicted to type hinting. Is that bad? It just gives me so much mental help when I read the code and also the IDE uses these hints and gives miraculous aid!

• Is the play() method of the Game class long? Maybe still a functional approach only with a facade of object-orientation? If it is, how can I make it shorter? (I've seen people say long methods are signs of bad/wrong OO designs) I had a hard time thinking of any tests for it so I just added a return value of 0 denoting success and checked if the test received that!

• I've defined both "play_card()" and "play_this()", because "play_this()" occured twice in the play_card(). Is this a bad choice to separate it? Because it adds one more layer to the call stack and this is a call that is made quite a lot of times(it doesn't add to the depth of the call stack though).

• Also the method "has_card()" does two things, both checking the existence of a card in one's hand, and checking the existence of a card with a certain suit in one's hand. IMO, It's more D-R-Y to write both of these in one method. But still it's a common advice to write methods that do only one thing. Should I break it into two methods? E.g. has_card and has_card_with_suit ?

• On the paper, sometimes I thought I had two choices of classes for taking a method. For instance, The "__prompt_choice()" method sounds a bit irrelevant to the class "Player" semantically (it seems much more relevant to the class "Game" probably? Or even a "Screen" class?). But still I thought it was best to be put there in the "Player" class because the method "play_card()" is using it and "play_card()" is in the Player class. Also it's not very unnatural if we think about it this way: "the player is pondering about its own choice".

• Sometimes what I did on paper needed modification when I got my hand in code. Now I've seen people explaining TDD saying it's a tests-first approach(I didn't do it, I wrote the tests "after" the code). So what if one writes the tests and then things turn out different from what they initially thought? E.g. you realize you need another public method, or maybe you realize you need a whole new class as well.

• I've used class variables like "HEARTS_ALLOWED", but I think somehow they are making a global state in the program... aren't they globalish?

Answer 1

I think I addressed most of your questions inline with this code review, but let me know if anything is unclear.

Suit and Rank should be Enums

Much of the code can be simplified or removed if you define Suit and Rank as enumerations. Here's an example implementation:

from enum import Enum


class Suit(Enum):
    SPADES = "♠"
    HEARTS = "♡"
    DIAMONDS = "♢"
    CLUBS = "♣"

    def __str__(self) -> str:
        return self.value


class Rank(Enum):
    TWO = 2
    THREE = 3
    FOUR = 4
    FIVE = 5
    SIX = 6
    SEVEN = 7
    EIGHT = 8
    NINE = 9
    TEN = 10
    JACK = 11
    QUEEN = 12
    KING = 13
    ACE = 14

    def __str__(self) -> str:
        if self is Rank.JACK:
            return "J"
        elif self is Rank.QUEEN:
            return "Q"
        elif self is Rank.KING:
            return "K"
        elif self is Rank.ACE:
            return "A"
        else:
            return str(self.value)

    def __gt__(self, other: "Rank") -> bool:
        return self.value > other.value

    def __lt__(self, other: "Rank") -> bool:
        return self.value < other.value

How does this help?

• Code that checks if a string is one of the suit strings ♠ ♡ ♢ ♣ or one of the rank strings 2 3 4 5 6 7 8 9 10 J Q K A can be removed. For example, instead of passing in a suit: str and performing validations on it, just pass in a suit: Suit. No validations necessary.
• Enumerations are Python classes, which means we can define canonical string representations with our own custom __str__ methods.
• We can also define comparison methods like __gt__ and __lt__ which is useful for Rank. This means we no longer need to create ad-hoc mappings from rank strings to their corresponding integer values, e.g.

  {rank: num for (rank, num) in zip(list("23456789")+["10"]+list("JQKA"), range(2,15))} 
  

  in order to compare or sort by rank.

Card

Card can be simplified a lot if we make it a NamedTuple. NamedTuples, like tuples, are immutable. This is appropriate for modeling Card because we don't need to mutate a card's suit or rank after instantiation.

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

    def __str__(self) -> str:
        return f"{self.suit}{self.rank}"

    def __gt__(self, other: "Card") -> bool:
        return self.suit == other.suit and self.rank > other.rank

Deck

I don't think this is needed. It's basically a very specialized List[Card] that only gets used in __set_new_round. Also, using it as a type in contexts when it actually refers to the player's hand (or a subset of the player's hand that is playable) is confusing.

I would consider removing this class. The logic of instantiating the deck as a list of cards, shuffling it, and dealing out the cards to players can be moved to __set_new_round. In places where Deck is currently expected as a parameter or return type, we can safely replace these with List[Card].

Trick

Unlike Deck, I think Trick is a good abstraction and deserves its own type, even if they both function as "containers" of Cards. A few notes:

• HEARTS_ALLOWED doesn't belong here. It makes more sense as an instance variable of Game.
• self.__cards makes more sense as a List[Card] since a trick is "empty" by default and we can add cards to it.
• It's a matter of preference, but I think adding the @property decorator to get_points and renaming it to something more appropriate like points would be a nicer interface.
• Your size validation of len(self.cards) <= 4 is not applied to the instantiation flow in __init__.

Player

• To answer your question about has_card, I am in favor of splitting it up into two methods: has_card(self, suit: Suit, rank: Rank) and has_card_with_suit(self, suit: Suit). I think having it as two separate methods handling two distinct types of queries makes it much easier to read.

Type hints

I also love type hints, and find that they improve code readability. To answer your question, I don't think you need to worry about being addicted to type hinting.

That said, there are issues with many of the type hints in your program. I ran mypy on your code and it found 40+ errors. I suspect that your IDE isn't running mypy on your code, otherwise it would have flagged these.

One example is the constructor of Trick, where cards is an Optional[Tuple[Card, ...]], but then you directly assign it to self.__cards and assert that it is now a Tuple[Card, ...].

Another is in play_card, where the return type should be Iterator[Trick] but it is just Trick.

What you can do is either set up your IDE with mypy integration (usually by installing a plugin) or periodically run mypy via the command line on your code to catch these errors.

Miscellaneous

• In HumanPlayer's _prompt_choice, if choiceCardIdx < len(sortedPlayable) should be if 0 <= choiceCardIdx < len(sortedPlayable)
• Also in HumanPlayer's _prompt_choice, there is a list comprehension that is created and thrown away in order to print out the playable cards in hand. Instead, I would generally suggest using a for loop here.
• I'm contradicting myself in the above bullet point because I don't think printing in a loop is the most readable approach here. I see a lot of places where print with end="" is used when it is probably a lot easier to construct intermediate strings first with str.join. For example, something like

  [print(f"\t{idx}: {card} ", end="") for idx, card in enumerate(sortedPlayable)]
  print("(Rest: ", end="")
  for nonPlayableCard in list(set(self._hand.cards)-set(playable)):
      print(nonPlayableCard, end="")
      print(" ", end="")
  print(")")
  

  can be replaced with

  playable_cards = "\t".join(
      f"{idx}: {card}" for idx, card in enumerate(sortedPlayable)
  )
  non_playable_cards = " ".join(
      str(card) for card in set(self._hand.cards) - set(playable)
  )
  print(f"{playable_cards} (Rest: {non_playable_cards})")
  

Case consistency

There is some inconsistency in the case used for your method, function, and variable names. Some names are in snake case (the recommended choice), but I also saw camel case and some combination of camel case and snake case.

Examples with suggestions on how to rename:

• get_winCard_idx -> get_win_card_idx
• choiceCardIdx -> choice_card_idx
• tricksPointsSum -> trick_points_sum
• nonPlayableCard -> non_playable_card
• numOfHumans -> num_humans or number_of_humans

Test-driven development (TDD)

Writing code via TDD isn't everyone's cup of tea, which I feel is fine because generally people approach problem-solving with many different strategies.

TDD gets you thinking about all the requirements first, and how you would validate those requirements through tests. And while doing that you are also forced to think about the shape of your data, the functions you'll need, the interfaces exposed by your classes, etc.

For example, consider the feature of figuring out which cards in a player's hand are playable. What would a test for this feature look like? To start, we would probably need the following:

• a trick with 0-3 cards
• knowledge of whether hearts has been broken yet
• a list of cards (the player's hand)

What do we want as output? Maybe the list of playable cards (List[Card]), or maybe we want both lists, playable and non-playable (Tuple[List[Card], List[Card]]). It depends, but we at least have a start here.

So now we have some idea that we want a method that takes in some parameters as described above, and returns the list of playable cards in some format. Maybe it could look like this:

def get_playable_cards(
    trick: Trick, hand: List[Card], hearts_is_broken: bool
) -> List[Card]:
    pass

We don't really care about how get_playable_cards will be implemented, because now we have all the information we need to start sketching out some tests.

One other question is, who has access to all of this information, i.e. who has access to the current trick in play, the current player's hand, and the answer to whether hearts has been broken yet? If I had to guess I'd say Game, but maybe there is a better answer.

The takeaway here is that TDD gets you asking yourself these types of questions which can be very illuminating and helpful before diving into the actual implementation. Yes, there are cases where you write some tests, and then figure out later on that your data model was slightly off, or that you could improve readability of both the code and its tests if you refactored things in a different way. It happens, and in that case you would need to go back and change both the code and the tests. But that's a relatively small price to pay, I think, because what you're getting in return is a maintained suite of tests you can run very quickly against your code at any time while developing it.

Like I said earlier, it's not everyone's preferred style, but you might find it helpful to try it out as an exercise for future coding problems, and then follow up afterwards to see how following TDD influenced your design decisions.