1
\$\begingroup\$

Is there a meaningful way to simplify and refactor this function? It sorts the flop by rank and assigns a new suit according to the sorted rank order. Higher ranked cards will be assigned spades, middle ranked cards diamonds, and lowest ranked cards clubs. If two or three cards of the input flop have the same suit, these cards in the converted flop should also be suited.

def convert_to_similar_flop(flop):

   RANKS = {'2':2, '3':3, '4':4, '5':5, '6':6, '7':7, '8':8, '9':9, '10':10, 'J':11, 'Q':12, 'K':13, 'A':14}
   SUITS = ['spade', 'diamond', 'club']

   sorted_flop = sorted(flop, key=lambda x: RANKS[x[0]], reverse=True)

   flop_suits = [s for r,s in sorted_flop]
   suit_counts = {suit:flop_suits.count(suit) for suit in flop_suits}

   # Assign new suits to the sorted flop
   new_flop = [(rank, 'spade') if i < suit_counts[flop_suits[0]] 
               else (rank, 'diamond') if i < (suit_counts[flop_suits[0]] + suit_counts.get(flop_suits[1], 0)) 
               else (rank, 'club') 
               for i, (rank, suit) in enumerate(sorted_flop)]

   return new_flop
flop_rainbow = [('K', 'heart'), ('10', 'diamond'), ('2', 'club')]
print(convert_to_similar_flop(flop_rainbow))  # [('K', 'spade'), ('10', 'diamond'), ('2', 'club')]

flop_twotone = [('K', 'heart'), ('10', 'heart'), ('2', 'club')]
print(convert_to_similar_flop(flop_twotone))  # [('K', 'spade'), ('10', 'spade'), ('2', 'diamond')]

flop_monotone = [('K', 'heart'), ('10', 'heart'), ('2', 'heart')]
print(convert_to_similar_flop(flop_monotone))  # [('K', 'spade'), ('10', 'spade'), ('2', 'spade')]
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code is for per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Commented Aug 5, 2023 at 11:00
  • \$\begingroup\$ BTW, flop is a particular term from Texas Hold-em and other poker games. Could it be that what you have here is a hand? It's really not clear what this code is for - if it's for a particular game format, please mention that. \$\endgroup\$ Commented Aug 5, 2023 at 11:01

2 Answers 2

5
\$\begingroup\$

It is not at all clear what this code is supposed to do, so it's hard to say during code review whether the code does it "correctly".

If there is some external specification or set of rules that this code tries to conform to, perhaps this one, then the source code should devote at least one line of comment in order to cite the reference.

The contract presented to the Gentle Reader is: convert_to_similar_flop. Ok, conversion is clear, and we might take a leap of faith that a flop is as defined on the wikipedia page. But the notion of similarity is not at all clear. It suggests that perhaps there's just one right answer, a "most similar" return value, and as I read the code I confess I wouldn't know how to decide on the correctness of any given value that was returned. The jargony phrase "... should also be suited" similarly deserves a sentence or two of source code comments and/or an URL citation.

def convert_to_similar_flop(flop):

Consider adopting optional type hints. Alternatively, write a """docstring""" that explains the input arg is a list of card tuples. Spell out that a card is a (rank, suit) pair. The saving grace in this codebase was the presence of example valid inputs like [('K', 'heart'), ..., for which I thank you.

If the list must have length of exactly three, assert that, or raise a ValueError for wrong length.


namedtuple

The [0] / [1] tuple subscripts are cryptic and inconvenient.

Better to define a namedtuple:

from collections import namedtuple

Card = namedtuple('Card', ['rank', 'suit'])

Now you can write expressions like:

sorted(flop, key=lambda card: RANKS[card.rank], ...)

   suit_counts = {suit: flop_suits.count(suit) for suit in flop_suits}

minor nit: Consider breaking out a helper function for this. Just so you have an opportunity to """document""" what goes in and out. And perhaps even add type hints.

Unit tests could then conveniently invoke the helper, further illustrating what it does.


   # Assign new suits to the sorted flop
   new_flop = [(rank, 'spade') if i < suit_counts[flop_suits[0]] 
               else (rank, 'diamond') if i < (suit_counts[flop_suits[0]] + suit_counts.get(flop_suits[1], 0)) 
               else (rank, 'club') 
               for i, (rank, suit) in enumerate(sorted_flop)]

Your heart was in the right place when you wrote the # comment, and I thank you for it. But please delete it, and instead use it as the name of a helper:

def assign_new_suit(i, card, suit_counts):

It might be convenient to turn this into a class, in order to implicitly refer to self.suit_counts.


flop_rainbow = [('K', 'heart'), ('10', 'diamond'), ('2', 'club')]
print(convert_to_similar_flop(flop_rainbow))  # [('K', 'spade'), ('10', 'diamond'), ('2', 'club')]

Thank you for including these statements, they are very helpful.

But they're not unit tests, they're not automated tests of any kind. That is, they are not self-evaluating, they don't know the right answer. They need a human to read the comment and decide if the answer is right.

Prefer to put these within a TestCase that you run with $ python -m unittest *.py


This code appears to achieve its design goals.

As written, it can only be maintained by Subject Matter Experts familiar with the relevant (unnamed!) card game(s), so I would not be willing to accept maintenance tasks on this codebase.

\$\endgroup\$
3
\$\begingroup\$

Embrace simple data objects. Rather than building a program around raw tuples, define a meaningful object to represent a Card. That will convert code revolving around opaque tuple indexes with code using declarative attribute names. It also provides a natural home for functionality that logically belongs to a card.

from dataclasses import dataclass

@dataclass
class Card:
    rank: str
    suit: str

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

    @property
    def rank_val(self):
       return self.RANK_VALS[self.rank]

A simpler suit remapping approach. Your current strategy seems convoluted: count how many times each suit appears and then use those counts, plus a fair bit of conditional logic, to convert the suit. If you want to remap information, the most natural mechanism is to use a dict mapping current values to new values. We can create such a dict fairly easily if we can get a unique list of the flop's suits, based on their order of first appearance within the sorted flop.

from operator import attrgetter

def convert_to_similar_flop(flop):
    # Sort cards based on rank.
    sorted_flop = sorted(flop, key = attrgetter('rank_val'), reverse = True)

    # Get unique suits, preserving sorted order.
    sorted_suits = [card.suit for card in sorted_flop]
    uniq_suits = list(dict.fromkeys(sorted_suits))

    # Dict mapping current suits to their replacements.
    suit_lookup = dict(zip(uniq_suits, ['spade', 'diamond', 'club']))

    # Return new flop with suit-remapping.
    return [
        Card(card.rank, suit_lookup[card.suit])
        for card in sorted_flop
    ]
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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