current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

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 __le__(self, other):
        if isinstance(other, AbstractCard):
            return self.rank <= other.rank
        raise TypeError("Cannot compare to non-card types")

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

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

    def __gt__(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:
"""Abstract class for shuffling and iterating over a collection of cards"""

    def __init__(self, cards=None):
        self.cards = cards

    def __len__(self):
        return len(cards)

    def __iter__(self):
        return iter(self.cards)

    def shuffle(self):
        """Shuffles the deck with the Fisher-Yates shuffle"""
        num_cards = len(self.cards)
        for i in range(0, num_cards):
            j = randint(i, num_cards - 1)
            self.cards[i], self.cards[j] = self.cards[j], self.cards[i]

Player class

class AbstractPlayer:
"""Abstract class for containing basic player information"""

    def __init__(self, name=None, cards=[]):
        self.name = name
        self.cards = list(cards)

    def name(self):
        return self.name

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

    def add_card(self, card):
        """Adds the given card to the existing cards"""
        self.cards.append(card)

    def remove_card(self, card):
        """Removes the given card from the player's hand"""
        self.cards.remove(card)

    def __repr__(self):
        return self.name

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

Game class

class AbstractGame:
"""Abstract class for running a card game"""

    def __init__(self, players=[], deck=None):
        self.players = list(players)
        self.deck = deck

    def play_game(self):
        """Override this function for custom game logic"""
        pass

How to use

  1. Construct a deck using a AbstractCardFactory
  2. Extend the AbstractGame class with custom game logic in the play_game() method
  3. Add a if __name__ == "__main__": block with player initialization logic
share|improve this question
    
This looks interesting. Any simple example you could implement with this to show how the different pieces fit together? –  Josay yesterday
    
I'm planning to use this in a few projects later, but I don't have anything quite like that now, but I'll add a comment as to how it all works –  mleyfman yesterday

2 Answers 2

up vote 4 down vote accepted

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
share|improve this answer
    
Thanks for the input! My implementation of AbstractCard does allow for ranking by suits, as it has a 3rd parameter called rank which can be manually altered or created with a rank_function that takes a card and is a parameter to the AbstractCardFactory. Not all card games have hands (solitaire for instance) so I did not include it for that reason. Also, the separation of Deck and the Factory is so that it is more intuitive, as a deck is basically a iterator/shuffler and does not need to know about suits/values and etc. Finally, AbstractGame is meant to be an easy extension point. –  mleyfman yesterday
up vote 1 down vote

Additional simple comments :

for i in range(0, num_cards): can be written for i in range(num_cards):

You can remove code duplication in 

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)

by writting (non-tested code) :

def generate(self):
    for suit in self.suits:
        for value in self.values:
            card = AbstractCard(value, suit)
            card.rank = rank_function(card) if self.rank_function else None
            yield card
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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