Simulate Monty Hall

Question

Problem statement: A contestant is presented with three doors. Behind one door is a valuable prize, behind the other two are gag gifts. After the contestant chooses a door, the host opens up one of the other two doors (never revealing the prize, of course). The contestant is then given the opportunity to switch to the other unopened door. Should the contestant do so? Write a program that takes an integer command-line argument number_of_trials, plays the game number_of_trials times using each of the two strategies (switch or do not switch) and print the chance of success for each strategy.

This is one of my self-imposed challenges in Rust to become better at it. The problem was taken from Sedgewick Exercise 1.3.42.

Here is my code:

use clap::Parser;
use rand::Rng;
use std::ops::RangeFrom;
use std::process;

const VALID_TRIAL_NUMBERS: RangeFrom<u32> = 1..;

#[derive(Debug, Parser)]
struct Arguments {
    #[arg(index = 1)]
    number_of_trials: u32,
}

fn main() {
    let arguments = Arguments::parse();

    if !VALID_TRIAL_NUMBERS.contains(&arguments.number_of_trials) {
        eprintln!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start,
        );
        process::exit(1);
    }

    let mut number_of_wins_with_change: u32 = 0;
    let mut number_of_wins_without_change: u32 = 0;

    for _ in 0..arguments.number_of_trials {
        if simulate_with_change() {
            number_of_wins_with_change += 1;
        }

        if simulate_without_change() {
            number_of_wins_without_change += 1;
        }
    }

    let win_probability_with_change: f64 =
        (number_of_wins_with_change as f64) / (arguments.number_of_trials as f64);
    let win_probability_without_change: f64 =
        (number_of_wins_without_change as f64) / (arguments.number_of_trials as f64);

    println!(
        "Winning probabilities after {} trials:",
        arguments.number_of_trials,
    );
    println!("With change: {}", win_probability_with_change);
    println!("Without change: {}", win_probability_without_change);
}

fn simulate_with_change() -> bool {
    let mut rng = rand::thread_rng();
    let door_with_prize: u8 = rng.gen_range(1..=3);
    let contestant_choice: u8 = rng.gen_range(1..=3);

    if door_with_prize == contestant_choice {
        false
    } else {
        true
    }
}

fn simulate_without_change() -> bool {
    let mut rng = rand::thread_rng();
    let door_with_prize: u8 = rng.gen_range(1..=3);
    let contestant_choice: u8 = rng.gen_range(1..=3);

    if door_with_prize == contestant_choice {
        true
    } else {
        false
    }
}

Is there any way that I can improve my code?

Answer 1

Use linters

Use e.g. cargo clippy to lint your code.

Don't repeat yourself

simulate_without_change() and simulate_without_change() are nearly identical. They return the opposite boolean value. You can use this fact to make the one function return the inverse of the other:

fn simulate_with_change() -> bool {
    !simulate_without_change()
}

fn simulate_without_change() -> bool {
    let mut rng = rand::thread_rng();
    let door_with_prize: u8 = rng.gen_range(1..=3);
    let contestant_choice: u8 = rng.gen_range(1..=3);

    if door_with_prize == contestant_choice {
        true
    } else {
        false
    }
}

Make use of boolean expressions

    if door_with_prize == contestant_choice {
        true
    } else {
        false
    }

is redundant. You can rewrite it to:

    door_with_prize == contestant_choice

Use variable interpolation

You can directly put variables in format strings:

    println!("With change: {win_probability_with_change}");
    println!("Without change: {win_probability_without_change}");

Cast safely

Prefer from() and try_from() over casting via as. In your current code casting u32 to f64 will work, but it may get lossy, if you change the types later on (e.g. to u64 or f32 respectively). Using safe casting using from() will result in a compiler error if you'd do that instead of silently producing loss.

    let win_probability_with_change: f64 =
        f64::from(number_of_wins_with_change) / f64::from(arguments.number_of_trials);
    let win_probability_without_change: f64 =
        f64::from(number_of_wins_without_change) / f64::from(arguments.number_of_trials);

Outsource further

Currently your main() function actually runs the simulation. Consider outsourcing that to an actual simulation function:

fn simulate_both(number_of_trials: u32) -> Result<(f64, f64), String> {
    if !VALID_TRIAL_NUMBERS.contains(&number_of_trials) {
        return Err(format!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start
        ));
    }

    let mut number_of_wins_with_change: u32 = 0;
    let mut number_of_wins_without_change: u32 = 0;

    for _ in 0..number_of_trials {
        if simulate_with_change() {
            number_of_wins_with_change += 1;
        }

        if simulate_without_change() {
            number_of_wins_without_change += 1;
        }
    }

    let win_probability_with_change: f64 =
        f64::from(number_of_wins_with_change) / f64::from(number_of_trials);
    let win_probability_without_change: f64 =
        f64::from(number_of_wins_without_change) / f64::from(number_of_trials);

    Ok((win_probability_with_change, win_probability_without_change))
}

Putting it all together

We can put all of the above together and use simulate() in main() by using the let else syntax as introduced in Rust 1.65:

use clap::Parser;
use rand::Rng;
use std::ops::RangeFrom;
use std::process;

const VALID_TRIAL_NUMBERS: RangeFrom<u32> = 1..;

#[derive(Debug, Parser)]
struct Arguments {
    #[arg(index = 1)]
    number_of_trials: u32,
}

fn main() {
    let arguments = Arguments::parse();

    let Ok((win_probability_with_change, win_probability_without_change)) =
        simulate_both(arguments.number_of_trials) else
    {
        eprintln!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start,
        );
        process::exit(1);
    };

    println!(
        "Winning probabilities after {} trials:",
        arguments.number_of_trials,
    );
    println!("With change: {win_probability_with_change}");
    println!("Without change: {win_probability_without_change}");
}

fn simulate_both(number_of_trials: u32) -> Result<(f64, f64), String> {
    if !VALID_TRIAL_NUMBERS.contains(&number_of_trials) {
        return Err(format!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start
        ));
    }

    let mut number_of_wins_with_change: u32 = 0;
    let mut number_of_wins_without_change: u32 = 0;

    for _ in 0..number_of_trials {
        if simulate_with_change() {
            number_of_wins_with_change += 1;
        }

        if simulate_without_change() {
            number_of_wins_without_change += 1;
        }
    }

    let win_probability_with_change: f64 =
        f64::from(number_of_wins_with_change) / f64::from(number_of_trials);
    let win_probability_without_change: f64 =
        f64::from(number_of_wins_without_change) / f64::from(number_of_trials);

    Ok((win_probability_with_change, win_probability_without_change))
}

fn simulate_with_change() -> bool {
    !simulate_without_change()
}

fn simulate_without_change() -> bool {
    let mut rng = rand::thread_rng();
    let door_with_prize: u8 = rng.gen_range(1..=3);
    let contestant_choice: u8 = rng.gen_range(1..=3);

    door_with_prize == contestant_choice
}

More optimization

You can outsource and use a single ThreadRng instance and pass it to the functions, so it does not need to be re-initialized on every function call:

use clap::Parser;
use rand::rngs::ThreadRng;
use rand::Rng;
use std::ops::RangeFrom;
use std::process;

const VALID_TRIAL_NUMBERS: RangeFrom<u32> = 1..;

#[derive(Debug, Parser)]
struct Arguments {
    #[arg(index = 1)]
    number_of_trials: u32,
}

fn main() {
    let arguments = Arguments::parse();
    let mut rng = rand::thread_rng();

    let Ok((win_probability_with_change, win_probability_without_change)) =
        simulate_both(arguments.number_of_trials, &mut rng) else
    {
        eprintln!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start,
        );
        process::exit(1);
    };

    println!(
        "Winning probabilities after {} trials:",
        arguments.number_of_trials,
    );
    println!("With change: {win_probability_with_change}");
    println!("Without change: {win_probability_without_change}");
}

fn simulate_both(number_of_trials: u32, rng: &mut ThreadRng) -> Result<(f64, f64), String> {
    if !VALID_TRIAL_NUMBERS.contains(&number_of_trials) {
        return Err(format!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start
        ));
    }

    let mut number_of_wins_with_change: u32 = 0;
    let mut number_of_wins_without_change: u32 = 0;

    for _ in 0..number_of_trials {
        if simulate_with_change(rng) {
            number_of_wins_with_change += 1;
        }

        if simulate_without_change(rng) {
            number_of_wins_without_change += 1;
        }
    }

    let win_probability_with_change: f64 =
        f64::from(number_of_wins_with_change) / f64::from(number_of_trials);
    let win_probability_without_change: f64 =
        f64::from(number_of_wins_without_change) / f64::from(number_of_trials);

    Ok((win_probability_with_change, win_probability_without_change))
}

fn simulate_with_change(rng: &mut ThreadRng) -> bool {
    !simulate_without_change(rng)
}

fn simulate_without_change(rng: &mut ThreadRng) -> bool {
    rng.gen_range(1..=3) == rng.gen_range(1..=3)
}

Note that I also removed the redundant temporary variables in simulate_without_change().

Consider using filter and map functions

... instead of a for-loop.
This surely is opinionated, but I like this more:

fn simulate_both(number_of_trials: u32, rng: &mut ThreadRng) -> Result<(f64, f64), String> {
    if !VALID_TRIAL_NUMBERS.contains(&number_of_trials) {
        return Err(format!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start
        ));
    }

    let Ok(number_of_wins_with_change) = u32::try_from(
        (0..number_of_trials)
            .map(|_| simulate_with_change(rng))
            .filter(|b| *b)
            .count(),
    ) else {
        return Err("Amount of wins with change is out of bounds.".to_string());
    };

    let Ok(number_of_wins_without_change) = u32::try_from(
        (0..number_of_trials)
            .map(|_| simulate_without_change(rng))
            .filter(|b| *b)
            .count(),
    ) else {
        return Err("Amount of wins without change is out of bounds.".to_string());
    };

    Ok((
        f64::from(number_of_wins_with_change) / f64::from(number_of_trials),
        f64::from(number_of_wins_without_change) / f64::from(number_of_trials),
    ))
}

It's also possible to reduce the code to one sole simulation function by inlining the now trivial test:

fn simulate_both(number_of_trials: u32, rng: &mut ThreadRng) -> Result<(f64, f64), String> {
    if !VALID_TRIAL_NUMBERS.contains(&number_of_trials) {
        return Err(format!(
            "Number of trials must be at least {}.",
            VALID_TRIAL_NUMBERS.start
        ));
    }

    let Ok(number_of_wins_with_change) = u32::try_from(
        (0..number_of_trials)
            .map(|_| rng.gen_range(1..=3) != rng.gen_range(1..=3))
            .filter(|b| *b)
            .count(),
    ) else {
        return Err("Amount of wins with change is out of bounds.".to_string());
    };

    let Ok(number_of_wins_without_change) = u32::try_from(
        (0..number_of_trials)
            .map(|_| rng.gen_range(1..=3) == rng.gen_range(1..=3))
            .filter(|b| *b)
            .count(),
    ) else {
        return Err("Amount of wins without change is out of bounds.".to_string());
    };

    Ok((
        f64::from(number_of_wins_with_change) / f64::from(number_of_trials),
        f64::from(number_of_wins_without_change) / f64::from(number_of_trials),
    ))
}