5
\$\begingroup\$

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.

Your solution should ask:

  • 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:

task/main.py:

from utils import read_options_from_stdin

from snap.game import Game


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

    while not game.finished:
        game.turn()

    print game.winner

task/utils.py:

from snap.consts import MATCHING_CONDITIONS


def read_options_from_stdin():
    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:
            print 'Please provide a number'

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

        matching_condition = raw_input('''
        Please choose a matching condition.
        Options are: {mathcing_consitions}
        '''.format(mathcing_consitions=MATCHING_CONDITIONS.keys()))

    return number_of_packs, matching_condition

task/snap/card.py:

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

task/snap/consts.py:

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_SPADES = 'spades'
SUIT_HEARTS = 'hearts'
SUIT_DIAMONDS = 'diamonds'
SUIT_CLUBS = 'clubs'

SUITS = [
    SUIT_SPADES,
    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,
]

task/snap/deck.py:

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)

    def __iadd__(self, other):
        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])

task/snap/game.py:

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:
            random.choice(self.players).add_pile(self.pile_size)
            self.pile_size = 0

        self.print_state(drawn_card, match)

        self.previously_drawn = drawn_card

task/snap/player.py:

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)

    def add_pile(self, number_of_cards):
        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?

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

"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:
        card.adopt(comparison)

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – fodma1 Jul 14 '16 at 14:41
  • \$\begingroup\$ 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__. \$\endgroup\$ – Peilonrayz Jul 15 '16 at 9:02
4
\$\begingroup\$

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

    def add_pile(self, number_of_cards):
        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")
    parser.add_argument('number_of_decks', type=int)
    group = parser.add_mutually_exclusive_group()
    group.add_argument('--no-suits', action='store_true', help='Disable suits comparison')
    group.add_argument('--no-values', action='store_true', help='Disable values comparison')

    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

    def __add__(self, other):
        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

    def add_pile(self, number_of_cards):
        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:
            random.choice(players).add_pile(pile)
            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'))


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")
    parser.add_argument('number_of_decks', type=int)
    group = parser.add_mutually_exclusive_group()
    group.add_argument('--no-suits', action='store_true', help='Disable suits comparison')
    group.add_argument('--no-values', action='store_true', help='Disable values comparison')

    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__':
    decks, compare = read_options()
    print game_of_snap(decks, compare)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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