4
\$\begingroup\$

While having experience with other languages I only recently started learning Rust and I was hoping to get some feedback on my little beginner project, especially regarding style (stuff like too many functions, should rather be refactored into struct with methods, etc.) and safety (whether there are situations that could crash the program).

I already ran Clippy but intentionally ignored two warnings because I preferred my own solution.

One thing I'm already unsure of is the way I solved draws and wins as I find my current solution not really nice.

use std::{fmt, io};

fn main() {
    let mut field = [FieldState::Blank; 9];

    print_field(&field);
    loop {
        println!("Player 1 (X) take your turn");
        take_turn(FieldState::Player1, &mut field);
        print_field(&field);
        print_win(check_win(&field));
        print_draw(&field);

        println!("Player 2 (O) take your turn");
        take_turn(FieldState::Player2, &mut field);
        print_field(&field);
        print_win(check_win(&field));
        print_draw(&field);
    }
}

#[derive(Copy, Clone, PartialEq)]
enum FieldState {
    Blank,
    Player1,
    Player2,
}

impl fmt::Display for FieldState {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            FieldState::Blank => write!(f, " "),
            FieldState::Player1 => write!(f, "X"),
            FieldState::Player2 => write!(f, "O"),
        }
    }
}

fn print_field(field: &[FieldState; 9]) {
    for i in 0..2 {
        println!("{}|{}|{}", field[i * 3], field[1 + i * 3], field[2 + i * 3]);
        println!("-----");
    }
    println!("{}|{}|{}", field[2 * 3], field[1 + 2 * 3], field[2 + 2 * 3]);
}

fn take_turn(p: FieldState, f: &mut [FieldState; 9]) {
    loop {
        let choice = (read_user_input() - 1) as usize;

        if f[choice] != FieldState::Blank {
            println!("There's already a marking there! Pick another place.");
            continue;
        } else {
            f[choice] = p;
            return;
        }
    }
}

fn read_user_input() -> u8 {
    loop {
        let mut ans = String::new();

        io::stdin()
            .read_line(&mut ans)
            .expect("Failed to read input");

        let ans: i16 = match ans.trim().parse() {
            Ok(n) => n,
            Err(_) => {
                println!("Please enter a number from 1 to 9");
                continue;
            }
        };

        if ans < 1 || ans > 9 {
            println!("Please enter a number from 1 to 9");
            continue;
        } else {
            return ans.try_into().unwrap();
        }
    }
}

fn check_win(f: &[FieldState; 9]) -> FieldState {
    // Columns
    for x in 0..3 {
        if f[x] != FieldState::Blank && f[x] == f[3 + x] && f[x] == f[6 + x] {
            return f[x];
        }
    }

    // Rows
    for y in 0..3 {
        if f[3 * y] != FieldState::Blank
            && f[3 * y] == f[1 + 3 * y]
            && f[3 * y] == f[2 + 3 * y]
        {
            return f[3 * y];
        }
    }

    // Diagonals
    if f[0] != FieldState::Blank && f[0] == f[4] && f[0] == f[8] {
        return f[0];
    }

    if f[2] != FieldState::Blank && f[2] == f[4] && f[0] == f[6] {
        return f[2];
    }

    FieldState::Blank
}

fn print_draw(f: &[FieldState; 9]) {
    for i in 0..9 {
        if f[i] == FieldState::Blank {
            return;
        }
    }

    println!("It's a draw!");
    std::process::exit(0);
}

fn print_win(winner: FieldState) {
    if winner == FieldState::Player1 {
        println!("Player 1 (X) won!");
        std::process::exit(0);
    } else if winner == FieldState::Player2 {
        println!("Player 2 (O) won!");
        std::process::exit(0);
    }
}

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Bug

check_win contains a copy-paste error:

if f[2] != FieldState::Blank && f[2] == f[4] && f[0] == f[6] {
    return f[2]; //                             ^^^^ should be f[2]
}

Simplify check_win

The logic for any group of three fields (column, row or diagonal) is the same: check if the first field isn't empty and if it equals the other two fields. This can be extracted into a helper closure:

fn check_win(f: &[FieldState; 9]) -> FieldState {
    let check_group = |i1, i2, i3| f[i1] != FieldState::Blank && f[i1] == f[i2] && f[i1] == f[i3];
    
    // Columns
    for x in 0..3 {
        if check_group(x, 3 + x, 6 + x) {
            return f[x];
        }
    }

    // Rows
    for y in 0..3 {
        if check_group(3 * y, 1 + 3 * y, 2 + 3 * y) {
           return f[3 * y];
        }
    }

    // Diagonals
    if check_group(0, 4, 8) {
        return f[0];
    }

    if check_group(2, 4, 6) {
        return f[2];
    }

    FieldState::Blank
}

With this, such a copy-paste error is also less likely to happen.

Integer type confusion

In read_user_input, you parse the input as an i16, then cast it to a u8 and then cast it to a usize at the call site. Just use usize directly and avoid casting. Further, the code to print the error message is repeated two times. Using a match guard makes this easier:

fn read_user_input() -> usize {
    loop {
        let mut ans = String::new();

        io::stdin()
            .read_line(&mut ans)
            .expect("Failed to read input");

        match ans.trim().parse::<usize>() {
            Ok(n) if (1..10).contains(&n) => return n,
            _ => println!("Please enter a number from 1 to 9"),
        }
    }
}

Shorter main

With an extra variable current_player we can avoid duplicating the code for player 1 and 2:

fn main() {
    let mut field = [FieldState::Blank; 9];
    let mut current_player = FieldState::Player1;

    print_field(&field);
    loop {
        if current_player == FieldState::Player1 {
            println!("Player 1 (X) take your turn");
        } else {
            println!("Player 2 (O) take your turn");
        }
        
        take_turn(current_player, &mut field);
        print_field(&field);
        print_win(check_win(&field));
        print_draw(&field);
        current_player = if current_player == FieldState::Player1 {
            FieldState::Player2
        } else {
            FieldState::Player1
        };
    }
}

This will come in handy for the next step.

Usage of std::process::exit

You use this function in print_draw and print_win. The problem with it is that it makes your code extremely unflexible. Right now, if you wanted to do anything else after the end of the game, it wouldn't be possible. Let's change that by adding another enum to describe the game state:

enum GameState {
    Win,
    Draw,
    Running,
}

Then, we can extract the logic from print_draw and update check_win to instead return the current GameState:

fn get_game_state(f: &[FieldState; 9]) -> GameState {
    let check_group = |i1, i2, i3| f[i1] != FieldState::Blank && f[i1] == f[i2] && f[i1] == f[i3];

    // Columns
    for x in 0..3 {
        if check_group(x, 3 + x, 6 + x) {
            return GameState::Win;
        }
    }

    // Rows
    for y in 0..3 {
        if check_group(3 * y, 1 + 3 * y, 2 + 3 * y) {
            return GameState::Win;
        }
    }

    // Diagonals
    if check_group(0, 4, 8) || check_group(2, 4, 6) {
        return GameState::Win;
    }

    // Draw checking
    if f.contains(&FieldState::Blank) {
        return GameState::Running;
    }

    GameState::Draw
}

The code for draw checking was simplified a bit too. Now, the updated main looks like this:

fn main() {
    let mut field = [FieldState::Blank; 9];
    let mut current_player = FieldState::Player1;

    print_field(&field);
    loop {
        if current_player == FieldState::Player1 {
            println!("Player 1 (X) take your turn");
        } else {
            println!("Player 2 (O) take your turn");
        }

        take_turn(current_player, &mut field);
        print_field(&field);
        match get_game_state(&field) {
            GameState::Win => {
                print_winner(current_player);
                break;
            }
            GameState::Draw => {
                println!("It's a draw!");
                break;
            }
            GameState::Running => (),
        };

        current_player = if current_player == FieldState::Player1 {
            FieldState::Player2
        } else {
            FieldState::Player1
        };
    }
}

print_draw was inlined since its logic was extracted and only a single println! remained. print_win remains, but the process::exit calls are removed.

Final code:

use std::{fmt, io};

fn main() {
    let mut field = [FieldState::Blank; 9];
    let mut current_player = FieldState::Player1;

    print_field(&field);
    loop {
        if current_player == FieldState::Player1 {
            println!("Player 1 (X) take your turn");
        } else {
            println!("Player 2 (O) take your turn");
        }

        take_turn(current_player, &mut field);
        print_field(&field);
        match get_game_state(&field) {
            GameState::Win => {
                print_winner(current_player);
                break;
            }
            GameState::Draw => {
                println!("It's a draw!");
                break;
            }
            GameState::Running => (),
        };

        current_player = if current_player == FieldState::Player1 {
            FieldState::Player2
        } else {
            FieldState::Player1
        };
    }
}

#[derive(Copy, Clone, PartialEq)]
enum FieldState {
    Blank,
    Player1,
    Player2,
}

impl fmt::Display for FieldState {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            FieldState::Blank => write!(f, " "),
            FieldState::Player1 => write!(f, "X"),
            FieldState::Player2 => write!(f, "O"),
        }
    }
}

enum GameState {
    Win,
    Draw,
    Running,
}

fn print_field(field: &[FieldState; 9]) {
    for i in 0..2 {
        println!("{}|{}|{}", field[i * 3], field[1 + i * 3], field[2 + i * 3]);
        println!("-----");
    }
    println!("{}|{}|{}", field[2 * 3], field[1 + 2 * 3], field[2 + 2 * 3]);
}

fn take_turn(p: FieldState, f: &mut [FieldState; 9]) {
    loop {
        let choice = read_user_input() - 1;

        if f[choice] != FieldState::Blank {
            println!("There's already a marking there! Pick another place.");
            continue;
        } else {
            f[choice] = p;
            return;
        }
    }
}

fn read_user_input() -> usize {
    loop {
        let mut ans = String::new();

        io::stdin()
            .read_line(&mut ans)
            .expect("Failed to read input");

        match ans.trim().parse::<usize>() {
            Ok(n) if (1..10).contains(&n) => return n,
            _ => println!("Please enter a number from 1 to 9"),
        }
    }
}

fn get_game_state(f: &[FieldState; 9]) -> GameState {
    let check_group = |i1, i2, i3| f[i1] != FieldState::Blank && f[i1] == f[i2] && f[i1] == f[i3];

    // Columns
    for x in 0..3 {
        if check_group(x, 3 + x, 6 + x) {
            return GameState::Win;
        }
    }

    // Rows
    for y in 0..3 {
        if check_group(3 * y, 1 + 3 * y, 2 + 3 * y) {
            return GameState::Win;
        }
    }

    // Diagonals
    if check_group(0, 4, 8) || check_group(2, 4, 6) {
        return GameState::Win;
    }

    // Draw checking
    if f.contains(&FieldState::Blank) {
        return GameState::Running;
    }

    GameState::Draw
}

fn print_winner(winner: FieldState) {
    if winner == FieldState::Player1 {
        println!("Player 1 (X) won!");
    } else {
        println!("Player 2 (O) won!");
    }
}

Further ideas:

Different purposes of FieldState

FieldState is used both to store the state of the field and to store the current player. But since the current player can never be FieldState::Blank, consider removing that variant from the enum and renaming it to enum Player. The fields can then be stored as an Option<Player>.

Game struct

I agree with your thought of using a struct for this. It improves reusability and makes the code easier to use. Here's a possibility how to define one:

struct Game {
    fields: [Option<Player>; 9],
    current_player: Player,
}

We should also add a constructor:

impl Game {
    fn new() -> Self {
        Self {
            fields: [None; 9],
            current_player: Player::Player1,
        }
    }
}

You could then turn most functions into methods of this struct.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.