4
\$\begingroup\$

I found this interesting challenge in Exercism. This is my approach to comparing poker hands in Rust. I've strived for code clarity. I wish I had figured out how to only implement Ord or PartialOrd, in the end I had to implement both for all structs.

It would be nice also if I had a way to avoid using the rank associated function on the enum HandType. My original approach was to establish constant values for each enum variant (StraightFlush = 9) but then I had to spread tons of explicit casts through the code and that looked dirty (hand.hadn_type as u8).

use std::cmp;
/// Given a list of poker hands, return a list of those hands which win.
///
/// Note the type signature: this function should return _the same_ reference to
/// the winning hand(s) as were passed in, not reconstructed strings which happen to be equal.
use std::cmp::Ordering;
use std::collections::HashMap;

#[derive(Debug)]
struct Card {
    suit: char,
    numeric_value: u8,
}

impl Card {
    const ACE: u8 = 14;
    const KING: u8 = 13;
    const QUEEN: u8 = 12;
    const JACK: u8 = 11;

    fn new(str_card: &str) -> Result<Self, String> {
        let size = str_card.len();
        if !(2..=3).contains(&size) {
            return Err("Invalid card length".into());
        }

        let value = match size {
            2 => str_card.chars().next().unwrap(),
            3 => {
                let first_2: &str = &str_card[0..2];
                if first_2 == "10" {
                    'T'
                } else {
                    return Err(format!("Invalid card: {}", str_card));
                }
            }
            _ => panic!("Invalid card length"),
        };

        let suit = str_card.chars().nth(size - 1).unwrap();
        if suit != 'D' && suit != 'C' && suit != 'S' && suit != 'H' {
            return Err(format!("Invalid suit: {}", suit));
        }
        let numeric_value = match value {
            '2'..='9' => value.to_digit(10).unwrap() as u8,
            'T' => 10,
            'J' => Card::JACK,
            'Q' => Card::QUEEN,
            'K' => Card::KING,
            'A' => Card::ACE,
            _ => return Err("Invalid value".into()),
        };

        Ok(Self {
            suit,
            numeric_value,
        })
    }
}

#[derive(Eq, PartialEq, Debug)]
struct HighCardData {
    cards: Vec<u8>,
}

impl HighCardData {
    fn new(cards: Vec<u8>) -> Self {
        Self { cards }
    }
}

impl PartialOrd for HighCardData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for HighCardData {
    fn cmp(&self, other: &Self) -> Ordering {
        for i in (0..self.cards.len()).rev() {
            let self_value = self.cards[i];
            let other_value = other.cards[i];

            if self_value != other_value {
                return self_value.cmp(&other_value);
            }
        }
        Ordering::Equal
    }
}

#[derive(Eq, PartialEq, Debug)]
struct OnePairData {
    pair_value: u8,
    remaining_cards: Vec<u8>,
}

impl OnePairData {
    fn new(pair_value: u8, remaining_cards: Vec<u8>) -> Self {
        Self {
            pair_value,
            remaining_cards,
        }
    }
}

impl PartialOrd for OnePairData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for OnePairData {
    fn cmp(&self, other: &Self) -> Ordering {
        let cmp_pair_value = self.pair_value.cmp(&other.pair_value);
        if cmp_pair_value != Ordering::Equal {
            return cmp_pair_value;
        }
        HighCardData::new(self.remaining_cards.clone())
            .cmp(&HighCardData::new(other.remaining_cards.clone()))
    }
}

#[derive(Eq, PartialEq, Debug)]
struct TwoPairsData {
    high_pair_value: u8,
    low_pair_value: u8,
    remaining_card: u8,
}

impl TwoPairsData {
    fn new(high_pair_value: u8, low_pair_value: u8, remaining_card: u8) -> Self {
        Self {
            high_pair_value,
            low_pair_value,
            remaining_card,
        }
    }
}

impl PartialOrd for TwoPairsData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for TwoPairsData {
    fn cmp(&self, other: &Self) -> Ordering {
        let cmp_high_pair_value = self.high_pair_value.cmp(&other.high_pair_value);
        if cmp_high_pair_value != Ordering::Equal {
            return cmp_high_pair_value;
        }
        let cmp_low_pair_value = self.low_pair_value.cmp(&other.low_pair_value);
        if cmp_low_pair_value != Ordering::Equal {
            return cmp_low_pair_value;
        }
        self.remaining_card.cmp(&other.remaining_card)
    }
}

#[derive(Eq, PartialEq, Debug)]
struct ThreeOfAKindData {
    three_of_a_kind_value: u8,
    remaining_cards: Vec<u8>,
}

impl ThreeOfAKindData {
    fn new(three_of_a_kind_value: u8, remaining_cards: Vec<u8>) -> Self {
        Self {
            three_of_a_kind_value,
            remaining_cards,
        }
    }
}

impl PartialOrd for ThreeOfAKindData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for ThreeOfAKindData {
    fn cmp(&self, other: &Self) -> Ordering {
        let cmp_value = self.three_of_a_kind_value.cmp(&other.three_of_a_kind_value);
        if cmp_value != Ordering::Equal {
            return cmp_value;
        }
        HighCardData::new(self.remaining_cards.clone())
            .cmp(&HighCardData::new(other.remaining_cards.clone()))
    }
}

#[derive(Eq, PartialEq, Debug)]
struct StraightData {
    cards: Vec<u8>,
}

impl StraightData {
    fn new(cards: Vec<u8>) -> Self {
        Self { cards }
    }
}

impl PartialOrd for StraightData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for StraightData {
    fn cmp(&self, other: &Self) -> Ordering {
        let sv = &self.cards;
        let ov = &other.cards;
        let self_vec: Vec<u8>;
        if sv[0] == 2 && sv[sv.len() - 1] == Card::ACE {
            self_vec = vec![1, sv[0], sv[1], sv[2], sv[3]];
        } else {
            self_vec = sv.clone();
        }
        let other_vec: Vec<u8>;
        if ov[0] == 2 && ov[ov.len() - 1] == Card::ACE {
            other_vec = vec![1, ov[0], ov[1], ov[2], ov[3]];
        } else {
            other_vec = ov.clone();
        }

        HighCardData::new(self_vec).cmp(&HighCardData::new(other_vec))
    }
}

#[derive(Eq, PartialEq, Debug)]
struct FullHouseData {
    three_of_a_kind_value: u8,
    pair_value: u8,
}

impl FullHouseData {
    fn new(three_of_a_kind_value: u8, pair_value: u8) -> Self {
        Self {
            three_of_a_kind_value,
            pair_value,
        }
    }
}

impl PartialOrd for FullHouseData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for FullHouseData {
    fn cmp(&self, other: &Self) -> Ordering {
        let cmp_three = self.three_of_a_kind_value.cmp(&other.three_of_a_kind_value);
        if cmp_three != Ordering::Equal {
            return cmp_three;
        }
        self.pair_value.cmp(&other.pair_value)
    }
}

#[derive(Eq, PartialEq, Debug)]
struct FourOfAKindData {
    four_of_a_kind_value: u8,
    remaining_card: u8,
}

impl FourOfAKindData {
    fn new(four_of_a_kind_value: u8, remaining_card: u8) -> Self {
        Self {
            four_of_a_kind_value,
            remaining_card,
        }
    }
}

impl PartialOrd for FourOfAKindData {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(&other))
    }
}

impl Ord for FourOfAKindData {
    fn cmp(&self, other: &Self) -> Ordering {
        let cmp = self.four_of_a_kind_value.cmp(&other.four_of_a_kind_value);
        if cmp != Ordering::Equal {
            return cmp;
        }
        self.remaining_card.cmp(&other.remaining_card)
    }
}

#[derive(Eq, PartialEq, Ord, PartialOrd, Debug)]
enum HandType {
    StraightFlush(HighCardData),
    FourOfAKind(FourOfAKindData),
    FullHouse(FullHouseData),
    Flush(HighCardData),
    Straight(StraightData),
    ThreeOfAKind(ThreeOfAKindData),
    TwoPairs(TwoPairsData),
    OnePair(OnePairData),
    HighCard(HighCardData),
}

impl HandType {
    fn rank(&self) -> u8 {
        match self {
            HandType::StraightFlush(_) => 9,
            HandType::FourOfAKind(_) => 8,
            HandType::FullHouse(_) => 7,
            HandType::Flush(_) => 6,
            HandType::Straight(_) => 5,
            HandType::ThreeOfAKind(_) => 4,
            HandType::TwoPairs(_) => 3,
            HandType::OnePair(_) => 2,
            HandType::HighCard(_) => 1,
        }
    }
}

#[derive(Eq, Debug)]
struct PokerHand<'a> {
    str_hand: &'a str,
    hand_type: HandType,
}

impl<'a> PokerHand<'a> {
    fn new_from_values(str_hand: &'a str, hand_type: HandType) -> Self {
        Self {
            str_hand,
            hand_type,
        }
    }

    fn new(str_hand: &'a str) -> Result<Self, String> {
        let mut cards: Vec<Card> = Vec::new();
        let split = str_hand.split(' ');
        for s in split {
            let possible_new_card = Card::new(s);
            let new_card = match possible_new_card {
                Ok(card) => card,
                Err(_) => return Err("Cannot create card".into()),
            };
            cards.push(new_card);
        }

        cards.sort_by(|a, b| a.numeric_value.cmp(&b.numeric_value));

        let first_card = &cards[0];
        let mut previous_suit = first_card.suit;
        let mut previous_value = 0;
        let mut suit_counter = 0;
        let mut straight_counter = 1;

        let mut kind_count_map: HashMap<u8, usize> = HashMap::new();

        let mut starts_at_two = false;

        for card in &cards {
            if card.suit == previous_suit {
                suit_counter += 1;
            } else {
                suit_counter = 0;
                previous_suit = card.suit;
            }
            if previous_value == 0 {
                if card.numeric_value == 2 {
                    starts_at_two = true;
                } else {
                    previous_value = card.numeric_value;
                }
            } else if card.numeric_value == (previous_value + 1) {
                straight_counter += 1;
            }
            if card.numeric_value != previous_value {
                previous_value = card.numeric_value;
            }
            *kind_count_map.entry(card.numeric_value).or_insert(0) += 1;
        }
        let is_flush = suit_counter == 5;
        let is_straight = PokerHand::is_straight(
            straight_counter,
            starts_at_two,
            cards[cards.len() - 1].numeric_value,
        );

        let sorted_numeric_values: Vec<u8> = cards.iter().map(|c| c.numeric_value).collect();

        if is_flush {
            if is_straight {
                return Ok(PokerHand::new_from_values(
                    str_hand,
                    HandType::StraightFlush(HighCardData::new(sorted_numeric_values)),
                ));
            } else {
                return Ok(PokerHand::new_from_values(
                    str_hand,
                    HandType::Flush(HighCardData::new(sorted_numeric_values)),
                ));
            }
        }

        if is_straight {
            return Ok(PokerHand::new_from_values(
                str_hand,
                HandType::Straight(StraightData::new(sorted_numeric_values)),
            ));
        }

        let mut has_three = false;
        let mut pair_count = 0;

        let mut three_of_a_kind_value: u8 = 0;

        let mut pairs: Vec<u8> = Vec::new();

        for (key, value) in kind_count_map {
            if value == 4 {
                let the_other_card: Vec<&u8> = sorted_numeric_values
                    .iter()
                    .filter(|v| *v != &key)
                    .collect();
                return Ok(PokerHand::new_from_values(
                    str_hand,
                    HandType::FourOfAKind(FourOfAKindData::new(key, *the_other_card[0])),
                ));
            } else if value == 3 {
                has_three = true;
                three_of_a_kind_value = key;
            } else if value == 2 {
                pairs.push(key);
                pair_count += 1;
            }
        }

        if has_three {
            if pair_count == 1 {
                return Ok(PokerHand::new_from_values(
                    str_hand,
                    HandType::FullHouse(FullHouseData::new(three_of_a_kind_value, pairs[0])),
                ));
            } else {
                let remaining_cards: Vec<u8> = sorted_numeric_values
                    .into_iter()
                    .filter(|v| *v != three_of_a_kind_value)
                    .collect();
                return Ok(PokerHand::new_from_values(
                    str_hand,
                    HandType::ThreeOfAKind(ThreeOfAKindData::new(
                        three_of_a_kind_value,
                        remaining_cards,
                    )),
                ));
            }
        }

        if pair_count == 2 {
            let highest_pair = cmp::max(pairs[0], pairs[1]);
            let lowest_pair = cmp::min(pairs[0], pairs[1]);
            let the_other_card: Vec<&u8> = sorted_numeric_values
                .iter()
                .filter(|v| *v != &highest_pair && *v != &lowest_pair)
                .collect();
            return Ok(PokerHand::new_from_values(
                str_hand,
                HandType::TwoPairs(TwoPairsData::new(
                    highest_pair,
                    lowest_pair,
                    *the_other_card[0],
                )),
            ));
        }

        if pair_count == 1 {
            let pair_value = pairs[0];
            let remaining_cards: Vec<u8> = sorted_numeric_values
                .into_iter()
                .filter(|v| *v != pair_value)
                .collect();
            return Ok(PokerHand::new_from_values(
                str_hand,
                HandType::OnePair(OnePairData::new(pair_value, remaining_cards)),
            ));
        }

        Ok(PokerHand::new_from_values(
            str_hand,
            HandType::HighCard(HighCardData::new(sorted_numeric_values)),
        ))
    }

    fn is_straight(
        straight_counter: usize,
        starts_at_two: bool,
        last_card_numeric_value: u8,
    ) -> bool {
        straight_counter == 5
            || PokerHand::is_straight_starting_with_ace(
                straight_counter,
                starts_at_two,
                last_card_numeric_value,
            )
    }

    fn is_straight_starting_with_ace(
        straight_counter: usize,
        starts_at_two: bool,
        last_card_numeric_value: u8,
    ) -> bool {
        straight_counter == 4 && starts_at_two && last_card_numeric_value == Card::ACE
    }
}

impl<'a> Ord for PokerHand<'a> {
    fn cmp(&self, other: &Self) -> Ordering {
        let rank_cmp = self.hand_type.rank().cmp(&other.hand_type.rank());
        if rank_cmp != Ordering::Equal {
            return rank_cmp;
        }
        self.hand_type.cmp(&other.hand_type)
    }
}

impl<'a> PartialOrd for PokerHand<'a> {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

impl<'a> PartialEq for PokerHand<'a> {
    fn eq(&self, other: &Self) -> bool {
        self.hand_type == other.hand_type
    }
}

pub fn winning_hands<'a>(hands: &[&'a str]) -> Option<Vec<&'a str>> {
    let size = hands.len();
    if size == 0 {
        return None;
    }
    if size == 1 {
        return Some(vec![hands[0]]);
    }

    let mut processed_hands: Vec<PokerHand> = Vec::new();

    for hand in hands {
        let processed_hand = match PokerHand::new(hand) {
            Ok(processed_hand) => processed_hand,
            Err(_) => return None,
        };

        processed_hands.push(processed_hand);
    }

    processed_hands.sort();
    processed_hands.reverse();

    let mut result = Vec::new();
    let highest_hand = &processed_hands[0];
    result.push(highest_hand.str_hand);

    for processed_hand in processed_hands.iter().skip(1) {
        if processed_hand.cmp(highest_hand) == Ordering::Equal {
            result.push(processed_hand.str_hand);
        } else {
            break;
        }
    }

    Some(result)
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ If you're using char for suits, you should definitely use '♠', '♥', '♣', and '♦' instead of 'S', 'H', 'C', 'D' ;-) \$\endgroup\$
    – trent
    Mar 29, 2021 at 12:28

1 Answer 1

6
\$\begingroup\$

General observations

  • rustfmt-compliant formatting 👍
  • no warnings 👍
  • #[derive(Debug)] on everything that might be useful to debug-print 👍
  • no tests or main 👎 (I guess this is built in to Exercism?) Some of the remainder of this review might be a little off base because I wasn't able to test the code.

Specific observations

Use newtypes and/or enums for restricted values

struct Card {
    suit: char,
    numeric_value: u8,
}

char and u8 don't really tell you much about those values. What about some enums? enum Suit { Heart, Spade, Diamond, Club } would work great for suit, and for value you could use a C-style enum Rank { Two = 2, Three, ... King, Ace } (C-style enums can be cast to integers with as for the straight check). It might feel a little weird writing out names for the numbers from 2 to 10, but it's not as if they're going to change any time soon, and you only have to do it twice (once in the definition and once for parsing, on which more later).

impl Card {
    const ACE: u8 = 14;
    const KING: u8 = 13;
    const QUEEN: u8 = 12;
    const JACK: u8 = 11;

Making Rank an enum would eliminate these consts as well.

Parsing and initialization

    fn new(str_card: &str) -> Result<Self, String> {

Consider implementing FromStr instead of having a new function that parses a string. I would usually reserve the name new for a trivial constructor, or one that only does validation, not parsing.

Parsing

        let size = str_card.len();
        if !(2..=3).contains(&size) {
            return Err("Invalid card length".into());
        }

This part is a little weird because len() returns byte counts but you go on to parse characters, which is the same thing here but only because all the characters are in the ASCII subset. It's not exactly wrong, but it's fragile; if you wanted to support input like "A♠", it would need a full rewrite. Consider using string methods to match against substrings instead of picking it apart character by character.

Note on error types

String is fine as an error type for a self-contained program. For a library you'd want to have a custom error type and return values like MyError::InvalidLength(n).

        let value = match size {
            2 => str_card.chars().next().unwrap(), // this is the second time you check the size of the string
            3 => {
                let first_2: &str = &str_card[0..2]; // multi-byte inputs will panic here
                if first_2 == "10" {
                    'T'
                } else {
                    return Err(format!("Invalid card: {}", str_card));
                }
            }
            _ => panic!("Invalid card length"), // this is the third time you check the size of the string
        };

        let suit = str_card.chars().nth(size - 1).unwrap(); // this is the FOURTH time you check the size of the string

Here's one example of doing something that kind of solves a problem, but doesn't really solve the whole problem. Because you check the length of the string, but don't validate the contents at this point, you will have to validate the contents in a second pass, during which you will have to essentially validate the length again. Forget validating the size in advance: chunk it up as best you can and parse the chunks individually.

Enums again

        if suit != 'D' && suit != 'C' && suit != 'S' && suit != 'H' {
            return Err(format!("Invalid suit: {}", suit));
        }

Okay, it's validated, but suit is still just a char, so there's no type-level record of this validation being performed. A suit is one of four things; a character could be anything. I think you get the point, let me hit it one more time:

        let numeric_value = match value {
            '2'..='9' => value.to_digit(10).unwrap() as u8,
            'T' => 10,
            'J' => Card::JACK,
            'Q' => Card::QUEEN,
            'K' => Card::KING,
            'A' => Card::ACE,
            _ => return Err("Invalid value".into()),
        };

Same here: you could use a struct Rank(u8); or an enum Rank {...}. If the parsing here is complex you could also implement FromStr for Rank and Suit, and defer to that, instead of implementing it in-line.

Let's skip ahead a bit...

HandType and ordering

#[derive(Eq, PartialEq, Ord, PartialOrd, Debug)]
enum HandType {
    StraightFlush(HighCardData),
    FourOfAKind(FourOfAKindData),
    FullHouse(FullHouseData),
    Flush(HighCardData),
    Straight(StraightData),
    ThreeOfAKind(ThreeOfAKindData),
    TwoPairs(TwoPairsData),
    OnePair(OnePairData),
    HighCard(HighCardData),
}

This enum is unnecessarily complicated. In the first case, it's smart to take advantage of the built-in behavior of #[derive(PartialOrd)], but you've made things unnecessarily hard on yourself by putting these in descending order of rank, so you can't rely on PartialOrd to give the ordering you want: you have to call rank, compare that first, then compare the rest of the contents. If you just list them in increasing order, you can make derive(PartialOrd) do the right thing without any additional work.

Secondly, StraightFlush should hold StraightData instead of HighCardData; as written, you're treating an 5-high straight flush as higher than a king-high straight flush. You wrote the logic for this but you only used it for "normal" straights (I'm probably butchering the vocabulary here, sorry, I don't play poker).

Furthermore, HighCardData's Ord implementation is used, directly or indirectly, in six of the nine variants, which is a strong hint that there's unexploited symmetry in this data structure. Let's look at that...

HighCardData

impl Ord for HighCardData {
    fn cmp(&self, other: &Self) -> Ordering {
        for i in (0..self.cards.len()).rev() {
            let self_value = self.cards[i];
            let other_value = other.cards[i];

            if self_value != other_value {
                return self_value.cmp(&other_value);
            }
        }
        Ordering::Equal
    }
}

Hmm... so the only difference between this and the default Ord behavior for Vec is that it's in reverse order? Why not reverse the cards in the new function, and simply #[derive] it?

OnePairData, etc.

impl Ord for OnePairData {
    fn cmp(&self, other: &Self) -> Ordering {
        
        let cmp_pair_value = self.pair_value.cmp(&other.pair_value);
        // (note `Ordering` has `then` and `then_with` methods that you might use here)
        if cmp_pair_value != Ordering::Equal {
            return cmp_pair_value;
        }
        HighCardData::new(self.remaining_cards.clone())
            .cmp(&HighCardData::new(other.remaining_cards.clone()))
    }
}

This would also work as #[derive]d if the remaining_cards were kept sorted in descending order of value. In fact, this is a common theme among most of the Ord implementations so I won't go through the rest of them one by one. StraightData is a bit special, though:

StraightData

impl Ord for StraightData {
    fn cmp(&self, other: &Self) -> Ordering {
        let sv = &self.cards;
        let ov = &other.cards;
        let self_vec: Vec<u8>;
        if sv[0] == 2 && sv[sv.len() - 1] == Card::ACE {
            self_vec = vec![1, sv[0], sv[1], sv[2], sv[3]];
        } else {
            self_vec = sv.clone();
        }
        let other_vec: Vec<u8>;
        if ov[0] == 2 && ov[ov.len() - 1] == Card::ACE {
            other_vec = vec![1, ov[0], ov[1], ov[2], ov[3]];
        } else {
            other_vec = ov.clone();
        }

        HighCardData::new(self_vec).cmp(&HighCardData::new(other_vec))
    }
}

This one might need a little more thought: if you sort the cards in descending order by rank, treating the ace as a 1, then the #[derive]d (Partial)Ord behavior (when the ace is treated as a 14) will be correct.

[K Q J 10 A]  <- Ace-high straight sorted in descending order (with ace as 1)
[K Q J 10 9]  <- King-high straight sorted in descending order

Using card ordering to your advantage

There's a common theme here: if you sort the cards in the "correct" order when creating HandType, you don't actually need any custom comparison code; it can all be #[derive]d. This suggests to me an alternate implementation:

#[derive(Debug, Eq, Ord, PartialEq, PartialOrd)]
struct Hand {
    kind: HandType,
    cards: Vec<u8>,  // or Vec<Rank>, or just Vec<Card>
}
// If, when creating `Hand`, we detect `kind` and use it to determine how to order 
// `cards`, then we don't need to write any custom (Partial)Ord code.
// Note none of these variants have data -- the cards are in `cards`.
#[derive(Debug, Eq, Ord, PartialEq, PartialOrd)]
enum HandType {
    HighCard,
    OnePair,
    TwoPairs,
    ThreeOfAKind,
    Straight,
    Flush,
    FullHouse,
    FourOfAKind,
    StraightFlush,
}

PokerHand

struct PokerHand<'a> {
    str_hand: &'a str,
    hand_type: HandType,
}

Minor quibble: assuming I'm willingly using a library that has to do with card games, I'm probably going to know that Hand is a hand of cards and not a primate appendage. Prefixing it with Poker doesn't, in my opinion, improve readability. Rust, unlike C, has a namespace system with fine-grained visibility controls, so we don't need to worry about it ever conflicting with some other library type also named Hand.

One thing I would not do here is keep the str_hand as part of this struct. The string is useless once the hand_type has been parsed out of it: you are keeping it around for the sake of winning_hands, but that really shouldn't be any concern of PokerHand. It doesn't come into play during analysis. Just keep track of the parsed data, and let winning_hands remember where it came from (more on this later).

Parsing and validating a PokerHand

impl<'a> PokerHand<'a> {
    fn new(str_hand: &'a str) -> Result<Self, String> {

This is another dense new function that blends the responsibilities of parsing and initialization. I'd make new take a Vec<Card> and do just the hand analysis, and again write a FromStr implementation that parses into a Vec<Card> and then calls new.

        let split = str_hand.split(' ');

Consider using split_whitespace instead.

        cards.sort_by(|a, b| a.numeric_value.cmp(&b.numeric_value));

You have a lot of lines that look something like this. You should usually use sort_unstable_by when the original ordering of equal values is unimportant. It won't likely give a big speed boost (although it might for some datasets), but it is a form of documentation about your assumptions. Also note that you can use u8::cmp(&a.numeric_value, &b.numeric_value) (or Ord::cmp), which I like better because it looks more symmetric, but there's nothing wrong with the way you've written it either.

State machines

        let mut previous_suit = first_card.suit;
        let mut previous_value = 0;
        let mut suit_counter = 0;
        let mut straight_counter = 1;

        let mut kind_count_map: HashMap<u8, usize> = HashMap::new();

        let mut starts_at_two = false;

This is a state machine with six state registers. I personally find that, while it's possible to keep track of five "things" at a time mentally for short periods, the realistic limit should usually be four. Six things is just too many, unless they have some structure to them that allows you to treat them as groups of things ((x, y, z) triplets, or something like that). I apply this general principle not just to state machines, but to function arguments, type parameters, and shopping lists. I would do one of two things here:

  1. (recommended) Break a complex state machine into smaller ones. Basically, do several passes over the cards instead of trying to do it all in a single loop. Since you're looping over a slice that is already in memory, resetting the loop is essentially free. Since there are commonalities between the checks you need to do, you might exploit some of those symmetries to skip parts of later loops. But you don't need to do it all in one shot.
  2. Combine several state registers into a struct, simplifying the outer loop by pushing the complexity down into the struct's methods. Call it something like CardSummary and give it methods like straight(&self) -> Option<Vec<Card>> and full_house(&self) -> Option<[Card; 3], [Card; 2]> to call when you're done. This has the advantage of not requiring iterating over the cards more than once; however, it's probably not necessary in this case.
            if card.numeric_value != previous_value {
                previous_value = card.numeric_value;
            }

This branch is extraneous. Just previous_value = card.numeric_value; should be fine. Making this change reveals that the else a few lines up is also redundant and can be eliminated as well.

        let mut has_three = false;
        let mut pair_count = 0;

        let mut three_of_a_kind_value: u8 = 0;

        let mut pairs: Vec<u8> = Vec::new();

One non-trivial state machine per function, please.

This one can be easily reduced to two state registers, because has_three is just the discriminant of the Option that should contain three_of_a_kind_value, and pair_count is just pairs.len().

All the logic embedded in if has_three, if pair_count == 2, etc. should be wrapped in smaller functions with descriptive names.

winning_hands

pub fn winning_hands<'a>(hands: &[&'a str]) -> Option<Vec<&'a str>> {

I assume Exercism gives you this signature to fill in, so you don't have the ability to control it. However, it's not very good for several reasons. Option<Vec> is weird because Vec can be empty, and in this case an empty output is the perfectly logical response to an empty input, so it should really just be Vec. Except that you're also using None to indicate an error condition, for which you should be using Result instead of Option. Moreover, the interesting part of this is taking a bunch of hands and deciding which one(s) win: not the parsing of hands from strings. So I would first write a function with the signature I want to write, and then write winning_hands to use it internally.

Here's the function signature I'd implement: fn(&[PokerHand]) -> Vec<usize> (the usizes are the indices of the winning hands in the input slice). Because it returns usizes, you can easily implement winning_hands without having to store the input strings in the PokerHands. Bonus challenge: figure out how to accept any iterable of &PokerHands.

    processed_hands.sort();
    processed_hands.reverse();

processed_hands.sort_by(|a, b| Ord::cmp(b, a)) (note the reversal of b and a) or processed_hands.sort_by(|a, b| a.cmp(b).reverse()).

    let highest_hand = &processed_hands[0];
    // ...
    for processed_hand in processed_hands.iter().skip(1) {

Consider using processed_hands.split_first() in situations like this. Not sure if it buys you much in this particular case.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you times a thousand. This was very generous and professional. I have this up on my github and I'll carefully consider your great recommendations and apply them over there. \$\endgroup\$
    – fpezzini
    Mar 30, 2021 at 16:42

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.