Comparison to boolean values
This has been said in other reviews before but instead of
if cond:
return True
return False
You can and should simply write:
return cond
This comments applies to various parts of your code:
def is_two_pair(hand):
"""returns True for two pair, else False."""
hand_values = [get_card_value(card) for card in hand]
val_count = {card: hand_values.count(card) for card in hand_values}
pair_count = sum(1 for val in val_count.values() if val == 2)
return pair_count == 2
def is_three_of_a_kind(hand):
"""returns True for three of a kind, else False."""
hand_values = [get_card_value(card) for card in hand]
val_count = {card: hand_values.count(card) for card in hand_values}
return 3 in val_count.values()
def is_full_house(hand):
"""returns True for a full house, False otherwise."""
return is_one_pair(hand) and is_three_of_a_kind(hand)
def is_four_of_a_kind(hand):
"""returns True for four of a kind, False otherwise."""
hand_values = [get_card_value(card) for card in hand]
val_count = {card: hand_values.count(card) for card in hand_values}
return 4 in val_count.values()
def is_straight_flush(hand):
"""returns True for a straight flush, False otherwise."""
return is_straight(hand) and is_flush(hand)
Improvements of is_royal_flush
You can return directly when it is not a flush:
def is_royal_flush(hand):
"""returns True for a royal flush, False otherwise."""
if not is_flush(hand):
return False
hand_vals = [get_card_value(card) for card in hand]
valid_cards = ['A', 'K', 'Q', 'J', 'T']
for valid in valid_cards:
if valid not in hand_vals:
return False
return True
You could use the all builtin:
hand_vals = [get_card_value(card) for card in hand]
valid_cards = ['A', 'K', 'Q', 'J', 'T']
return all(valid in hand_vals for valid in valid_cards)
You could also use set comparison:
hand_vals = {get_card_value(card) for card in hand}
valid_cards = {'A', 'K', 'Q', 'J', 'T'}
return hand_vals == valid_cards
Improvements in handling of hands
Once again, this has been said before but: in general, it is best to avoid using range when what you want is to iterate over an iterable - see also Ned Batchelder's excellent talk: "Loop like a native".
Here, we'd have:
scores = [compare_hands(h1, h2) for (h1, h2) in zip(hands1, hands2)]
Note: this leads to a slightly different behavior as the last hand used to be ignored. Was this a bug ?
Also this suggests that things could be done in a different way: instead of returning a tuple of lists, it would make more sense to return a list of tuples.
def get_hands(filename):
"""returns player 1 and player 2 hands."""
hands = open(filename).read().rstrip().split('\n')
return [(hand.split()[:5], hand.split()[5:]) for hand in hands]
...
hands = get_hands('p054_poker.txt')
scores = [compare_hands(h1, h2) for (h1, h2) in hands]
Also, we could call split only once on each string:
def get_hands(filename):
"""returns player 1 and player 2 hands."""
lines = open(filename).read().rstrip().split('\n')
splitted_lines = [l.split() for l in lines]
return [(l[:5], l[5:]) for l in splitted_lines]
Improve get_first_hand_max
The function uses a dictionnary without any particular reason.
We could use a set with little or no change:
def get_first_hand_max(hand):
"""returns the value of the 1st maximum card in hand."""
hand_vals = {CARD_RANKS[card[:-1]] for card in hand}
return max(hand_vals)
or even no particular data structure:
def get_first_hand_max(hand):
"""returns the value of the 1st maximum card in hand."""
return max(CARD_RANKS[card[:-1]] for card in hand)
Improve is_straight
The inner loop could use a break to stop as soon as check is set to 0.
This is also a good chance to get familiar with the optional else clause for a for loop:
for value in valid_range:
if value not in hand_vals:
break
else: # no break
return True
Alternatively, this could be written with all:
def is_straight(hand):
"""returns True for a straight, False otherwise."""
valid_ranges = [['A', '2', '3', '4', '5'], ['6', '7', '8', '9', 'T']]
for index in range(2, 6):
valid = [str(num) for num in range(index, index + 5)]
valid_ranges.append(valid)
hand_vals = [get_card_value(card) for card in hand]
for valid_range in valid_ranges:
if all(value in hand_vals for value in valid_range):
return True
return False
Consistency
You have defined get_card_value. You should use it in get_first_hand_max and get_second_hand_max:
def get_first_hand_max(hand):
"""returns the value of the 1st maximum card in hand."""
return max(CARD_RANKS[get_card_value(card)] for card in hand)
def get_second_hand_max(hand):
"""returns value of the 2nd maximum card in hand."""
hand_vals = sorted([(card, CARD_RANKS[get_card_value(card)]) for card in hand], key=operator.itemgetter(1), reverse=True)
return hand_vals[1][1]
Using classes
Code could be reorganised by using well defined data structure.
For example, instead of using strings everywhere, we could use a Card object.
This can be introduced with minimal changes:
class Card:
def __init__(self, value, suit):
self.value = value
self.suit = suit
@classmethod
def from_string(cls, card):
value = card[:-1]
suit = card[-1]
return cls(value, suit)
def __str__(self):
return self.value + ", " + self.suit
def __repr__(self):
return self.__class__.__name__ + "(" + self.value + ", " + self.suit +")"
def get_hands(filename):
"""returns player 1 and player 2 hands."""
lines = open(filename).read().rstrip().split('\n')
splitted_lines = [[Card.from_string(c) for c in l.split()] for l in lines]
return [(l[:5], l[5:]) for l in splitted_lines]
def get_card_value(card):
"""returns card value ex: KC --> K."""
return card.value
def get_card_suit(card):
"""returns card suit ex: 10D --> D."""
return card.suit
Once this is done, we can easily get rid of get_card_value and get_card_suit.
Using Counter
You can use Counter from the collection module
Improving get_hand_score
get_hand_score returns both a hand and a score. I don't really understand how returning the hand is useful.
We can simplify the code:
def get_hand_score(hand):
"""returns the score."""
scores = {is_one_pair(hand): 1, is_two_pair(hand): 2, is_three_of_a_kind(hand): 3, is_straight(hand): 4,
is_flush(hand): 5, is_full_house(hand): 6, is_four_of_a_kind(hand): 7, is_straight_flush(hand): 8,
is_royal_flush(hand): 9}
total = 0
for x, y in scores.items():
if x:
total += y
return total
def compare_hands(hand1, hand2):
"""returns 1 for hand1 or 2 for hand2 if either wins."""
score1 = get_hand_score(hand1)
score2 = get_hand_score(hand2)
Also, we could use sum in get_hand_score.
def get_hand_score(hand):
"""returns the score."""
scores = {is_one_pair(hand): 1, is_two_pair(hand): 2, is_three_of_a_kind(hand): 3, is_straight(hand): 4,
is_flush(hand): 5, is_full_house(hand): 6, is_four_of_a_kind(hand): 7, is_straight_flush(hand): 8,
is_royal_flush(hand): 9}
return sum(y for x, y in scores.items() if x)
Simplify compare_hands
You can consider:
if score1 > score2:
return 1
if score2 > score1:
return 2
at the beginning of the function so that you can assume score1 == score2 in the following part of the function.
Then, we get:
def compare_hands(hand1, hand2):
"""returns 1 for hand1 or 2 for hand2 if either wins."""
score1 = get_hand_score(hand1)
score2 = get_hand_score(hand2)
if score1 > score2:
return 1
if score2 > score1:
return 2
assert score1 == score2
if score1 == 0:
max1 = get_first_hand_max(hand1)
max2 = get_first_hand_max(hand2)
if max1 > max2:
return 1
if max2 > max1:
return 2
assert max1 == max2
max11 = get_second_hand_max(hand1)
max22 = get_second_hand_max(hand2)
if max11 > max22:
return 1
if max22 > max11:
return 2
if score1 == 1:
max1 = CARD_RANKS[is_one_pair(hand1)[1]]
max2 = CARD_RANKS[is_one_pair(hand2)[1]]
if max1 > max2:
return 1
if max2 > max1:
return 2
Improve is_one_pair
The docstring is wrong as we do return only True.
We could compute pair_val only in the pair_count == 1 case.
Also, we could iterate over the Counter directly:
def is_one_pair(hand):
"""returns True for one pair, else False."""
hand_values = [card.value for card in hand]
val_count = Counter(hand_values)
pairs = [value for value, count in val_count.items() if count == 2]
if len(pairs) == 1:
return True, pairs[0]
return False
More simplification in get_hand_score
At the moment, the function calls many functions and somehow adds their result.
We could just try everything starting from the higher value to the smaller value until we find a match:
def get_hand_score(hand):
"""returns the score."""
if is_royal_flush(hand): return 9
if is_straight_flush(hand): return 8
if is_four_of_a_kind(hand): return 7
if is_full_house(hand): return 6
if is_flush(hand): return 5
if is_straight(hand): return 4
if is_three_of_a_kind(hand): return 3
if is_two_pair(hand): return 2
if is_one_pair(hand): return 1
return 0
Or using a data structure (note that unlike in your code, we don't call always call all the functions):
def get_hand_score(hand):
"""returns the score."""
scores = [
(is_royal_flush, 9),
(is_straight_flush, 8),
(is_four_of_a_kind, 7),
(is_full_house, 6),
(is_flush, 5),
(is_straight, 4),
(is_three_of_a_kind, 3),
(is_two_pair, 2),
(is_one_pair, 1),
]
for func, score in scores:
if func(hand):
return score
return 0
Storing directly rank instead of value
We could compute the ranks from the Card class directly and never rely on the "stringed" value:
@classmethod
def from_string(cls, card):
value = CARD_RANKS[card[:-1]]
suit = card[-1]
return cls(value, suit)
There are a few places to change so I'll let you do it.
Stopping at this point
At this stage, the code I have is:
import operator
from time import time
from collections import Counter
CARD_RANKS = {'2': 1, '3': 2, '4': 3, '5': 4, '6': 5, '7': 6, '8': 7, '9': 8, 'T': 9, 'J': 10, 'Q': 11, 'K': 12, 'A': 13}
class Card:
def __init__(self, value, suit):
self.value = value
self.suit = suit
@classmethod
def from_string(cls, card):
value = card[:-1] # CARD_RANKS[card[:-1] could be nice
suit = card[-1]
return cls(value, suit)
def __str__(self):
return self.value + ", " + self.suit
def __repr__(self):
return self.__class__.__name__ + "(" + self.value + ", " + self.suit +")"
def get_hands(filename):
"""returns player 1 and player 2 hands."""
lines = open(filename).read().rstrip().split('\n')
splitted_lines = [[Card.from_string(c) for c in l.split()] for l in lines]
return [(l[:5], l[5:]) for l in splitted_lines]
def is_one_pair(hand):
"""returns True for one pair, else False."""
pairs = [value for value, count in Counter(card.value for card in hand).items() if count == 2]
if len(pairs) == 1:
return True, pairs[0]
return False
def is_two_pair(hand):
"""returns True for two pair, else False."""
pairs = [value for value, count in Counter(card.value for card in hand).items() if count == 2]
return len(pairs) == 2
def is_three_of_a_kind(hand):
"""returns True for three of a kind, else False."""
return 3 in Counter(card.value for card in hand).values()
def is_straight(hand):
"""returns True for a straight, False otherwise."""
valid_ranges = [['A', '2', '3', '4', '5'], ['6', '7', '8', '9', 'T']]
for index in range(2, 6):
valid = [str(num) for num in range(index, index + 5)]
valid_ranges.append(valid)
hand_vals = [card.value for card in hand]
for valid_range in valid_ranges:
if all(value in hand_vals for value in valid_range):
return True
return False
def is_flush(hand):
"""returns True for a flush, False otherwise."""
return len(set(card.suit for card in hand)) == 1
def is_full_house(hand):
"""returns True for a full house, False otherwise."""
return is_one_pair(hand) and is_three_of_a_kind(hand)
def is_four_of_a_kind(hand):
"""returns True for four of a kind, False otherwise."""
val_count = Counter(card.value for card in hand)
return 4 in val_count.values()
def is_straight_flush(hand):
"""returns True for a straight flush, False otherwise."""
return is_straight(hand) and is_flush(hand)
def is_royal_flush(hand):
"""returns True for a royal flush, False otherwise."""
if not is_flush(hand):
return False
hand_vals = {card.value for card in hand}
valid_cards = {'A', 'K', 'Q', 'J', 'T'}
return hand_vals == valid_cards
def get_first_hand_max(hand):
"""returns the value of the 1st maximum card in hand."""
return max(CARD_RANKS[card.value] for card in hand)
def get_second_hand_max(hand):
"""returns value of the 2nd maximum card in hand."""
hand_vals = sorted([(card, CARD_RANKS[card.value]) for card in hand], key=operator.itemgetter(1), reverse=True)
return hand_vals[1][1]
def get_hand_score(hand):
"""returns the score."""
scores = [
(is_royal_flush, 9),
(is_straight_flush, 8),
(is_four_of_a_kind, 7),
(is_full_house, 6),
(is_flush, 5),
(is_straight, 4),
(is_three_of_a_kind, 3),
(is_two_pair, 2),
(is_one_pair, 1),
]
for func, score in scores:
if func(hand):
return score
return 0
def compare_hands(hand1, hand2):
"""returns 1 for hand1 or 2 for hand2 if either wins."""
score1 = get_hand_score(hand1)
score2 = get_hand_score(hand2)
if score1 > score2:
return 1
if score2 > score1:
return 2
assert score1 == score2
if score1 == 0:
max1 = get_first_hand_max(hand1)
max2 = get_first_hand_max(hand2)
if max1 > max2:
return 1
if max2 > max1:
return 2
assert max1 == max2
max11 = get_second_hand_max(hand1)
max22 = get_second_hand_max(hand2)
if max11 > max22:
return 1
if max22 > max11:
return 2
if score1 == 1: # One Pairs
max1 = CARD_RANKS[is_one_pair(hand1)[1]]
max2 = CARD_RANKS[is_one_pair(hand2)[1]]
if max1 > max2:
return 1
if max2 > max1:
return 2
if __name__ == '__main__':
start_time = time()
hands = get_hands('p054_poker.txt')
scores = [compare_hands(h1, h2) for (h1, h2) in hands]
player1_score = sum(1 for player in scores if player == 1)
print(f'Player 1 score: {player1_score}')
print(f'Time: {time() - start_time} seconds.')
Many details can still be improved.
My own solution
Reviewing your code got me thinking about how I'd implement the solution.
I don't consider my code to be great in any ways but it may be interesting to you on various points:
- code organisation
- usage of Python standard modules
import collections
class Card:
"""Card object (value and suit)."""
CARD_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}
def __init__(self, value, suit):
self.value = value
self.suit = suit
@classmethod
def from_string(cls, card):
value, suit = card
return cls(value, suit)
def __str__(self):
return str(self.value) + self.suit
def __repr__(self):
return self.__class__.__name__ + "(" + self.value + ", " + self.suit + ")"
def evaluate(self):
return Card.CARD_VALUES[self.value]
class Hand:
"""Hand object (iterable of NB_CARDS cards)."""
NB_CARDS = 5
def __init__(self, cards):
assert len(cards) == Hand.NB_CARDS
self.cards = cards
@classmethod
def from_string(cls, string):
cards = [Card.from_string(chunk) for chunk in string.split()]
return cls(cards[:Hand.NB_CARDS]), cls(cards[Hand.NB_CARDS:])
def __str__(self):
return "-".join(str(c) for c in self.cards)
def __repr__(self):
return self.__class__.__name__ + "(" + repr(self.cards) + ")"
def evaluate(self):
"""Return an arbitrarly formed tuple that can be used to
sort hands using lexicographic order. First element is an
integer describing the type of hand. Other values are added
to be able to differentiate hands.
Integers used:
1 High Card: Highest value card.
2 One Pair: Two cards of the same value.
3 Two Pairs: Two different pairs.
4 Three of a Kind: Three cards of the same value.
5 Straight: All cards are consecutive values.
6 Flush: All cards of the same suit.
7 Full House: Three of a kind and a pair.
8 Four of a Kind: Four cards of the same value.
9 Straight Flush: All cards are consecutive values of same suit.
9 Royal Flush: Ten, Jack, Queen, King, Ace, in same suit.
"""
values = sorted((c.evaluate() for c in self.cards), reverse=True)
count = collections.Counter(values)
mc, mc2 = count.most_common(2)
mc_val, mc_nb = mc
mc2_val, mc2_nb = mc2
if mc_nb == 4:
return (8, mc_val, values)
elif mc_nb == 3:
if mc2_nb == 2:
return (7, mc_val, mc2_val, values)
else:
return (4, mc_val, values)
elif mc_nb == 2:
if mc2_nb == 2:
return (3, sorted((mc_val, mc2_val)), values)
else:
return (2, mc_val, values)
else:
assert mc_nb == 1
is_flush = len(set(c.suit for c in self.cards)) == 1
delta = values[0] - values[-1]
is_straight = delta == Hand.NB_CARDS - 1
if is_straight:
return (9 if is_flush else 5, values)
else:
return (6 if is_flush else 1, values)
def __gt__(self, other): # Note: other magic methods should be defined as well
return self.evaluate() > other.evaluate()
def euler54(f='p054_poker.txt'):
"""Solution for problem 54."""
ret = 0
with open(os.path.join(resource_folder, f)) as file_:
for l in file_:
hand1, hand2 = Hand.from_string(l)
ret += hand1 > hand2
return ret
