Beginner:Terminal Single Player Poker Game - Code Review

Hi,
I'm trying to learn rust and tried to put together a small poker based game that runs on ubuntu terminal. The compiler has been very helpful so I got the code to run. It can detect flush, straight, full house, two of a kind, three of a kind, four of a kind and two pairs. I would appreciate it if anyone could help me improve it and also help me out with best practices.

Code available at Sole Poker Git Link
Edited code changing cl to clear. Caused error in other system.

Dockerfile along with executable available at https://github.com/bejohn/pokerdocker.git
After creating docker image, run /home/rust/game/solo_poker

Thanks

1 Like

I'm sorry, I still can't play the game. Maybe my unusual system settings are causing the problem.

Anyway, here's some general suggestions:

  • Consider creating a README so that visitors can take a tour of the game before deciding to download it.

  • Use cargo fmt to format your code — right now there are some places where the unusual formatting hurts readability a bit.

  • Run cargo clippy to find some common mistakes — I received 1 error and 4 warnings by using it, including a redundant return, a redundant use, reference cloning (you probably want to clone the underlying data instead), and two occurrences of format!("{}", "...") (which is just "...".to_string()).

  • pub types generally implement Debug.

  • You can derive many useful traits for Card, House, and Value: Clone, Copy, Debug, Eq, Hash, PartialEq.

  • Display should fix the format:

    fn format_value(&self) -> String {
        match self.val {
            Value::Num(x) => {
                if x == 10 {
                    format!("{} ", x)
                } else {
                    format!("{}  ", x)
                }
            }
            Value::Ace => format!("{}  ", "A"),
            Value::King => format!("{}  ", "K"),
            Value::Queen => format!("{}  ", "Q"),
            Value::Jack => format!("{}  ", "J"),
        }
    }
    

    Instead, implementations of Display use methods like Formatter::pad to respect the user's format settings, such as {:3}. Similar goes for format_card. Also, House and Value should probably implement Display too.

    See this link to the playground for a demonstration of what I mean.

  • Personally, I would define Deck as a tuple struct:

    #[derive(Clone)]
    pub struct Deck(Vec<Card>);
    
  • Deck::new should be renamed to Deck::random, since it shuffles the deck. You can also use Itertools::iproduct to simplify the nested loop: (untested)

    // shuffling omitted, so "canonical"
    pub fn canonical() -> Deck {
        use itertools::iproduct;
    
        static houses: &[House; 4] = [House::Diamonds, House::Hearts, House::Clubs, House::Spades];
        static special_values: &[Value; 4] = [Value::Ace, Value::King, Value::Queen, Value::Jack];
    
        let cards = iproduct!(
            houses.iter().copied(),
            special_values
                .iter()
                .copied()
                .chain((2..=10).map(Value::Num)),
        )
        .map(|house, value| Card { house, value });
    
        Deck(cards.collect())
    }
    

I haven't read all the code, and I'll come back with more if I have time :slight_smile:

3 Likes

Thank you so much @L.F. There's a lot to learn already. Going to start with Display after the README since I have a lot of that. Been especially wondering how to format the splash screen parts better.

Sorry you haven't been able to play it. There is a dockerfile along with the executable at https://github.com/bejohn/pokerdocker.git, if you run docker build in the folder with the Dockerfile and the file solo_poker, you would be able to create the docker image. The game will be at /home/rust/game/solo_poker in the docker instance.

Sorry @benoy, apparently an emulated Ubuntu terminal doesn't suffice, which I am using.

Anyway, I'll read the rest of the code and give suggestions on the weekend if I have time!

thanks @L.F will implement your suggestions over the weekend. learning about some of them. So will be slow.

Here's more suggestions :slight_smile:

  • Try not to declare variables beforehand — it makes the logic a bit intimidating:

    let mut d = decking::Deck::new();
    let mut hand: Vec<decking::Card> = Vec::new();
    let mut dealer: Vec<decking::Card> = Vec::new();
    let mut input_txt = String::new();
    let mut total_bet = 0_i32;
    #[allow(unused_assignments)]
    let mut current_bet = 0_i32;
    let mut bet_limit = total_chips;
    

    For example, current_bet can be declared inside the loop, where it is used.

  • You can use a match here instead of if else if else if else:

    let show_dealer = |n: i32| {
        if n == 1 {
            hide_four();
            println!("{}", dealer[0]);
        } else if n == 2 {
            hide_three();
            println!("{} {}", dealer[0], dealer[1]);
        } else if n == 5 {
            println!(
                "{} {} {} {} {}",
                dealer[0], dealer[1], dealer[2], dealer[3], dealer[4]
            );
        } else {
            show_none();
        }
    };
    
  • In show_hand, for cd in hand.iter() can be written more idiomatically as for cd in &hand; see the clippy lint explicit_iter_loop.

  • The clone here is not a necessary:

    let temp = &(input_txt.trim()).clone();
    

    A simple &str will do and avoid an allocation:

    let temp = input_txt.trim();
    
  • It might be better for Card to provide the evaluation functionality, rather than letting the user always do an explicit match:

    match dc.val {
        decking::Value::Num(x) => *v_hash.entry(x as u32).or_insert(0) += 1,
        decking::Value::Ace => *v_hash.entry(1_u32).or_insert(0) += 1,
        decking::Value::Jack => *v_hash.entry(11_u32).or_insert(0) += 1,
        decking::Value::Queen => *v_hash.entry(12_u32).or_insert(0) += 1,
        decking::Value::King => *v_hash.entry(13_u32).or_insert(0) += 1,
    }
    
  • The game function is quite long. Consider breaking it into pieces.

  • This:

    let mut val: Vec<u32> = Vec::new();
    for (x, _) in val_hash {
        val.push(x);
    }
    

    can be simplified to

    let val: Vec<_> = map.keys().collect();
    
  • Try not to overflow the screen with extraordinarily long string literals. One way is to use line concatenation:

    "\
    123\
    456\
    789\
    "
    

    is equivalent to

    "123456789"
    

Overall, the code seems good :slight_smile:

1 Like

Wow. Thanks @L.F. Just took up two points quickly cause I had to see how that worked. Well they worked.

  • changed let temp = &(input_txt.trim()).clone() and subsequently changed temp == &"quit" to temp == "quit".

  • Also changed hand.iter() to &hand. Did not know that. Thanks especially for the link.

*This point has me a little confused "It might be better for Card to provide the evaluation functionality". The hashmap v_hash is would be storing the count of each type of card in both dealer and player's hand. The struct Card itself would not be aware of that since it knows only the individual card and struct Deck would contain all the cards, it won't know the values from the dealer or player's hand. Combining this point and your point about breaking the game function, would having a separate struct for the user and dealer hand be more ideal and idiomatic?

Sorry I didn't articulate that point clearly. I meant something like this:

// impl Value
pub fn rank(self) -> u32 {
    match self {
        Card::Num(n) => n,
        Card::Ace => 1,
        Card::Jack => 11,
        // ...
    }
}

so that the user can directly use, say,

*v_hash.entry(dc.val.rank()).or_insert(0) += 1;

and not have to match the value on the spot. That was what I meant by "evaluation", which is admittedly a highly ambiguous word.

Yeah, we can have a dedicated struct for game state, including things like the dealer, the hand, the current and total bet, etc., to help organize the game logic in one place.

1 Like

That is a more elegant solution. Thanks

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.