HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Abstract card game code in Python3

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
abstractcardgamepython3code

Problem

I've written up some generic code for a card-game and would like to hear any and all suggestions for how to improve this code further, in any way shape or form.

The code is on github or pasted directly below:

Card class:

class AbstractCard:
    """Abstract class for handling comparisons between cards based on rank"""

    def __init__(self, value=None, suit=None, rank=None):
        self.value = value
        self.suit = suit
        self.rank = rank

    def __lt__(self, other):
        if isinstance(other, AbstractCard):
            return self.rank  other.rank
        raise TypeError("Cannot compare to non-card types")

    def __ge__(self, other):
        if isinstance(other, AbstractCard):
            return self.rank >= other.rank
        raise TypeError("Cannot compare to non-card types")

    def __repr__(self):
        data = (self.value, self.suit, self.rank)
        return "(value:{}, suit:{}, rank:{})".format(*data)

    def __str__(self):
        data = (self.value, self.suit)
        return "{} of {}".format(*data)


CardFactory class

from AbstractCard import AbstractCard

class AbstractCardFactory:
    """Factory for making AbstractCards"""

    def __init__(self, values=None, suits=None, rank_function=None):
        self.values = values
        self.suits = suits
        self.rank_function = rank_function

    def __repr__(self):
        return "Values: {}\nSuits: {}".format(self.values, self.suits)

    def __str__(self):
        return self.__repr__()

    def generate(self):
        if self.rank_function:
            for suit in self.suits:
                for value in self.values:
                    card = AbstractCard(value, suit)
                    card.rank = rank_function(card)
                    yield card
        else:
            for suit in self.suits:
                for value in self.values:
                    yield AbstractCard(value, suit)


Deck class

```
from random import *

class AbstractDeck:
"""Abstrac

Solution

In your AbstractDeck class, you:

from random import *


This is unwise - it pollutes the namespace with many names you aren't using, and makes it harder to tell where the one you are using came from. In this case, you only want one name from random, so

from random import randint


is an improvement. Better still, you can factor out the awkward subtraction:

j = randint(i, num_cards - 1)


using randrange (which is half-open, like range) instead:

j = randrange(i, num_cards)


In case you weren't aware of it, there is also a random.shuffle, which apparently uses Fisher-Yates and saves you from implementing it yourself.

You have a potential error in AbstractPlayer:

def hand(self):
    """Returns a copy of the player's hand"""
    return self.cards


This doesn't return a copy; if the calling function mutates the hand, it will affect the player too. To return a copy, do e.g. return self.cards[:]. Alternatively, alter the docstring.

In general (and I apologise if this sounds harsh), much of your code seems either so abstract as to be pointless (AbstractGame) or insufficiently abstract as to be useful (AbstractCard - what about games where suits are important, or games like Uno where some cards are unordered? There are more exceptions than rules here!) I think the card factory is unnecessary - does this logic not belong in a Deck?

I have suggested a class structure for Blackjack elsewhere; you could adapt this to your needs. Note e.g. the separation of a Player (where the I/O and/or playing logic sit, along with e.g. the name and current balance for betting games), and a Hand (where the player's cards sit).

This is by no means a perfect structure - looking at it now, for instance, perhaps Deck and Hand could have some common abstract ancestor CardCollection. Also, you will find that different games have e.g. PokerHand subclasses for whatever specific needs you have - the value of a specific set of cards will depend on the game being played.

If you do keep the card factory, note you can simplify:

from itertools import product

...    

    def generate(self):
        for suit, value in product(self.suits, self.values):
            card = AbstractCard(value, suit)
            if self.rank_function is not None:
                card.rank = rank_function(card)
            yield card

Code Snippets

from random import *
from random import randint
j = randint(i, num_cards - 1)
j = randrange(i, num_cards)
def hand(self):
    """Returns a copy of the player's hand"""
    return self.cards

Context

StackExchange Code Review Q#58925, answer score: 6

Revisions (0)

No revisions yet.