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

How object oriented is my Crazy Eights game?

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

Problem

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: ")

Solution

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.

Context

StackExchange Code Review Q#32537, answer score: 8

Revisions (0)

No revisions yet.