Abstract card game code in Python3

Question

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

• SylvainD: This looks interesting. Any simple example you could implement with this to show how the different pieces fit together?
• mleyfman: 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

Answer 1

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

• mleyfman: 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.
• Emanuele Paolini: I strongly agree with jonrsharpe. The best representation of an AbstractCard is Object, AbstractCardFactory should be removed. Deck is best represented by a list. A Hand is a set. I would suggest you to start with a real implementation of a simple game and compare how simple is to use basic python objects with respect to extend your given classes.

Answer 2

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