How object oriented is my Crazy Eights game?

Question

I have this Crazy Eights game (which at the moment does not include crazy cards):

class Card(object):
 RANKS = ("2", "3", "4", "5", "6", "7", "8", "9", "T", "J", "Q", "K", "A")
 SUITS = ("c", "d", "h", "s")
 def __init__(self, rank, suit):
 self.rank = rank
 self.suit = suit
 def __str__(self):
 return self.rank + self.suit
 def __eq__(self, other):
 return self.rank == other.rank and self.suit == other.suit
 def canPlayOn(self, other):
 return self.rank == other.rank or self.suit == other.suit
class Hand(object):
 def __init__(self):
 self.cards = []
 def __str__(self):
 return " ".join([str(card) for card in self.cards])
 def __bool__(self):
 return bool(self.cards)
 def give(self, other, card):
 self.remove(card)
 other.add(card)
 def add(self, card):
 self.cards.append(card)
 def remove(self, card):
 self.cards.remove(card)
class Pile(Hand):
 @property
 def topCard(self):
 return self.cards[-1]
class DrawPile(Pile):
 def populate(self):
 for rank in Card.RANKS:
 for suit in Card.SUITS:
 self.add(Card(rank, suit))
 def deal(self, others, cards = 7):
 import random
 random.shuffle(self.cards)
 for _ in range(cards):
 for hand in others:
 self.give(hand, self.topCard)
class Player(Hand):
 def playMove(self, discardPile, drawPile):
 card = self.getCard(discardPile.topCard)
 self.give(discardPile, card) if card else \
 drawPile.give(self, drawPile.topCard)
 def getCard(self, topCard):
 card = input("This is your hand: " + str(self) +
 ". The is the top card: " + str(topCard) +
 ". Enter your card (enter PU to pick up a card): ")
 while self.isInvalid(card, topCard):
 card = input("Invalid input. Enter your card: ")
 return Card(card[0], card[1]) if card != "PU" else None
 def isInvalid(self, card, topCard):
 if card == "PU":
 return False
 elif len(card) != 2 or card[0] not in Card.RANKS or \
 card[1] not in Card.SUITS or \
 Card(card[0], card[1]) not in self.cards or \
 not Card(card[0], card[1]).canPlayOn(topCard):
 return True
 return False
class Computer(Hand):
 def playMove(self, discardPile, drawPile):
 card = self.getCard(discardPile.topCard)
 if card:
 self.give(discardPile, card)
 print("Computer plays " + str(card), end = ". ")
 else:
 drawPile.give(self, drawPile.topCard)
 print("Computer picks up.", end = " ")
 print("Computer has " + str(len(self.cards)) + " cards remaining.")
 def getCard(self, topCard):
 for card in self.cards:
 if card.canPlayOn(topCard):
 return card
 return None
def main():
 drawPile = DrawPile()
 drawPile.populate()
 player = Player()
 computer = Computer()
 playerList = [player, computer]
 drawPile.deal(playerList)
 discardPile = Pile()
 drawPile.give(discardPile, drawPile.topCard)
 playerIndex = 0
 while isStillPlaying(playerList):
 playerList[playerIndex].playMove(discardPile, drawPile)
 if playerIndex < len(playerList) - 1:
 playerIndex += 1
 else:
 playerIndex = 0
 print("Computer wins!" if player else "You win!")
def isStillPlaying(playerList):
 for player in playerList:
 if not player:
 return False
 return True
if __name__ == "__main__":
 main()

Can someone provide a code review (particularity on my OOP skills)? Also, what would be the most object oriented way to implement "crazy cards"?

Answer 1

I'm just going to look at your OOP design for this review, as you requested.

Your canPlayOn() method does not belong in the Card class. The purpose of your Card class should be to represent a card, and nothing more. You should be able to use the same Card class for Blackjack, Bridge, or any other card game. Logic specific to Crazy Eights does not belong there, and should probably be moved into the Player class.

I was initially thinking that your Hand should consist of a set of cards rather than an array, since the items are distinct and order doesn't matter. Then I saw that your Pile derives from Hand, and in Pile, order does matter. Maybe using an array is OK after all, if you accept that a Pile is a subclass of Hand. I think that such an inheritance relationship is more of a hack than an accurate model, but I can let it slide because it seems to work in this case.

In DrawPile.deal(), I would rename the others parameter to recipient_hands. Also imports should go at the beginning of the file.

I take issue with Player(Hand) and Computer(Hand). First of all, I would say that a player has a hand rather than a player is a hand. That suggest that composition should be favoured over inheritance. Also, the computer is a kind of player, so there should be an inheritance relationship there. Furthermore, there is a lot of redundant code between Player and Computer, differing mainly in what the output looks like. Perhaps your Player and Computer classes could be merged into a Crazy8Player class, with the constructor taking name, narrator, and strategy arguments.

Answer 2

1. What happens if the drawPile runs out of cards? It looks to me as though eventually you'll get

   IndexError: list index out of range
   

   when trying to return the topCard of an empty Pile. You need to check at the end of the main loop to see if the draw pile is empty, and take some action accordingly. For example, you could shuffle the discard pile into the draw pile. (But this might still leave the draw pile empty, so you need a fallback for that case, unlikely as it might be.)

2. You ask, "what would be the most object oriented way to implement this?" but is that really the right question to ask? Object-orientation is a technique, not a goal in itself.

3. I'm not convinced that you gain anything by having a Card class. What would you lose if you represented a card as a two-character string like "8h"?

4. Since all the ranks are one letter long, instead of:

   RANKS = ("2", "3", "4", "5", "6", "7", "8", "9", "T", "J", "Q", "K", "A")
   

   you can write:

   RANKS = '23456789TJQKA'
   

   (and similarly for the suits).

5. Transferring the top card from one pile to another is such a common operation that it would simplify your code if you special-cased it in the give method.

6. I'm not convinced that you gain anything by having separate Hand and Pile classes. Logically speaking you're right that a pile differs from a hand in that it has a top card, but this has no practical effect (you never call the topCard method on a Hand object and nothing would go wrong if you did). So it's just added complexity for no benefit.

   (If you really wanted to apply strict object-oriented logic, you wouldn't subclass Player from Hand since a player has a hand of cards, not "is a hand of cards".)

7. I'm not convinced that you gain anything by having a DrawPile class. There is only ever one object of this class, and moreover its populate and deal methods are only called from one place.

8. I concur with Morwenn that the code would be much simplified by inheriting Hand from list.

9. Whenever you have two nested loops, as in your deal method, consider using itertools.product to simplify them to a single loop.

10. When you iterate over a sequence repeatedly (as in your main loop where you are iterating over the players), consider using itertools.cycle.

Here's a revised version of the code that implements the suggestions above, and contains a few other changes too.

from abc import abstractmethod, ABCMeta
from itertools import cycle, product
from random import shuffle
RANKS = '23456789TJQKA'
SUITS = 'CDHS'
INITIAL_CARDS = 7
class Pile(list):
 """An ordered sequence of cards."""
 def __str__(self):
 return ' '.join(self)
 @property
 def top(self):
 """The top card of the pile."""
 return self[-1]
 def transfer(self, dest, card=None):
 """Transfer card (or top card if omitted) to dest and return it."""
 if card is None:
 card = self.top
 self.remove(card)
 dest.append(card)
 return card
class Player(object, metaclass=ABCMeta):
 def __init__(self, game):
 self.game = game
 self.hand = Pile()
 def can_play_on(self, card, other):
 """Return True iff card can legally be played on other."""
 return any(c == o for c, o in zip(card, other))
 def draw(self):
 """Draw a card and return it."""
 return self.game.draw.transfer(self.hand)
 def discard(self, card=None):
 """Discard a card and return it."""
 return self.hand.transfer(self.game.discard, card)
 @abstractmethod
 def play(self):
 """Play one turn."""
 pass
class Human(Player):
 win_message = "You win!"
 def play(self):
 print("This is your hand: {}.\n"
 "This is the top card: {}."
 .format(self.hand, self.game.discard.top))
 cmd = input("Enter your card (or P to pick up a card): ")
 while True:
 cmd = cmd.upper()
 if cmd.startswith('P'):
 print("You pick up {}.".format(self.draw()))
 return
 elif cmd not in self.hand:
 cmd = input("You don't have that card. Try again: ")
 elif not self.can_play_on(cmd, self.game.discard.top):
 cmd = input("You can't play that card. Try again: ")
 else:
 self.discard(cmd)
 return
class Computer(Player):
 win_message = "Computer wins!"
 def play(self):
 for card in self.hand:
 if self.can_play_on(card, self.game.discard.top):
 self.discard(card)
 print("Computer plays {}.".format(card))
 break
 else:
 self.draw()
 print("Computer picks up.")
 cards = len(self.hand)
 print("Computer has {} card{} remaining."
 .format(cards, '' if cards == 1 else 's'))
class Game(object):
 def __init__(self):
 self.draw = Pile(''.join(card) for card in product(RANKS, SUITS))
 self.discard = Pile()
 self.players = [Human(self), Computer(self)]
 shuffle(self.draw)
 for _, player in product(range(INITIAL_CARDS), self.players):
 player.draw()
 self.draw.transfer(self.discard)
 def play(self):
 for player in cycle(self.players):
 player.play()
 if not player.hand:
 print(player.win_message)
 break
 if not self.draw:
 print("Draw pile is empty!\n"
 "Shuffling discard pile into draw pile.")
 self.draw, self.discard = self.discard, self.draw
 shuffle(self.draw)
 self.draw.transfer(self.discard)
 if not self.draw:
 print("Out of cards! Let's call it a tie.\n")
 break
if __name__ == "__main__":
 Game().play()

Answer 3

Generally speaking, the code seems quite good. However, there are some parts that could be a little bit enhanced. I did not try to understand the whole thing, but I can give you some pointers.

for _ in range(cards):

The name name _ is often imported is the built-in functions as an alias for the gettext function. Using it to discard unwanted variables could result in surprising errors.

self.give(discardPile, card) if card else \
 drawPile.give(self, drawPile.topCard)

This line is so long that you had to split it. The inline if ... else does not add anything to the code or the readability here. Just go back to a regular if ... else:

if card:
 self.give(discardPile, card)
else:
 drawPile.give(self, drawPile.topCard)

Also, your class Hand is nothing but a list of cards. Instead of having to always index self.cards, you could probably have Hand inherit from list. Not sure whether you would want to do this or not, but it could be pretty handful to avoid having one more layer of indirection every time you want to access your list of cards. On the other hand, all the classes derived from Hand would be unable to derive from a second built-in type. I'm not saying it is a good idea, but it may be worth trying. Also, if you do this, you can get rid of Hand.remove and Hand.add since the methods remove and append would be directly inherited from list.