# Python implementation of snap

I did an assessment task where I had to implement the game snap. The description:

Simulate a simplified game of snap between two computer players using N packs of cards (standard 52 card, 4 suit packs). A game of snap proceeds by shuffling N packs of cards and then drawing cards from the top of the shuffled pile. Each card drawn is compared to the previous card drawn and, if they match, all the cards drawn since the last match are randomly allocated to one of the players. The game continues until all the cards have been drawn from the shuffled pile. Any cards played without ending in a match once all the cards have been drawn are ignored. The winner is the player who has accumulated the most cards at the end of the game.

There are three possible matching conditions:

1. The cards have the same suit.
2. The cards have the same value.
3. The cards have the same suit or the same value.

• How many (N) packs of cards to use.
• Which of the three matching conditions to use.

Then simulate a game being played and display the winner.

My solution:

from utils import read_options_from_stdin

from snap.game import Game

if __name__ == '__main__':
game = Game.init_game(number_of_packs, matching_condition)

while not game.finished:
game.turn()

print game.winner


from snap.consts import MATCHING_CONDITIONS

number_of_packs = None
while type(number_of_packs) != int:
try:
number_of_packs = int(raw_input('Please tell me how many packs to use: ').strip())
except ValueError:

matching_condition = None
while matching_condition not in MATCHING_CONDITIONS:
if matching_condition is not None:
print 'wrong matching condition.'

matching_condition = raw_input('''
Options are: {mathcing_consitions}
'''.format(mathcing_consitions=MATCHING_CONDITIONS.keys()))

return number_of_packs, matching_condition


class Card(object):
def __init__(self, value, suit):
self.value = value
self.suit = suit

def __str__(self):
return 'Card {suit} {value}'.format(suit=self.suit, value=self.value)

class CardSameValue(Card):
def __init__(self, *args, **kwargs):
super(CardSameValue, self).__init__(*args, **kwargs)

def __eq__(self, other):
return self.value == other.value

class CardSameSuit(Card):
def __init__(self, *args, **kwargs):
super(CardSameSuit, self).__init__(*args, **kwargs)

def __eq__(self, other):
return self.suit == other.suit

class CardSameValueOrSameSuit(Card):
def __init__(self, *args, **kwargs):
super(CardSameValueOrSameSuit, self).__init__(*args, **kwargs)

def __eq__(self, other):
return self.value == other.value or self.suit == other.suit


import card

MATCHING_CONDITION_SAME_SUIT = 'same_suit'
MATCHING_CONDITION_SAME_VALUE = 'same_value'
MATCHING_CONDITION_SAME_SUIT_OR_VALUE = 'same_suit_or_value'

MATCHING_CONDITIONS = {
MATCHING_CONDITION_SAME_SUIT: card.CardSameSuit,
MATCHING_CONDITION_SAME_VALUE: card.CardSameValue,
MATCHING_CONDITION_SAME_SUIT_OR_VALUE: card.CardSameValueOrSameSuit,
}

SUIT_HEARTS = 'hearts'
SUIT_DIAMONDS = 'diamonds'
SUIT_CLUBS = 'clubs'

SUITS = [
SUIT_HEARTS,
SUIT_DIAMONDS,
SUIT_CLUBS,
]

VALUE_ACE = 'ACE'
VALUE_1 = '1'
VALUE_2 = '2'
VALUE_3 = '3'
VALUE_4 = '4'
VALUE_5 = '5'
VALUE_6 = '6'
VALUE_7 = '7'
VALUE_8 = '8'
VALUE_9 = '9'
VALUE_10 = '10'
VALUE_J = 'J'
VALUE_Q = 'Q'
VALUE_K = 'K'

VALUES = [
VALUE_ACE,
VALUE_1,
VALUE_2,
VALUE_3,
VALUE_4,
VALUE_5,
VALUE_6,
VALUE_7,
VALUE_8,
VALUE_9,
VALUE_10,
VALUE_J,
VALUE_Q,
VALUE_K,
]


import random

from .consts import SUITS, VALUES, MATCHING_CONDITIONS

class Deck(object):

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

def shuffle(self):
random.shuffle(self.cards)

def draw(self):
return self.cards.pop(0)

def is_empty(self):
return len(self.cards) == 0

def size(self):
return len(self.cards)

self.cards += other.cards
return self

@classmethod
def get_empty_deck(cls):
return cls([])

@classmethod
def get_pack(cls, matching_condition):
card_class = MATCHING_CONDITIONS[matching_condition]

cards = []
for value in VALUES:
for suit in SUITS:
cards.append(card_class(value, suit))

return cls(cards)

def __str__(self):
return str([str(card) for card in self.cards])


import random

from .deck import Deck
from .player import Player

class Game(object):

def __init__(self, deck, players):
self.deck = deck
self.players = players
self.previously_drawn = None
self.pile_size = 0

@property
def finished(self):
return self.deck.is_empty()

@property
def winner(self):
print 'cards left ', self.pile_size
return max(self.players, key=lambda player:player.number_of_cards).player_name

@classmethod
def init_game(cls, number_of_packs, matching_condition):
players = [
Player.player_factory('Player 1'),
Player.player_factory('Player 2'),
]

deck = Deck.get_empty_deck()
for _ in xrange(number_of_packs):
deck += Deck.get_pack(matching_condition)

deck.shuffle()

return cls(deck, players)

def print_state(self, drawn_card, match):
print 'pile_size:{pile_size} cards_left: {cards_left}\tdrawn: {drawn_card},\tPrev. drawn: {previously_drawn_card}\tMatch: {match}\tplayers {players}'.format(
pile_size=self.pile_size,
cards_left=self.deck.size(),
drawn_card=drawn_card,
previously_drawn_card=self.previously_drawn,
match=match,
players=[str(player) for player in self.players]
)

def turn(self):
drawn_card = self.deck.draw()
self.pile_size += 1

match = False

if self.previously_drawn is not None:
match = drawn_card == self.previously_drawn

if match:
self.pile_size = 0

self.print_state(drawn_card, match)

self.previously_drawn = drawn_card


class Player(object):
def __init__(self, number_of_cards, player_name):
self.number_of_cards = number_of_cards
self.player_name = player_name

@classmethod
def player_factory(cls, player_name):
return cls(0, player_name)

self.number_of_cards += number_of_cards

def __str__(self):
return '{name} {pile}'.format(name=self.player_name, pile=self.number_of_cards)


I got a feedback that the quality of this code is not high enough. I'm wondering what should have been done differently?

• Welcome to Code Review, nice first question! I hope you get some good reviews! – Peilonrayz Jul 14 '16 at 11:29
• You probably shouldn't have both an 'Ace' and a '1' card... – Dan Oberlam Jul 14 '16 at 18:55

### "Constants"

Excessive use of "constants" like MATCHING_CONDITION_SAME_SUIT and SUIT_DIAMONDS. If you like that approach, you can simply go with SUITS, VALUES and MATCHING_CONDITIONS and you won't lose any functionality, since all suits, values and matching conditions are treated equally - you never use MATCHING_CONDITION_SAME_SUIT directly.

Also, why caps?

### Initing deck with cards

Deck's __init__ accepts cards. I get that you wanted the deck to be able to handle all 3 types of cards but you could simply inject class of card being used and make deck perfectly capable of initing itself beyond that point (if you really want to have 3 different classes for cards).

I see that you implemented deck summation. If you made things a bit differently, you could simply write deck = Pack52(SameValueCard) * number_of_packs with Pack52 - subclass of Deck - initing itself in an obvious way.

### New class just for 1 new method

You use different classes for different methods of comparison. This is a straigtforward solution: 3 methods - 3 __eq__'s - 3 classes. However, that looks like abit of a waste - we know that these 'cards' would not have differences besides their __eq__'s. If we haste to split into several classes here, we may run into diamond problem somewhere down the road. Instead, you could supply a function that compares 2 cards.

Sure, using = operator is nice. Well, Card could have an .adopt() method that accepts comparison function and uses it in __eq__.

Something like this:

matching_conditions = {
'same_suit' : lambda a,b : a.suit == b.suit
}
...
from contextlib import contextmanager
@contextmanager
def matching_condition(condition):
yield matching_conditions[condition]

with matching_condition('same_suit') as comparison:
for card in deck.cards:


Supplying comparison function to the __init__ will do the job too, albeit it limits us a bit.

### Needless coupling in read_options_from_stdin

It's not reusable. To pick matching condition, make a function that prompts user with a list of options (and maybe some prompt). Same thing with number of packs.

I got a feedback that the quality of this code is not high enough. I'm wondering what should have been done differently?

Were there more context? I've listed some things that could be done differently but there may be some tradeoffs I'm not aware of.

• Caps: afaik there are no real constants in Python so caps indicate that those values are not meant to be changed. Just like the underscore for private functions. As for the rest of your answer, thank you, this is the answer I was looking for. – fodma1 Jul 14 '16 at 14:41
• You may want to read PEP8's naming conventions but the list is roughly: normal_variables, Class, CONSTANT, _weak_internal_use, avoid_conflict_, __name_mangle, __magic_method__. – Peilonrayz Jul 15 '16 at 9:02

# Fragmentation

Each file is rather short. I assume that you probably wanted to have a very dedicated scope for each file, but splitting the logic that much makes it a bit harder to follow through, especially on small projects like that. I would have used 2 files, one dedicated to decks and cards, and the other one for the game itself, with player and input handling.

# OOP

You define __init__ methods on subclasses of cards that just use super on generic arguments. There is absolutely no need to that as it is the default behaviour in Python. What is left is just 3 classes with the __eq__ method which thus would be better of as functions:

def compare_cards_by_value(card1, card2):
return card1.value == card2.value

def comapare_cards_by_suit(card1, card2):
return card1.suit == card2.suit

def compare_cards_by_suit_or_value(card1, card2):
return compare_cards_by_suit(card1, card2) or compare_cards_by_value(card1, card2)


Which means Cards would only be a class to store attributes, but a namedtuple would fit better there, as it would make a Card immutable:

Card = namedtuple('Card', 'value suit')


Your use of classmethod is also unconventional. Especially in the Player class. If you want to be able to build a Player without specifying the amount of cards, it would be better to give this parameter a default value in the constructor:

class Player(object):
def __init__(self, name, number_of_cards=0):
self.cards = number_of_cards
self.name = name

self.cards += number_of_cards

def __str__(self):
return '{p.name} {p.cards}'.format(p=self)


Pretty much the same argument for Deck:

class Deck:
def __init__(self, cards=None):
if cards is None:
cards = [Card(value, suit) for value in VALUES for suit in SUITS]
self.cards = cards


get_empty_deck being only Deck([]) doesn't add much value here. What could have been a better classmethod, however, is a merge method:

    @classmethod
def merge(cls, number_of_decks):
return sum((cls() for _ in range(number_of_decks)), cls([]))


but this writing requires you to write an __add__ method instead of the __iadd__ one:

    def __add__(self, other):
return self.__class__(self.cards + other.cards)


Moreover some methods of this class would be better of as special ("magic") methods:

    def __len__(self):
return len(self.cards)

def __nonzero__(self):
return bool(self.cards)


Which will let you use more pythonic construct such as while my_deck

I also feel that there is not much added value to write a Game class and a function or two would suffice, example code latter.

# Handling user input

I don't know if I misunderstood slightly the requirements, but I thought user input should be handled from the command line. Which is ofter neater as you can use modules that encapsulate boilerplate code from sanitizing it. For example, using argparse:

def read_options():
parser = argparse.ArgumentParser(description="A snap game simulator. "
"Perform comparisons on suits and values of consecutively drawn "
"cards to know whether or not give previously drawn cards to a player")

args = parser.parse_args()
if args.no_suits:
compare_cards = compare_cards_by_value
elif args.no_values:
compare_cards = compare_cards_by_suits
else:
compare_cards = compare_cards_by_suits_or_value

return args.number_of_decks, compare_cards


# Proposed improvements

cards.py

import random
from collections import namedtuple

class Card(namedtuple('Card', 'value suits')):
SUITS = ('spades', 'hearts', 'diamonds', 'clubs')
VALUES = (
# No '1' because of 'ACE'
'2', '3', '4', '5', '6', '7', '8',
'9', '10', 'J', 'Q', 'K', 'ACE'
)

def __str__(self):
return '{c.value} of {c.suit}'.format(c=self)

def compare_cards_by_value(card1, card2):
return card1.value == card2.value

def comapare_cards_by_suit(card1, card2):
return card1.suit == card2.suit

def compare_cards_by_suit_or_value(card1, card2):
return compare_cards_by_suit(card1, card2) or compare_cards_by_value(card1, card2)

class Deck(object):
def __init__(self, cards=None):
if cards is None:
cards = [Card(value, suit)
for value in Card.VALUES
for suit in Card.SUITS]
self.cards = cards

return self.__class__(self.cards + other.cards)

@classmethod
def merge(cls, number_of_decks):
return sum((cls() for _ in range(number_of_decks)), cls([]))

def shuffle(self):
random.shuffle(self.cards)

def draw(self):
return self.cards.pop(0)

def __len__(self):
return len(self.cards)

def __nonzero__(self):
return bool(self.cards)

def __str__(self):
return ', '.join(str(card) for card in self.cards)


snap.py

import argparse
import operator
from cards import Deck

class Player(object):
def __init__(self, name, number_of_cards=0):
self.cards = number_of_cards
self.name = name

self.cards += number_of_cards

def __str__(self):
return '{p.name}: {p.cards} cards'.format(p=self)

def format_state(pile, deck, drawn, previously_drawn, is_match, players):
return '''\
pile_size: {}, cards left: {}
card drawn: {}
card previously drawn: {}
They {} match
Players infos: {}'''.format(
pile, deck, drawn, previously_drawn,
'do' if is_match else "don't",
' / '.join(str(p) for p in players))

def game_of_snap(number_of_decks, compare_cards):
deck = Deck.merge(number_of_decks)
deck.shuffle()
players = [Player('player 1'), Player('player 2')]
previous = None
pile = 0

while deck:
card = deck.draw()
pile += 1

try:
match = compare_cards(card, previous)
except AttributeError:
# In case previous is None
match = False

if match:
pile = 0

print format_state(pile, len(deck), card, previous, match, players)

previous = card  # or previous = card if not match else None ???

return max(players, key=operator.attrgetter('cards'))

parser = argparse.ArgumentParser(description="A snap game simulator. "
"Perform comparisons on suits and values of consecutively drawn "
"cards to know whether or not give previously drawn cards to a player")

args = parser.parse_args()
if args.no_suits:
compare_cards = compare_cards_by_value
elif args.no_values:
compare_cards = compare_cards_by_suits
else:
compare_cards = compare_cards_by_suits_or_value

return args.number_of_decks, compare_cards

if __name__ == '__main__':