rustymines - a mine sweeping game with a shenanigan

Question

I am learning Rust for about a week now. I just finished my first little working project, a mine sweeping game. The game comes with a little extra feature, namely old, broken (rusty) mines, that will not explode if you step on them. I'd like to have feedback on the general code style, since I'm still new to Rust and would also like suggestions, how I could improve the Board.visit_neighbors() which I somehow find pretty ugly.

src/coordinate.rs:

use std::str::FromStr;

#[derive(Debug)]
pub enum CoordinateParseError {
    NotTwoNumbers,
    InvalidXValue,
    InvalidYValue,
}

#[derive(Debug)]
pub struct Coordinate {
    x: usize,
    y: usize,
}

impl Coordinate {
    pub fn new(x: usize, y: usize) -> Self {
        Self { x: x, y: y }
    }

    pub fn x(&self) -> usize {
        self.x
    }

    pub fn y(&self) -> usize {
        self.y
    }

    fn from_str_pair((x, y): (&str, &str)) -> Result<Self, CoordinateParseError> {
        match x.parse::<usize>() {
            Ok(x) => match y.parse::<usize>() {
                Ok(y) => Ok(Coordinate::new(x, y)),
                Err(_) => Err(CoordinateParseError::InvalidYValue),
            },
            Err(_) => Err(CoordinateParseError::InvalidXValue),
        }
    }
}

impl FromStr for Coordinate {
    type Err = CoordinateParseError;

    fn from_str(string: &str) -> Result<Self, Self::Err> {
        match string.split_once(' ') {
            Some(value) => Self::from_str_pair(value),
            None => Err(CoordinateParseError::NotTwoNumbers),
        }
    }
}

src/game.rs:

use std::fmt;

mod args;
use args::parse;
use args::GameArgs;

mod board;
use board::Board;
use board::MoveResult;

#[derive(Debug)]
pub struct Game {
    board: Board,
    over: bool,
}

impl Game {
    pub fn new(width: usize, height: usize, mines: u8, duds: u8) -> Result<Self, &'static str> {
        Ok(Self {
            board: Board::new(width, height, mines, duds)?,
            over: false,
        })
    }

    pub fn from_args(args: &impl GameArgs) -> Result<Self, &'static str> {
        Self::new(args.width(), args.height(), args.mines(), args.duds())
    }

    pub fn parse() -> Result<Self, &'static str> {
        Self::from_args(&parse())
    }

    pub fn visit(&mut self, x: usize, y: usize) {
        match self.board.visit(x, y) {
            MoveResult::AlreadyVisited => println!("You already visited the field at {}x{}.", x, y),
            MoveResult::Continue => println!("{}", self),
            MoveResult::InvalidPosition => {
                println!("The field at {}x{} is not on the board.", x, y)
            }
            MoveResult::Lost => {
                self.over = true;
                println!("You lost the game.")
            }
            MoveResult::Won => {
                self.over = true;
                println!("You won the game.")
            }
        }
    }

    pub fn over(&self) -> bool {
        self.over
    }

    pub fn to_string(&self) -> String {
        self.board.to_string(self.over())
    }
}

impl fmt::Display for Game {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.to_string())
    }
}

src/game/args.rs:

use clap::Parser;

pub trait GameArgs {
    fn width(&self) -> usize;
    fn height(&self) -> usize;
    fn mines(&self) -> u8;
    fn duds(&self) -> u8;
}

#[derive(Parser)]
#[clap(name = "rustymines")]
#[clap(author = "Richard Neumann <[email protected]>")]
#[clap(version = "1.0.0")]
#[clap(about = "A mine sweeping game written in Rust.", long_about = None)]
struct GameArgsParser {
    #[clap(short, long, value_parser, default_value_t = 5)]
    width: usize,

    #[clap(short, long, value_parser, default_value_t = 5)]
    height: usize,

    #[clap(short, long, value_parser, default_value_t = 8)]
    mines: u8,

    #[clap(short, long, value_parser, default_value_t = 0)]
    duds: u8,
}

pub fn parse() -> impl GameArgs {
    GameArgsParser::parse()
}

impl GameArgs for GameArgsParser {
    fn width(&self) -> usize {
        self.width
    }

    fn height(&self) -> usize {
        self.height
    }

    fn mines(&self) -> u8 {
        self.mines
    }

    fn duds(&self) -> u8 {
        self.duds
    }
}

src/game/board.rs:

use itertools::Itertools;
use std::collections::HashMap;

use grid::Grid;
use rand::{seq::IteratorRandom, thread_rng};

mod field;
use field::Field;

#[derive(Debug, PartialEq, Eq)]
pub enum MoveResult {
    AlreadyVisited,
    Continue,
    InvalidPosition,
    Lost,
    Won,
}

#[derive(Debug)]
pub struct Board {
    fields: Grid<Field>,
    mines: u8,
    duds: u8,
    initialized: bool,
}

impl Board {
    pub fn new(width: usize, height: usize, mines: u8, duds: u8) -> Result<Self, &'static str> {
        if width < 1 {
            Err("field too narrow")
        } else if height < 1 {
            Err("field too flat")
        } else if width * height <= mines as usize {
            Err("too many mines for field size")
        } else if duds > mines {
            Err("more duds than mines")
        } else {
            Ok(Self {
                fields: Grid::new(width, height, Field::new),
                mines: mines,
                duds: duds,
                initialized: false,
            })
        }
    }

    pub fn visit(&mut self, x: usize, y: usize) -> MoveResult {
        match self.make_move(x, y) {
            MoveResult::Lost => MoveResult::Lost,
            MoveResult::InvalidPosition => MoveResult::InvalidPosition,
            _ => {
                if self.all_mines_cleared() {
                    MoveResult::Won
                } else {
                    MoveResult::Continue
                }
            }
        }
    }

    pub fn to_string(&self, game_over: bool) -> String {
        self.header()
            + &self
                .fields
                .rows()
                .enumerate()
                .map(|(y, row)| {
                    format!("{:x}|", y)
                        + &row
                            .iter()
                            .enumerate()
                            .map(|(x, field)| {
                                field.to_string(self.neighboring_mines(x, y), game_over)
                            })
                            .join(" ")
                })
                .join("\n")
    }

    fn header(&self) -> String {
        " |".to_string()
            + &(0..self.fields.width())
                .map(|x| format!("{:x}", x))
                .join("|")
            + "\n--"
            + &(0..self.fields.width()).map(|_| '-').join("-")
            + "\n"
    }

    fn neighboring_mines(&self, x: usize, y: usize) -> usize {
        self.fields
            .neighbors(x, y)
            .filter(|(_, _, field)| field.has_mine())
            .count()
    }

    fn make_move(&mut self, x: usize, y: usize) -> MoveResult {
        if !self.initialized {
            self.first_move(x, y)
        } else {
            self.visit_coordinate(x, y)
        }
    }

    fn first_move(&mut self, x: usize, y: usize) -> MoveResult {
        match self.fields.get_mut(x, y) {
            Ok(field) => {
                field.visit();
                self.populate_mines();
                self.populate_duds();
                self.visit_coordinate(x, y);
                self.initialized = true;
                MoveResult::Continue
            }
            Err(_) => MoveResult::InvalidPosition,
        }
    }

    fn populate_mines(&mut self) {
        self.fields
            .iter_mut()
            .filter(|field| !field.visited())
            .choose_multiple(&mut thread_rng(), self.mines as usize)
            .into_iter()
            .for_each(|field| field.set_mine());
    }

    fn populate_duds(&mut self) {
        self.fields
            .iter_mut()
            .filter(|field| field.has_mine())
            .choose_multiple(&mut thread_rng(), self.duds as usize)
            .into_iter()
            .for_each(|field| field.set_dud());
    }

    fn visit_coordinate(&mut self, x: usize, y: usize) -> MoveResult {
        match self.fields.get_mut(x, y) {
            Ok(field) => {
                if self.initialized && field.visited() {
                    MoveResult::AlreadyVisited
                } else {
                    field.visit();

                    if field.has_mine() && !field.is_dud() {
                        MoveResult::Lost
                    } else {
                        if self.neighboring_mines(x, y) == 0 {
                            self.visit_neighbors(x, y);
                        }
                        MoveResult::Continue
                    }
                }
            }
            Err(_) => MoveResult::InvalidPosition,
        }
    }

    fn visit_neighbors(&mut self, x: usize, y: usize) {
        let mut neighbors = HashMap::new();
        neighbors.insert((x, y), ());

        loop {
            let new_neighbors = neighbors
                .iter()
                .filter(|(&(x, y), _)| self.neighboring_mines(x, y) == 0)
                .flat_map(|(&(x, y), _)| {
                    self.fields.neighbors(x, y).filter(|(nx, ny, neighbor)| {
                        !neighbor.has_mine() && !neighbors.contains_key(&(*nx, *ny))
                    })
                })
                .collect_vec();

            if new_neighbors.len() == 0 {
                break;
            }

            for (x, y, _) in new_neighbors {
                neighbors.insert((x, y), ());
            }
        }

        for &(x, y) in neighbors.keys() {
            match self.fields.get_mut(x, y) {
                Ok(field) => field.visit(),
                Err(_) => continue,
            }
        }
    }

    fn all_mines_cleared(&self) -> bool {
        self.fields
            .iter()
            .filter(|field| !field.has_mine())
            .all(|field| field.visited())
    }
}

src/game/board/field.rs:

#[derive(Debug)]
pub struct Field {
    mine: bool,
    dud: bool,
    visited: bool,
}

impl Field {
    pub fn new() -> Self {
        Self {
            mine: false,
            dud: false,
            visited: false,
        }
    }

    pub fn has_mine(&self) -> bool {
        self.mine
    }

    pub fn set_mine(&mut self) {
        self.mine = true;
    }

    pub fn is_dud(&self) -> bool {
        self.dud
    }

    pub fn set_dud(&mut self) {
        self.dud = true;
    }

    pub fn visited(&self) -> bool {
        self.visited
    }

    pub fn visit(&mut self) {
        self.visited = true;
    }

    pub fn to_string(&self, adjacent_mintes: usize, game_over: bool) -> String {
        match (game_over, self.visited, self.mine, self.dud) {
            (_, true, true, true) => "~".to_string(),
            (_, true, true, false) => "*".to_string(),
            (false, true, false, _) | (true, _, false, _) => {
                if adjacent_mintes > 0 {
                    adjacent_mintes.to_string()
                } else {
                    " ".to_string()
                }
            }
            (true, false, true, _) => "o".to_string(),
            _ => "■".to_string(),
        }
    }
}

src/io.rs:

use std::fmt::Debug;
use std::io::Write;
use std::str::FromStr;

pub fn read<T>(prompt: &str) -> Result<T, &'static str>
where
    T: FromStr,
    <T as FromStr>::Err: Debug,
{
    print_prompt(prompt);
    let mut input = String::new();

    match std::io::stdin().read_line(&mut input) {
        Ok(_) => match input.trim().parse::<T>() {
            Ok(value) => Ok(value),
            Err(_) => Err("invalid value"),
        },
        Err(_) => Err("no value read"),
    }
}

pub fn read_repeat<T>(prompt: &str) -> T
where
    T: FromStr,
    <T as FromStr>::Err: Debug,
{
    loop {
        match read::<T>(prompt) {
            Err(msg) => eprintln!("Error: {}", msg),
            Ok(value) => return value,
        }
    }
}

fn print_prompt(prompt: &str) {
    print!("{}", prompt);

    match std::io::stdout().flush() {
        Ok(_) => (),
        Err(_) => (),
    }
}

src/main.rs:

mod coordinate;
use coordinate::Coordinate;

mod game;
use game::Game;

mod io;
use io::read_repeat;

fn main() {
    match Game::parse() {
        Ok(mut game) => run_game(&mut game),
        Err(msg) => eprintln!("Error: {}", msg),
    }
}

fn run_game(game: &mut Game) {
    println!("{}", game);

    while !game.over() {
        let coordinate = read_repeat::<Coordinate>("Enter coordinate: ");
        game.visit(coordinate.x(), coordinate.y());
    }

    println!("{}", game);
}

Cargo.toml:

[package]
name = "rustymines"
version = "1.0.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
itertools = "*"
rand = "*"
grid = { git = "https://github.com/conqp/grid.git", branch = "main" }
clap = { version = "3.2.22", features = ["derive"] }

Answer 1

I don't have time for a complete review right now, so I will just make some observations.

First of all: That's a solid improvement over the already good code you submitted last time!

---

Your main begins with

fn main() {
    match Game::parse() {

which is odd to look at. What is parsed here? Where does the stuff to parse come from? Generally, I would try to show the flow of information rather than hiding it. Also, internally the method calls Self::from_args(&parse()), so nothing is actually parsed, but you use the parsed args.

I would rewrite that as something like:

fn main() {
    let args = Args::parse_args();
    match Game::from(args) {

That would also be a good opportunity to use the standard trait: impl From<Args> for Game.

---

Then you have this one.

while !game.over() {
    let coordinate = read_repeat::<Coordinate>("Enter coordinate: ");
    game.visit(coordinate.x(), coordinate.y());
}

So you read a coordinate, copy its x and y, and then pass the individual values to visit(). But you don't want to visit two different usizes, you want to visit an actual coordinate. So write it like this.

    pub fn visit(&mut self, coord: Coordinate) {

That way you also don't need to specify the type of read_repeat in main.

This change would need to be propagated all the way down to first_move and visit_coordinate and Board::get_mut.

---

read_repeat is strangely worded IMO. Is it necessary for the coder to see that this method will repeat on this level of abstraction? If not, just call it read, which is already taken in your case, so maybe read_user_input will do.

---

match self.board.visit(x, y) {
    MoveResult::AlreadyVisited => println!("You already visited the field at {}x{}.", x, y),

You must have a bug somewhere, because that message is not shown when I visit 0 0 in a new game repeatedly.

---

I like your neighbor_coordinates() method a lot! Nice showcase of functional programming.

---

Usability: The user does not see how the input format looks like and must figure it out themselves. Also it's initially unclear which axis corresponds to which coordinate. You could label the axes.

Answer 2

Regarding Board::visit_neighbors I refactored it to using a HashSet rather than a HashMap, since I don't store any keys anyways. Also I split up the different steps of aggregating the neighbors into two more helper functions:

fn visit_neighbors(&mut self, coordinate: &Coordinate) {
    for coordinate in self.collect_neighbors(coordinate) {
        match self.fields.get_mut(&coordinate) {
            Ok(field) => _ = field.visit(),
            Err(_) => continue,
        }
    }
}

fn collect_neighbors(&self, coordinate: &Coordinate) -> HashSet<Coordinate> {
    let mut neighbors = HashSet::from([*coordinate]);

    loop {
        let new_neighbors = self.new_neighbors(&neighbors);

        if new_neighbors.is_empty() {
            break;
        }

        for coordinate in new_neighbors {
            neighbors.insert(coordinate);
        }
    }

    neighbors
}

fn new_neighbors(&self, neighbors: &HashSet<Coordinate>) -> Vec<Coordinate> {
    neighbors
        .iter()
        .filter(|coordinate| self.neighboring_mines(coordinate) == 0)
        .flat_map(|coordinate| {
            self.fields
                .neighbors(coordinate)
                .filter(|(coordinate, neighbor)| {
                    !neighbor.has_mine()
                        && !neighbor.is_flagged()
                        && !neighbors.contains(coordinate)
                })
        })
        .map(|(coordinate, _)| coordinate)
        .collect_vec()
}