Optimizing "Poker hands" challenge solution

Question

I'm trying to solve the project Euler problem 54.

In the card game poker, a hand consists of five cards and are ranked, from lowest to highest, in the following way:

• High Card: Highest value card.
• One Pair: Two cards of the same value.
• Two Pairs: Two different pairs.
• Three of a Kind: Three cards of the same value.
• Straight: All cards are consecutive values.
• Flush: All cards of the same suit.
• Full House: Three of a kind and a pair.
• Four of a Kind: Four cards of the same value.
• Straight Flush: All cards are consecutive values of same suit.
• Royal Flush: Ten, Jack, Queen, King, Ace, in same suit.

The cards are valued in the order: 2, 3, 4, 5, 6, 7, 8, 9, 10, Jack, Queen, King, Ace.

If two players have the same ranked hands then the rank made up of the highest value wins; for example, a pair of eights beats a pair of fives (see example 1 below). But if two ranks tie, for example, both players have a pair of queens, then highest cards in each hand are compared (see example 4 below); if the highest cards tie then the next highest cards are compared, and so on.

Consider the following hand dealt to two players:

Player 1 (5H 5C 6S 7S KD), Player 2 (2C 3S 8S 8D TD), Winner: Player 2

The file, poker.txt, contains one-thousand random hands dealt to two players. Each line of the file contains ten cards (separated by a single space): the first five are Player 1's cards and the last five are Player 2's cards. You can assume that all hands are valid (no invalid characters or repeated cards), each player's hand is in no specific order, and in each hand there is a clear winner.

How many hands does Player 1 win?

Following is the code I have written - the code works alright, but I'm interested in learning how I can improve its quality in any possible way.

from collections import Counter

suites = {
    'C' : "Club",
    'D' : "Diamond",
    'H' : "Heart",
    'S' : "Spade",
    }

values = {
    '2' : 2,
    '3' : 3,
    '4' : 4,
    '5' : 5,
    '6' : 6,
    '7' : 7,
    '8' : 8,
    '9' : 9,
    'T' : 10,
    'J' : 11, 
    'Q' : 12,
    'K' : 13,
    'A' : 14,
    }

class InvalidCard(Exception):
    pass

class Card(object):

    def __init__(self, card='AC'):
        if type(card) != str or len(card) != 2:
            raise InvalidCard
        self.value = values[card[0]]
        self.suite = suites[card[1]]
        self.card = card

    def __unicode__(self):
        return "suite: %s value:%s" % (self.suite, self.value)


class InvalidHand(Exception):
    pass

class Hand(object):

    def __init__(self, cards = []):
        if type(cards) != list or len(cards) != 5 or \
                not all(type(c) == Card for c in cards):
            raise InvalidHand
        self.cards = cards
        self.get_rank()

    def print_values(self):
        return "{} {} {} {}".format(self.card_values(),
                                    self.cards_by_hand,
                                    "%2d" % self.rank,
                                    self.combination)

    def values(self):
        return sorted([c.value for c in self.cards])

    def values_desc(self):
        return sorted([c.value for c in self.cards], reverse = True)

    def card_values(self):
        return [c.card for c in self.cards]

    def suites(self):
        return [c.suite for c in self.cards]

    def get_rank(self):
        values = self.values()
        counter_values = Counter(self.values())
        counter_suites = Counter(self.suites())

        self.cards_by_hand = self.values_desc()
        if len(counter_suites.keys()) == 1 and set(values) == set(range(10, 15)):
            self.rank = 1
            self.combination = "Royal Flush"
        elif len(counter_suites.keys()) and \
                set(values) == set(range(values[0], values[0] + 5)):
            self.rank = 2
            self.combination = "Straight Flush"
        elif len(counter_values.keys()) == 2:
            if counter_values.values()[0] in [1, 4]:
                self.rank = 3
                self.combination = "Four Of A Kind"
                if self.cards_by_hand[0] != self.cards_by_hand[1]:
                    self.cards_by_hand = self.cards_by_hand[1:] + self.cards_by_hand[:1]
            else:
                self.rank = 4
                self.combination = "Full House"
                if self.cards_by_hand[0] != self.cards_by_hand[2]:
                    self.cards_by_hand = self.cards_by_hand[2:] + self.cards_by_hand[:2]
        elif len(counter_suites.keys()) == 1 :
            self.rank = 5
            self.combination = "Flush"
        elif set(values) == set(range(values[0], values[0] + 5)):
            self.rank = 6
            self.combination = "Straight"
        elif len(counter_values.keys()) == 3:
            if 3 in counter_values.values():
                self.rank = 7
                self.combination = "Three Of A Kind"
                if not (self.cards_by_hand[0] == self.cards_by_hand[2]) :
                    if not(self.cards_by_hand[1] == self.cards_by_hand[2]):
                        self.cards_by_hand = self.cards_by_hand[2:] + self.cards_by_hand[:2]
                    else:
                        self.cards_by_hand = self.cards_by_hand[1:4] + self.cards_by_hand[:1] + self.cards_by_hand[4:]
            else:
                self.rank = 8
                self.combination = "Double Pair"
                if self.cards_by_hand[0] != self.cards_by_hand[1]:
                    self.cards_by_hand = self.cards_by_hand[1:] + self.cards_by_hand[:1]
                elif self.cards_by_hand[2] != self.cards_by_hand[3]:
                    self.cards_by_hand = self.cards_by_hand[3:] + self.cards_by_hand[:3]
        elif len(counter_values.keys()) == 4:
            self.rank = 9
            self.combination = "Pair"
            if self.cards_by_hand[1] == self.cards_by_hand[2]:
                self.cards_by_hand = self.cards_by_hand[1:3] + self.cards_by_hand[:1] + self.cards_by_hand[3:]
            elif self.cards_by_hand[2] == self.cards_by_hand[3]:
                self.cards_by_hand = self.cards_by_hand[2:4] + self.cards_by_hand[:2] + self.cards_by_hand[4:]
            elif self.cards_by_hand[3] == self.cards_by_hand[4]:
                self.cards_by_hand = self.cards_by_hand[3:] + self.cards_by_hand[:3]
        else:
            self.rank = 10
            self.combination = "High Card"

class InvalidGame(Exception):
    pass

class Game(object):

    def __init__(self, hands = [], game_string = ""):
        if game_string:
            card_strings = game_string.split()
            cards = [Card(string) for string in card_strings]
            hands = [Hand(cards[:5]), Hand(cards[5:])]
        elif type(hands) != list or len(hands) != 2 \
                or not all(type(h) == Hand for h in hands):
            raise InvalidGame
        self.hands = hands

    def compare_hands(self, hands = []):
        if not hands:
            hands = self.hands
        winner = None
        if hands[0].rank != hands[1].rank:
            winner = 0 if hands[0].rank < hands[1].rank else 1
        else:
            winner = 0 if hands[0].cards_by_hand > hands[1].cards_by_hand else 1
        print "%s\n%s\nwinner: %s\n" %(hands[0].print_values(), hands[1].print_values(), str(winner))
        return winner


def construct_game(string):
    game = Game(game_string = string)
    return game.compare_hands()

def main():
    wins_player_A = 0
    with open("poker.txt", "r") as f:
        games = f.read().split("\n")
    for game in games:
        if game:
            wins_player_A += ((construct_game(game) + 1 ) % 2)
    print wins_player_A


if __name__ == "__main__":
    main()

Answer 1

Naming:

Playing card's have suits, not suites (fancy hotel rooms), so adjust your code accordingly.

Also, why does hands[0].cards_by_hand return a list of values and not a list of cards? By the name of it it sounds like it should be hands[0].card_values instead

Documentation:

You have no documentation. Quality code needs documentation, which in the case of Python, is a docstring for every class and function that looks like this:

def factorial(n):
    """Returns n! if n > 0 and if it can be converted to type int"""

Code:

First off, it's good that you made some new exceptions, however, they are of minor use to the programmer without a corresponding message. It would've been better if you had given an already existing exception with a message such as ValueError("Hand has more than 5 cards") rather than an InvalidHand exception with no message because the first gives more info than the second. Ideally of course you would use InvalidHand("Hand has more than 5 cards").

Secondly, you have a big nasty rank function that we can clean up a bit.

• In your check for "Straight Flush" you use len(counter_suits).keys() which works for your purposes (because 1 is taken to be True) but this is not what a straight flush is, since it relies on having just 1 suit (while this condition works for 2, 3, 4 suits), so this should be changed to: len(counter_suits.keys()) == 1

• This leads to another code simplification: instead of checking 1 suit and royal flush, 1 suit and straight, and 1 suit, have an outer condition for 1 suit, and inner conditions for the others like so:

---

if len(counter_suits.keys()) == 1:
    if set(values) == set(range(10, 15)):
        # Royal Flush
    elif set(values) == set(range(values[0], values[0] + 5)):
        # Straight Flush
    else:
        # Flush

---

• Then, go through the counts of the values. But instead of recalculating it every time, calculate the number of distinct values once, and store in a variable.

• I also would add extra helper functions to determine the pair and high card and so on to help trim down the rank function to a decent size.

Finally, let's use some of Python's magic (specifically, magic methods). Instead of manually retrieving values and so on, let's make the Card type work like this:

Card('5H') > Card('4H') # produces True
Card('5H') < Card('4H') # produces False
Card('5H') == Card('5H') # produces True

How do we do this, you ask? Easy, override these 3 methods in Card:

__eq__(self, other):
    if isinstance(other, Card):
        return self.value == other.value
    return False
__lt__(self, other):
    if isinstance(other, Card):
        return self.value < other.value
    return False
__gt__(self, other):
    if isinstance(other, Card):
        return self.value > other.value
    return False

That should help you trim down most of your hand/hand-ranking code