7
\$\begingroup\$

I'm working right now on Project Euler problem 54, and I figured that this was the perfect opportunity to try to work with classes. This is my first time, so I'm sure that there are a lot of style, efficiency or functionality errors. I would like some insights for that.

The Class is initiated with a string consisting of the cards, i.e., '2C AC 8H 9D 9S', and the Hand inherits several variables that I'm interested in:

Hand.rank and Hand.rank_val # String with the rank name and numerical value of the rank Hand.cards and Hand.num_vals # Lists of cards and numerical value of the cards Hand.max_val # Value of the highest card in the rank Hand.rest_vals # Rest of values for the cards that are not the highest in the rank

There are also the variables:

all_vals, order_vals, all_ranks and order_ranks

that give the custom sort order for the cards and the ranks.

It works as I want it to, at least for all the tests that I've given it (which were all that I could think of). I haven't implemented a test to check if the hand is impossible (repeated or non existing cards), but I may do it in the future. The next step is to create a process that compares two hands and gives the winner. Should I do a function or a Class method?

Anyway, here is the code of my program. It's a bit long, so thank you to anyone who takes some time to read it:

from collections import Counter class Hand: '''Class representing the rank of a hand in poker.''' # Possible card values all_vals = [ '2', '3', '4', '5', '6', '7', '8', '9', 'T', 'J', 'Q', 'K', 'A'] # Custom order for the values order_vals = dict(zip(all_vals, range(len(all_vals)))) # Possible ranks (including empty/unknown rank) all_ranks = ['', 'High Card', 'One Pair', 'Two Pairs', 'Three of a Kind', 'Straight', 'Flush', 'Full House', 'Four of a Kind', 'Straight Flush', 'Royal Flush'] # Custom order for the ranks order_ranks = dict(zip(all_ranks, range(len(all_ranks)))) def __init__(self, cards): self.cards = cards.split(' ') self.vals_and_suits() self.calculate_rank() def vals_and_suits(self): self.vals = [card[0] for card in self.cards] # Ordered values of the cards with custom sort self.vals.sort(key=lambda val: self.order_vals[val]) # Numeric values of the cards with custom sort self.num_vals = [self.order_vals[val] for val in self.vals] # Suits of the cards self.suits = [card[1] for card in self.cards] def calculate_rank(self): def flush_and_straight(): '''Checks all combinations of flush and straight.''' # Generate new list with range lowest and highest numeric values min_max = list(range(self.num_vals[0], self.num_vals[4] + 1)) # If the list hasn't changed, cards are consecutive consecutive = (self.num_vals == min_max) # And we also need to consider the case of a wheel straight (1*) # where the card values are 2, 3, 4, 5 and Ace wheel_num_vals = [0, 1, 2, 3, 12] wheel = (self.num_vals == wheel_num_vals) # Straight if the cards are consecutive or wheel if consecutive or wheel: self.rank = 'Straight' # If all cards from the same suit if (len(set(self.suits)) == 1): # Flush if not Straight if self.rank != 'Straight': self.rank = 'Flush' # Add cards different than max to compare between flushes self.rest_vals += self.num_vals[:-1] else: # Straight Flush if smallest card is not a 10 if self.num_vals[0] != 8: self.rank = 'Straight Flush' # Royal Flush otherwise else: self.rank = 'Royal Flush' # If any value has been specified for rank if self.rank != "": if wheel: # Value for the wheel self.max_val = 3 else: # Value for anything else self.max_val = self.num_vals[-1] # Value for the current rank self.rank_val = self.order_ranks[self.rank] def repeated_cards(): # Counter for the repeated values of the cards count_num_vals = Counter(self.num_vals) # Returns a list [(value, count),(value, count),...] order by count popular = count_num_vals.most_common() # Most common values most_popular = popular[0] # Only combinations for len == 2 are 4-1 and 3-2 if len(popular) == 2: # Four of a Kind if the most common count is equal to four if most_popular[1] == 4: self.rank = 'Four of a Kind' print(self.rest_vals, popular) self.rest_vals += [popular[1][0]] # Full House if it is equal to three else: self.rank = 'Full House' self.rest_vals += [popular[1][0]]*2 self.max_val = most_popular[0] self.rank_val = self.order_ranks[self.rank] # Only if the rank is less than Straight if self.rank_val <= 4: # Three of a Kind if most common is equal to three if most_popular[1] == 3: self.rank = 'Three of a Kind' self.max_val = most_popular[0] self.rest_vals += [popular[1][0]] + [popular[2][0]] # Two Pairs if first and second most common are equal to two elif popular[0][1] == 2 and popular[1][1] == 2: self.rank = 'Two Pairs' self.max_val = popular[1][0] # Add card not in pairs and lowest valued pair (that order) self.rest_vals += sorted([popular[2][0]]+[popular[0][0]]*2) # One Pair if previous test failed and most common is two elif popular[0][1] == 2: self.rank = 'One Pair' self.max_val = popular[0][0] # Add the remaining cards self.rest_vals += [popular[1][0]] \ + [popular[2][0]] + [popular[3][0]] # High Card if all tests until now have failed else: self.rank = 'High Card' self.max_val = self.num_vals[-1] # Add the remaining cards self.rest_vals += self.num_vals[:-1] self.rank_val = self.order_ranks[self.rank] # Init rank name, max_val, rank_val and rest_vals # Name of the rank self.rank = '' # Value of the rank self.rank_val = -1 # Should change eventually # Max card value for particular rank self.max_val = -1 # Should change eventually # Excess of card values not in particular rank self.rest_vals = [] flush_and_straight() # Not RF or SF if self.rank_val <= 8: repeated_cards() if __name__ == '__main__': # tests hand1 = Hand('5C 2H 2C AD AC') print("This hand is a", hand1.rank, "with rank value", hand1.rank_val) print("The cards are", hand1.cards, "and their values are", hand1.num_vals) print("The highest card value in the rank is", hand1.max_val) print("The remaining values in the hand are", hand1.rest_vals) ''' #-------# # Notes # #-------# (1*) If there is a wheel straight, self.vals are ['2', '3', '4', '5', 'A'] and self.num_vals are [0, 1, 2, 3, 4, 12] '''
\$\endgroup\$
2
  • \$\begingroup\$ could if self.rank != "" be replaced by if self.rank? \$\endgroup\$ Commented Feb 28 at 10:33
  • \$\begingroup\$ @coder yes, you're right! Thanks for pointing that out \$\endgroup\$ Commented Feb 28 at 10:37

4 Answers 4

Reset to default
9
\$\begingroup\$

I encourage you to read the other answers, they have good advice. I'll focus on using classes.

What is a type?

That's a fairly abstract question, I recognize, but it's a crucial part of the foundations that all developers need.

A type is a multi-faceted concept:

  • It is a name. This is important, because names carry semantic information in general, and it's doubly important in nominally typed language where even if A and B are otherwise identical, an A is NOT a B.
  • It is a set of values. For example, there's no Golf/G suit. If we were to model Suit as its own type (an enumerated one), the only valid values would be Spade (S), Diamond (D), Club (C), and Heart (H). Nothing else.
  • It is a set of operations. The oft forgotten property. Continuing with the Suit type, an addition operation would likely not be very sensible: what's Club + Diamond? What's Club + "hello"?

What is a class?

A class is a representation, in a programming language, of a type.

In a nominally typed language, a class thus:

  • Has a name.
  • Has a set of values.
  • Has a set of operations.

The latter, in particular, is where you've erred: are vals_and_suits or calculate_rank meaningful operations on your Hand class (as they are defined?). No, they are not.

This doesn't mean you don't need them, necessarily, but it does mean they are only helper function not meaningful operations offered to the class users, and therefore should be private. Just prefix them with _ (one underscore) to signal to the user those are implementation details.

Big Ball of Mud

You may have heard that global variables are bad? The general issue of global variables is that there's no telling which function read/write them, at a glance, and thus it's impossible to locally reason about a piece of code: you have to know the full call graph, and which of the functions in call graph potentially refer to the global variables, and under which conditions. That's how you end up with a big ball of mud.

Thing is, though, it's very easy to turn a class in a (smaller) ball of mud too, and in fact your constructor falls right into this trap:

def __init__(self, cards): self.cards = cards.split(' ') self.vals_and_suits() self.calculate_rank()

Looking at this function, there's no obvious dependency between vals_and_suits and calculate_rank. If I were to swap them -- merge gone bad? -- then the code would still "look right":

def __init__(self, cards): self.cards = cards.split(' ') self.calculate_rank() self.vals_and_suits()

But it wouldn't work, would it?

It's perfectly fine to require helper functions, but when you need multiple functions to operate in a specific order, I heartily recommend making the dependencies explicit -- using return values & arguments.

def __init__(self, cards): self.cards = cards.split(' ') self.vals, self.num_vals = Hand._compute_values(self.cards) self.suits = Hand._compute_suits(self.cards) self... = Hand._compute_rank(self.cards, self.vals, self.suits)

I took the liberty of splitting vals & suits, because there's really no reason to bundle them together.

Now it's clear which operations can be swapped -- setting values & suits -- and which cannot -- can't compute the rank without first having the values & suits.

Constructor, constructor

The last piece of advice is that... not everything need be pre-computed.

Yes, you may want to know the rank of a Hand... but you also may not.

This has several implications:

  1. Not every answer has to be represented as a field on a class.
  2. Not every answer has to be computed in the constructor.

I would recommend, in general, aiming for minimality. Aiming for the essence.

What's the essence of a Hand in your case? A Hand is a set of 5 distinct cards.

In details:

  • A Hand is composed of Cards.
  • There's 5 Cards in a Hand.
  • No two Cards in a Hand can be the same.
  • The order of the Cards do not matter.

You only need the constructor to establish those invariants, ie verify there's no error in the input.

Then you can have a method which computes the rank of the Hand, but this method should NOT store the results of the calculation as fields; it should simply return them.

\$\endgroup\$
4
  • \$\begingroup\$ Upvoted! If computing the rank of a hand is a costly operation that the OP wants to avoid recomputing more than once, it could be worth having a private field __rank initially set to None in the constructor, a private method __compute_and_set_rank() and a public method get_rank(), with the code of get_rank() being if self.__rank is None: self.__compute_and_set_rank(); return self.__rank. But whether or not to store the rank as a private field remains an implementation detail, not a public field. \$\endgroup\$ Commented Mar 3 at 9:58
  • 1
    \$\begingroup\$ @Stef: I hesitated about touching on this. Other techniques include using a memoizing decorator (such as wiki.python.org/moin/PythonDecoratorLibrary#Memoize), which has the advantage of keeping the class "clean" by offloading the memoized result. The problem with memoization (manual or delegated) is that it suffers from the cache invalidation problems. If it becomes possible to modify the class, then any modification needs to invalidated some/all cached (memoized) results, and it gets messy quickly. Sometimes, simpler is better: the user can avoid computing the rank N times. \$\endgroup\$ Commented Mar 3 at 10:51
  • \$\begingroup\$ Small nit: in python, single leading underscore is used to indicate "private implementation detail, don't touch me", and double leading underscore is only for very special cases to avoid accidental name collisions in subclasses (name mangling). Don't use __something for just a private method, one underscore is enough. \$\endgroup\$ Commented Mar 5 at 23:00
  • \$\begingroup\$ @STerliakov: Thanks for the nit, I'll edit my answer :) \$\endgroup\$ Commented Mar 6 at 8:06
8
\$\begingroup\$

Naming

Most of the functions and variables have descriptive, meaningful names.

However, the function vals_and_suits is a little vague. It looks like it sets values, so perhaps sets_vals_and_suits is more descriptive.

Similarly for repeated_cards. That function could use a docstring to describe what it is doing.

The cards variable has 2 different purposes, which is a little confusing:

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

It might be clearer to use different variable names:

def __init__(self, card_string): self.cards = card_string.split(' ')

Comments

Many of the comments are helpful. However, several can be deleted since they merely repeat what the code shows. For example:

# Suits of the cards self.suits = [card[1] for card in self.cards]

It is obvious what the suits variable is for because you chose a meaningful name for the variable. That comment can be deleted.

Here is another example of a comment which can be deleted:

# Royal Flush otherwise else: self.rank = 'Royal Flush'

This will make the code less cluttered.

Simpler

When I see an if/else with a != in it like this :

if self.rank != 'Straight': else:

I prefer to invert the polarity to make it easier to understand:

if self.rank == 'Straight': else:

This means you need to swap the lines in each branch as well.

The same for:

if self.num_vals[0] != 8: else:

Parentheses are conventionally omitted for an if condition:

if (len(set(self.suits)) == 1):

This is simpler:

if len(set(self.suits)) == 1:

In the repeated_cards function, there seems to be a misplaced print. Perhaps that was left behind while you were debugging.

The default for the split function is to split on space:

 self.cards = cards.split(' ')

This is a little simpler, and it allows for multiple spaces ('5C 2H'):

 self.cards = cards.split()
\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much for all of the insights! How about the class code structure (functions inside methods and such), is there a more correct way to do it or is my approach fine? \$\endgroup\$ Commented Feb 28 at 12:45
  • \$\begingroup\$ @BraisRomero: You're welcome. The "helper" functions seem fine; they are good for encapsulation. \$\endgroup\$ Commented Feb 28 at 12:50
2
\$\begingroup\$

Your test is not a test; it doesn't assert anything. Consider instead something like

def test() -> None: hand1 = Hand('5C 2H 2C AD AC') assert hand1.rank == 'Two Pairs' assert hand1.rank_val == 3 assert hand1.cards == ['5C', '2H', '2C', 'AD', 'AC'] assert hand1.num_vals == [0, 0, 3, 12, 12] assert hand1.max_val == 12 assert hand1.rest_vals == [0, 0, 3]

You say

this was the perfect opportunity to try to work with classes

Great. Your class initialisation scheme is problematic because vals_and_suits sets new members outside of the constructor. There are several ways around this, including having vals_and_suits return a tuple for the constructor to unpack and set on self.

I think rather than your lists-of-properties (e.g. self.vals) you should have an array of Cards. You can also validate based on a regular expression. This could look like

import re import typing ALL_VALUES = '23456789TJQKA' CARD_PAT = re.compile( rf'''(?x) # Verbose (?P<name> # Top-level named group (?P<value> # Named group [{re.escape(ALL_VALUES)}] # Character class for value ) (?P<suit> # Named group [SDCH] # Character class for suit ) ) (?: # Non-capturing group for suffix \s|$ # The match must precede a space or the end of the string ) ''') class Card(typing.NamedTuple): name: str suit: typing.Literal['S', 'D', 'C', 'H'] value: int @classmethod def cards_from_string(cls, s: str) -> list[typing.Self]: # Could check here for unmatched content based on index information; that is not done here return [ cls( name=match['name'], suit=match['suit'], value=ALL_VALUES.index(match['value']), ) for match in CARD_PAT.finditer(s) ] def __repr__(self) -> str: return self.name def demo() -> None: print(Card.cards_from_string('5C 2H 2C AD AC')) # [5C, 2H, 2C, AD, AC] if __name__ == '__main__': demo()
\$\endgroup\$
3
  • \$\begingroup\$ I like the idea of modelling a Card as a class, but find from_string parsing a list of cards a tad counter-intuitive to be honest -- from the name I'd expect it to parse one card. May I suggest list_from_string? \$\endgroup\$ Commented Mar 1 at 17:18
  • \$\begingroup\$ @MatthieuM. list is already obvious by the hint; but a better name is still possible; perhaps cards_from_string. \$\endgroup\$ Commented Mar 1 at 17:43
  • 1
    \$\begingroup\$ When I see the hint, yes. At the call site, however, I don't necessarily see the hint, so it can be confusing. cards_from_string would work well, too. \$\endgroup\$ Commented Mar 1 at 17:49
2
\$\begingroup\$

According to one quick Google search, a 'fresh' deck of cards is "commonly" sorted from Ace (low) to King.

 ... # Straight Flush if smallest card is not a 10 if self.num_vals[0] != 8: ... # Value for the wheel self.max_val = 3 ...

There are a few instances of unexplained & mind-bending magic numbers in this code because it has been written to consider Ace high (with special handling - wheel - to consider Ace as low.)

Base-zero indexing makes the code even more opaque.

Suggest you find a way to map cards 'Ace thru 10, Jack, Queen, King' to their obvious ordinal values (1-13). (Note: this mapping directly corresponds to playing card graphics in the Unicode character chart.)

I haven't implemented a test to check if the hand is impossible (repeated or non existing cards), but I may do it in the future. The next step is to create a process that compares two hands and gives the winner.

While admirable and normally recommended, validation of duplicate or badly formed data, here, is inappropriate. If you are going to be dealing with multiple hands (pardon the pun), responsibility for delivering 'pre-approved' cards (from one deck) must be done at that 'higher' level. After all, two-or-more players cannot all claim to have been dealt 4 Aces.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.