Beating C++ in a CVu Code Critique

#1

The ACCU CVu magazine https://accu.org/index.php/journal runs a Code Critique in each issue. The idea is to take a bit of code which is usually broken in some way, and to critique it for errors and for failings to be idiomatic. The code is usually C++ for historical reasons. I like to critique by saying C++ is the wrong language, use X, where in the past X has usually been Groovy, Python, or D, or more than one of them. I’d like to introduce Rust into the mix. The current critique question is:

"I’m trying to write a very simple dice game where the computer simulates
two players each throwing dice. The higher score wins and after a
(selectable) number of turns the player who’s won most times wins the game.
(I’m going to make the game cleverer once it’s working.) But the games
always seem to be drawn and I can’t see why. Here is what the program
produces:

dice_game
Let’s play dice
How many turns? 10
Drawn!
How many turns? 8
Drawn!
How many turns? ^D

What’s going wrong - and how might you help the programmer find the problem?
As usual, there may be other suggestions you might make of some other
possible things to (re-)consider about their program."

followed by some C++ code which not only doesn’t work, it shows C++ is so far behind the state of the art that D, Go, and Rust win hands down. I have come up with a Rust solution which has clearly been informed by the C++ solution and probably shouldn’t be. So I am looking for some informed opinion on the code as idiomatic Rust code.

extern crate rand;

use std::io;
use std::io::Write;

use rand::distributions::Range;
use rand::distributions::Sample;

fn play(turns: u8, generator: &mut FnMut()->u8) {
    let player1 = (0..turns).into_iter().map(|_|{ generator() }).collect::<Vec<u8>>();
    let player2 = (0..turns).into_iter().map(|_|{ generator() });
    let total: i32 = player1.iter().zip(player2).map(
        |p| if p.0 > &p.1 { 1 } else if p.0 < &p.1 { -1 } else { 0 }
    ).sum();
    if total == 0 {
        println!("Drawn!");
    }else {
        println!("Player {} wins.", if total > 0 { 1 } else { 2 })
    }
}

fn main() {
    println!("Let's play dice");
    let mut between = Range::new(1u8, 7u8); // TODO make this not mut.
    let mut rng = rand::thread_rng();
    loop {
        print!("How many turns? ");
        io::stdout().flush().unwrap();
        let mut buffer = String::new();
        match io::stdin().read_line(&mut buffer) {
            Ok(_) =>match buffer.trim().parse::<u8>() {
                Ok(turns) => play(turns, &mut ||{ between.sample(&mut rng) } ),
                    Err(_) => break
                },
            Err(_) => break
        }
    }
}
#2

Some random comments that spontaneously come to mind.

First of all, in play(), the definitions of player1 and player2 are quite repetitive. A way to simplify this would be to create a single vector of 2*turns dice rolls, then iterate over pairs of dice rolls using the chunks() slice method.

If you allow yourself to use itertools, you can also simplify things further and reduce memory usage to O(1) by rewriting the beginning of play() into…

let player_rolls = itertools::repeat_call(generator)
                             .tuples::<(_, _)>()
                             .take(turns);

The fact that play() mixes together I/O and “application logic” may not be of everyone’s taste either. A cleaner approach would be for play() to return an enum modeling all possible game outcomes (player N wins + draw), and extracting all the result reporting I/O logic into main().

The error handling in the main loop is somewhat inelegant. I think nicer error handling code could be produced by handling unexpected termination (e.g. stdout flush failure) via expect(), which is basically a self-documenting variant of unwrap(), and centralizing the “expected” termination scenarios into a single match statement using either Result methods (map, map_err, etc) or a dedicated loop iteration functions that uses early exit.

If you were going through the futile exercise of optimizing this toy code for performance, you could as well reduce dynamic memory allocations by moving the definition of “buffer” outside of the main loop and clearing it on every loop iteration instead.

EDIT: As a matter of taste, I would also rename “between” into “dice”, since dice.sample(rng) has imo a slightly clearer intent than between.sample(rng).

#3

Here is the nicest version which I managed to come up with so far. Notice the new FIXMEs.

extern crate itertools;
extern crate rand;

use std::io::{self, Write};
use itertools::Itertools;
use rand::distributions::{Range, Sample};

enum GameOutcome { PlayerWon(u8), Draw }

// FIXME: Should "turns" really be an u8?
fn play(turns: u8, dice_roll: &mut FnMut() -> u8) -> GameOutcome {
    let player_roll_pairs = itertools::repeat_call(dice_roll)
                                      .tuples::<(_, _)>()
                                      .take(turns as usize);
    let total: i32 = player_roll_pairs.map(
        |p| if p.0 > p.1 { 1 } else if p.0 < p.1 { -1 } else { 0 }
    ).sum();
    if total != 0 {
        GameOutcome::PlayerWon(if total > 0 { 1 } else { 2 })
    } else {
        GameOutcome::Draw
    }
}

fn main() {
    println!("Let's play dice");
    let mut dice = Range::new(1u8, 7u8); // TODO make this not mut.
    let mut rng = rand::thread_rng();
    let mut buffer = String::new();
    loop {
        print!("How many turns? ");
        io::stdout().flush().expect("Failed to flush standard output");
        buffer.clear();
        // FIXME: Should examine the I/O error kind more carefully here
        //        since not every read_line error is expected!
        if let Err(_) = io::stdin().read_line(&mut buffer) { break; }
        let turns = buffer.trim().parse::<u8>()
                          .expect("Invalid number of turns");
        match play(turns, &mut || { dice.sample(&mut rng) } ) {
            GameOutcome::Draw => println!("Drawn!"),
            GameOutcome::PlayerWon(p) => println!("Player {} wins.", p),
        }
    }
}
#4

Another way to write a comparison is this:

    let total: i32 = player_roll_pairs.map(|p| match p.0.cmp(&p.1) {
        Ordering::Less => -1,
        Ordering::Equal => 0,
        Ordering::Greater => 1,
    }).sum();

More verbose, but also a bit more direct.

#5

Thanks to @HadrienG and @matklad for some very useful feedback and input.

#6 
let total: i32 = player_roll_pairs.map(
        |p| (p.0 - p.1).signum()
    ).sum();
#7

Also:

match player_roll_pairs.map(|p| (p.0 as i32 - p.1 as i32).signum()).sum::<i32>().cmp(&0) {
    Ordering::Less => GameOutcome::PlayerWon(1),
    Ordering::Equal => GameOutcome::Draw,
    Ordering::Greater => GameOutcome::PlayerWon(2),
}

(Not even compile-tested)
(EDIT: @matklad is right: .cmp() is better)

#8

I tried to use shortcuts in case of errors to make it lighter (too bad RFC 1937 isn’t there yet):

extern crate itertools;
extern crate rand;

use std::io::{self, Write};
use std::cmp::Ordering;
use itertools::Itertools;
use rand::distributions::{Range, Sample};

enum GameOutcome {
    PlayerWon(u8),
    Draw,
}

// FIXME: Should "turns" really be an u8?
fn play(turns: u8, dice_roll: &mut FnMut() -> u8) -> GameOutcome {
    let player_roll_pairs = itertools::repeat_call(dice_roll).tuples::<(_, _)>().take(
        turns as usize,
    );
    match player_roll_pairs
        .map(|p| ((p.0 as i32 - p.1 as i32).signum()))
        .sum::<i32>()
        .cmp(&0) {
        Ordering::Less => GameOutcome::PlayerWon(1),
        Ordering::Equal => GameOutcome::Draw,
        Ordering::Greater => GameOutcome::PlayerWon(2),
    }
}

fn party() -> io::Result<()> {
    println!("Let's play dice");
    let mut dice = Range::new(1u8, 7u8); // TODO make this not mut.
    let mut rng = rand::thread_rng();
    let mut buffer = String::new();
    loop {
        print!("How many turns? ");
        io::stdout().flush()?;
        buffer.clear();
        io::stdin().read_line(&mut buffer)?;
        let turns = match buffer.trim().parse::<u8>() {
            Ok(n) => n,
            Err(_) => {
                eprintln!("Invalid number of turns");
                return Ok(());
            }
        };
        match play(turns, &mut || dice.sample(&mut rng)) {
            GameOutcome::Draw => println!("Drawn!"),
            GameOutcome::PlayerWon(p) => println!("Player {} wins.", p),
        }
    }
}

fn main() {
    if let Err(err) = party() {
        eprintln!("IO Error: {}", err.description());
    };
}

But I have a problem: I can’t differentiate between possible ParseIntErrors. If .parse() returns a ParseIntError { kind: Empty } I’d like to end gracefully, otherwise complain about bad input, but I can’t find a way to do that.
Does anyone know ?