5
\$\begingroup\$

I've read about half of a pretty extensive and popular Python book (I'm a complete beginner) and I decided to get my hands dirty with this little exercise to crystallize all the concepts I've taken in.

The program does as explained above in the title, it creates a showdown situation with random community cards and players in Hold-em and establishes the winner or winners and their hand ranking.

I would appreciate if you can criticise the style in every aspect and spot any non-idiomatic Python there may be. At least it seems to work as intended.

I have a Java background, by the way.

'''

This program finds the winning hand at showdown in texas holdem for
an arbitrary number of players.

For time and development complexity constraints we make the following simplification:
- Any two players or more players with the same hand ranking tie for the pot, regardless of the strength of their hand. 
eg. any pair ties against another pair, regardless of the rank of the pair and kickers.


'''
import random
import enum
import itertools
import collections

CARD_RANK = ['2', '3', '4', '5', '6', '7', '8', '9', 'T', 'J', 'Q', 'K', 'A']

CARD_SUIT = ['heart', 'diamond', 'spade', 'club']

CARD_COUNT = 52

COMMUNITY_CARDS_COUNT = 5

MIN_NUMBER_OF_PLAYERS = 2
MAX_NUMBER_OF_PLAYERS = 10

class HandRanking (enum.Enum):
    HIGH_CARD = 1
    ONE_PAIR = 2
    TWO_PAIR = 3
    THREE_OF_A_KIND = 4
    STRAIGHT = 5
    FLUSH = 6
    FULL_HOUSE = 7
    FOUR_OF_A_KIND = 8
    STRAIGHT_FLUSH = 9

class Showdown (object):
    """A class that represents a showdown situation."""
    def __init__(self, community_cards, players):
        self.community_cards = community_cards
        self.players = players

''' 
Creates the showdown situation with an arbitrary number of players between
two and ten and the five cards on the board
'''
def create_random_showdown (shuffled_deck):
    number_of_players = random.randint (MIN_NUMBER_OF_PLAYERS, MAX_NUMBER_OF_PLAYERS)
    players = {player_position : {shuffled_deck.pop (), shuffled_deck.pop ()} for player_position in range (1, number_of_players + 1)}
    community_cards = {shuffled_deck.pop () for _ in range (COMMUNITY_CARDS_COUNT)}
    return Showdown (community_cards, players)

def _is_straight_flush (cards):
    return _is_straight (cards) and _is_flush (cards)

def _is_flush (cards):
    suit_count = len ({c [1] for c in cards})
    return suit_count == 0

def _is_four_of_a_kind (cards):
    rank_set = {c [0] for c in cards}
    return len (rank_set) == 1

def _is_full_house (cards):
    rank_set = {c [0] for c in cards}
    return len (rank_set) == 2

def _is_one_pair (cards):
    rank_set = {c [0] for c in cards}
    return len (rank_set) == 4

def _is_two_pair (cards):
    rank_set = {c [0] for c in cards}
    if len (rank_set) != 3:
        return False
    rank_list = [c [0] for c in cards]
    most_common_rank_count = collections.Counter (rank_list).most_common (1) [0] [1]
    return most_common_rank_count == 2

def _is_three_of_a_kind (cards):
    rank_set = {c [0] for c in cards}
    if len (rank_set) != 3:
        return False
    rank_list = [c [0] for c in cards]
    most_common_rank_count = collections.Counter (rank_list).most_common (1) [0] [1]
    return most_common_rank_count == 3

def _is_straight (cards):
    # All ranks must be diferent
    rank_set = {c [0] for c in cards}
    if len (rank_set) != 5:
        return False

    rank_set = {c [0] for c in cards}
    ordered_ranks_minus_our_cards_ranks = [None if x in rank_set else x for x in CARD_RANK]

    first_index = ordered_ranks_minus_our_cards_ranks.index (None)
    last_index = len (CARD_RANK) - ordered_ranks_minus_our_cards_ranks[::-1].index (None) - 1

    if last_index - first_index == (5 - 1):
        return True

    #Special case for A2345
    first_not_removed_rank = [x for x in ordered_ranks_minus_our_cards_ranks if x != None][0]
    if ordered_ranks_minus_our_cards_ranks [len (CARD_RANK) - 1] == None and first_not_removed_rank == '6':
        return True

    return False

def determine_hand_ranking (cards):
    if len (cards) != 5:
        raise ValueError ("'cards' must have exactly five cards")

    if _is_straight_flush (cards):
        return  HandRanking.STRAIGHT_FLUSH

    if _is_four_of_a_kind (cards):
        return  HandRanking.FOUR_OF_A_KIND

    if _is_full_house (cards):
        return  HandRanking.FULL_HOUSE

    if _is_flush (cards):
        return  HandRanking.FLUSH

    if _is_straight (cards):
        return  HandRanking.STRAIGHT

    if _is_three_of_a_kind (cards):
        return HandRanking.THREE_OF_A_KIND

    if _is_two_pair (cards):
        return HandRanking.TWO_PAIR

    if _is_one_pair (cards):
        return HandRanking.ONE_PAIR

    return HandRanking.HIGH_CARD

def main ():
    deck = [ (rank, suit) for rank in CARD_RANK for suit in CARD_SUIT]
    random.shuffle (deck)
    showdown = create_random_showdown (deck)
    players_and_hand_rankings = dict ()
    for position, hole_cards in showdown.players.items ():
        hand_ranking = HandRanking.HIGH_CARD
        all_cards = hole_cards.union (showdown.community_cards)
        cards_combinations_for_a_hand = itertools.combinations (all_cards, 5)
        for cards in cards_combinations_for_a_hand:
            tmp_hand_ranking = determine_hand_ranking (cards)
            if tmp_hand_ranking.value > hand_ranking.value:
                hand_ranking = tmp_hand_ranking
        players_and_hand_rankings [position] = hand_ranking
    print ('Community cards: ', showdown.community_cards)
    print ('Players: ', showdown.players)
    # Establish the winner
    winning_players = set ()
    highest_hand_ranking = HandRanking.HIGH_CARD
    for position, hand_ranking in players_and_hand_rankings.items ():
        if hand_ranking.value > highest_hand_ranking.value:
            winning_players.clear()
            winning_players.add (position)
            highest_hand_ranking = hand_ranking
        elif hand_ranking.value == highest_hand_ranking.value:
            winning_players.add (position)
    # Print the results
    if len (winning_players) == 1:
        print('Player ', next (iter (winning_players)) , ' wins pot with ', highest_hand_ranking)
    else:
        print('Players ', winning_players, ' tie for pot with ', highest_hand_ranking)

if __name__ == "__main__": main()
\$\endgroup\$
  • \$\begingroup\$ I don't know Python but it would appear that there is some condensing you could do with loops for those long lists of ifs. \$\endgroup\$ – Ulthran Dec 16 '15 at 1:53
4
\$\begingroup\$

Comparing hand rankings

At a high level, this code is incorrect. determine_hand_ranking yields you some enum, and then you compare those values - but hand ranking alone is insufficient to do a comparison. For instance, AAKKQ beats 7755A, but you would only get that correct if the aces up hand went first. You need more info than simply the rankings.

Repetitions

Several of the hand ranking formulas are based on determining how many of what card you have. Your logic for _is_four_of_a_kind is incorrect - that would be the logic for five of a kind. But rather than having lots of independent functions for this, determine_hand_ranking should just figure that all out internally. Using Counter for that is good - but just do it once, in one spot.

Card representation

Having (rank, suit) is not very helpful. Especially when you reference those things by index. Instead, add names to those tuples with namedtuple:

Card = collections.namedtuple('Card', 'rank suit')

This way, you can refer to card.rank or card.suit directly. The card suit should be an enum.Enum as well, instead of just an arbitrary string - it'll be easier to compare against. Also it'll make your life easier if you make the ranks just the values 2 thru 14. For example:

def _is_straight(cards):
    ranks = sorted(c.rank for c in ranks)
    # two cases: the wheel and not the wheel
    return ranks in (list(range(ranks[0], ranks[0]+5)), [2, 3, 4, 5, 14])

Finding the best ranking

I said before the determine_hand_ranking should yield something more complex than just an enum. But it should yield something comparable. This will let you reduce this loop:

hand_ranking = HandRanking.HIGH_CARD
all_cards = hole_cards.union (showdown.community_cards)
cards_combinations_for_a_hand = itertools.combinations (all_cards, 5)
for cards in cards_combinations_for_a_hand:
    tmp_hand_ranking = determine_hand_ranking (cards)
    if tmp_hand_ranking.value > hand_ranking.value:
        hand_ranking = tmp_hand_ranking

to this one:

max_hand_ranking = max(
    itertools.combinations(all_cards, 5),
    key=determine_hand_ranking
)

Although...

Finding the best ranking, II

Instead of taking all 5-card combinations of 7 cards and determining the max of that (which would involve 21 iterations), just determine the best ranking looking at all 7 cards together. That's the really the same amount of work as finding the rank for 5 cards at a time, but instead - solve the whole problem.

Spacing

There's a lot of places where you put spaces in front of parentheses or brackets (e.g. random.shuffle (deck) or dict () or player_and_hand_rankings [position], etc.). Don't put spaces there.

DO put a space after colons: so main() should be on its own line.

There's a lot more to say, but for now this code is fundamentally wrong.

\$\endgroup\$
  • \$\begingroup\$ Thanks, that was helpful. The code is indeed wrong at a high level, it's explained in the module comment at the top. Maybe I'll fix it in another practice session but I felt it was enough work for now. \$\endgroup\$ – DPM Dec 16 '15 at 3:00
1
\$\begingroup\$

It's good that you're using the right format for constant names. It would also be good to use tuples instead of lists though. Tuples are a lot like lists, except their contents can't be modified so it communicates their constant nature more clearly.

CARD_RANK = ('2', '3', '4', '5', '6', '7', '8', '9', 'T', 'J', 'Q', 'K', 'A')
CARD_SUIT = ('heart', 'diamond', 'spade', 'club')

Your Showdown class is strange and seems unnecessary. It just holds two attributes and has not functions. You could literally have create_random_showdown just return players, community_cards and then access those directly when it's returned:

players, community_cards = create_random_showdown(deck)

There's no need to pair them together when you just access them separately anyway.

You use position and player_position as quite verbose names for an index multiple times. Instead, consider using i or n if you want to distance the player numbers being considered indices. Either would make your code shorter and actually clearer, as i is often used with range to denote the looping number. In your current code I often thought position was something else, especially when you iterated over the dictionaries later.

Using dict() to make an empty dict is rarely done and looks odd, just use {}.

players_and_hand_rankings = dict ()

cards_combinations_for_a_hand is redundant. You could iterate directly over itertools.combinations(all_cards, 5) and it would be a lot clearer to read:

    for cards in itertools.combinations (all_cards, 5):
        tmp_hand_ranking = determine_hand_ranking (cards)
        if tmp_hand_ranking.value > hand_ranking.value:
            hand_ranking = tmp_hand_ranking

You could actually shorten that a lot too by using max. max will take the highest value from a collection, and as @Barry pointed out, you can use the key parameter to give it a function to use for sorting. In your case, if you pass determine_hand_ranking that function will be run on each element and the highest result found will be returned:

hand_ranking = max(itertools.combinations(all_cards, 5), key=determine_hand_ranking)

max calls determine_hand_ranking on every element in itertools.combinations(all_cards, 5) and then returns the highest value from those calculations.

There's also no need to pre-set hand_ranking since your function will always at least return hand_ranking.HIGH_CARD anyway.

\$\endgroup\$
  • 1
    \$\begingroup\$ max(x, key=f) is much better than max(map(f, x)) - no need to create the intermediate list. \$\endgroup\$ – Barry Dec 17 '15 at 15:27
  • \$\begingroup\$ @Barry Excellent point, I didn't realise max had a key parameter. \$\endgroup\$ – SuperBiasedMan Dec 17 '15 at 15:31
  • \$\begingroup\$ Thank you for the suggestions, I've improved the code a lot with the suggestions in both replies. I tend to create intermediate variables just to declare what I'm trying to accomplish at the cost of one extra line but I think is fine either way. \$\endgroup\$ – DPM Dec 17 '15 at 18:29

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.