6
\$\begingroup\$

I implemented Tic-Tac-Toe in Rust to learn the language.

I split the implementation into two files: game.rs contains the actual game logic, and main.rs contains the main function and the command-line UI.

I'm interested in any feedback. One particular question I have: I'm interested in trying to implement a GUI interface for the game – have I split the logic correctly between main.rs and game.rs so that game.rs could be reused in a GUI version?

Cargo.toml

[package]
name = "tic-tac-toe"
version = "0.1.0"
authors = ["Stephen Wade <[email protected]>"]
edition = "2018"

[dependencies]
rustyline = "7.1.0"

game.rs

use std::fmt;
use std::ops::{Deref, DerefMut};

#[derive(Clone, Copy, PartialEq)]
pub enum BoardValue {
    Filled(Player),
    Empty,
}

impl fmt::Display for BoardValue {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match self {
            Self::Filled(player) => write!(f, "{}", player),
            Self::Empty => write!(f, " "),
        }
    }
}

impl Default for BoardValue {
    fn default() -> Self {
        Self::Empty
    }
}

impl BoardValue {
    fn player(self) -> Player {
        if let Self::Filled(player) = self {
            player
        } else {
            panic!("not filled")
        }
    }
}

#[derive(Clone, Copy, PartialEq)]
pub enum Player {
    X,
    O,
}

impl fmt::Display for Player {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(
            f,
            "{}",
            match self {
                Self::X => "X",
                Self::O => "O",
            }
        )
    }
}

impl Default for Player {
    fn default() -> Self {
        Player::X
    }
}

type BoardType = [[BoardValue; 3]; 3];

#[derive(Default)]
pub struct Board(BoardType);

impl Deref for Board {
    type Target = BoardType;

    fn deref(&self) -> &BoardType {
        &self.0
    }
}

impl DerefMut for Board {
    fn deref_mut(&mut self) -> &mut BoardType {
        &mut self.0
    }
}

impl fmt::Display for Board {
    #[rustfmt::skip]
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        writeln!(f, "┌───┬───┬───┐")?;
        writeln!(f, "│ {} │ {} │ {} │", self[0][0], self[0][1], self[0][2])?;
        writeln!(f, "├───┼───┼───┤")?;
        writeln!(f, "│ {} │ {} │ {} │", self[1][0], self[1][1], self[1][2])?;
        writeln!(f, "├───┼───┼───┤")?;
        writeln!(f, "│ {} │ {} │ {} │", self[2][0], self[2][1], self[2][2])?;
        write!  (f, "└───┴───┴───┘")
    }
}

impl Board {
    fn get_winnable_slices(&self) -> Vec<[&BoardValue; 3]> {
        vec![
            // Horizontal
            [&self[0][0], &self[0][1], &self[0][2]],
            [&self[1][0], &self[1][1], &self[1][2]],
            [&self[2][0], &self[2][1], &self[2][2]],
            // Vertical
            [&self[0][0], &self[1][0], &self[2][0]],
            [&self[0][1], &self[1][1], &self[2][1]],
            [&self[0][2], &self[1][2], &self[2][2]],
            // Diagonal
            [&self[0][0], &self[1][1], &self[2][2]],
            [&self[0][2], &self[1][1], &self[2][0]],
        ]
    }

    fn get_all_cells(&self) -> Vec<&BoardValue> {
        self.iter().flatten().collect::<Vec<&BoardValue>>()
    }
}

pub struct Game {
    pub board: Board,
    pub current_player: Player,
}

pub enum GameStatus {
    Continue,
    PlayerWins(Player),
    Draw,
}

pub enum PlayError {
    InvalidMove,
}

impl Game {
    pub fn new() -> Self {
        Game {
            board: Board::default(),
            current_player: Player::default(),
        }
    }

    pub fn play(&mut self, row: usize, column: usize) -> Result<GameStatus, PlayError> {
        if matches!(self.board[row][column], BoardValue::Filled(_)) {
            return Err(PlayError::InvalidMove);
        }

        self.board[row][column] = BoardValue::Filled(self.current_player);

        let game_status = self.get_game_status();

        if matches!(game_status, GameStatus::Continue) {
            self.current_player = match self.current_player {
                Player::X => Player::O,
                Player::O => Player::X,
            };
        }

        Ok(game_status)
    }

    fn get_game_status(&self) -> GameStatus {
        for slice in self.board.get_winnable_slices() {
            if matches!(*slice[0], BoardValue::Filled(_))
                && slice[0] == slice[1]
                && slice[1] == slice[2]
            {
                return GameStatus::PlayerWins(slice[0].player());
            }
        }

        if self
            .board
            .get_all_cells()
            .into_iter()
            .all(|cell| matches!(*cell, BoardValue::Filled(_)))
        {
            return GameStatus::Draw;
        }

        GameStatus::Continue
    }
}

main.rs

mod game;

use game::*;

use std::str;
use rustyline::Editor;

fn main() {
    let mut game = Game::new();

    println!("Welcome to tic tac toe!");
    loop {
        println!("{}", game.board);
        println!("It's {}'s turn.", game.current_player);

        let inputs = match get_row_and_column() {
            Ok(inputs) => inputs,
            Err(_) => return,
        };
        match game.play(inputs.0, inputs.1) {
            Ok(GameStatus::Continue) => continue,
            Ok(GameStatus::Draw) => {
                println!("{}", game.board);
                println!("It's a draw.");
                return;
            }
            Ok(GameStatus::PlayerWins(player)) => {
                println!("{}", game.board);
                println!("{} wins!", player);
                return;
            }
            Err(PlayError::InvalidMove) => {
                println!("Invalid move. Please try again.");
            }
        };
    }
}

fn get_row_and_column() -> Result<(usize, usize), ()> {
    let strings = match read_row_and_column_strings() {
        Ok(strings) => strings,
        Err(_) => return Err(()),
    };

    let numbers = match parse_row_and_column_strings(strings) {
        Ok(numbers) => numbers,
        Err(_) => return Err(()),
    };

    Ok((numbers.0 - 1, numbers.1 - 1))
}

fn parse_row_and_column_strings(
    input: (String, String),
) -> Result<(usize, usize), <usize as str::FromStr>::Err> {
    let row: usize = input.0.parse()?;
    let column: usize = input.1.parse()?;

    Ok((row, column))
}

fn read_row_and_column_strings() -> Result<(String, String), rustyline::error::ReadlineError> {
    let mut rl = Editor::<()>::new();

    let valid_inputs: Vec<&str> = vec!["1", "2", "3"];
    let mut row_line: String;
    let mut column_line: String;

    loop {
        row_line = rl.readline("Enter a row (1, 2, 3): ")?;
        if valid_inputs.iter().any(|&s| row_line == s) {
            break;
        };
    }
    loop {
        column_line = rl.readline("Enter a column (1, 2, 3): ")?;
        if valid_inputs.iter().any(|&s| column_line == s) {
            break;
        };
    }

    Ok((row_line, column_line))
}
```
\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

General impression

The code is clear and concise — it is a great pleasure to read your code. This review is thus less about problems than it is about pointing out opportunities to simplify the code.

The split between game logic and UI also seems fine — I wouldn't worry too much about it at this point.

Naming

Some personal thoughts on naming:

  • simplify BoardValue to Cell or similar;

  • rename X and O to Cross and Nought for more legibility;

  • rename BoardValue::player to something along the lines of BoardValue::unwrap_player (following Option::unwrap), to clearly indicate its panicking behavior;

  • rename play, which is too generic, to try_place.

User interface

I would index the board by

+---+---+---+
| 1 | 2 | 3 |
+---+---+---+
| 4 | 5 | 6 |
+---+---+---+
| 7 | 8 | 9 |
+---+---+---+

rather than use (row, column), for simplicity.

Board computation

get_winnable_slices and get_all_cells feel wasteful — an iterator or callback interface should be possible in theory.

Using crates

There are some crates you can use to reduce boilerplate:

Helper functions

Ideas for helper functions:

  • BoardValue::is_filled, to reduce the number of matches!;

  • Player::toggle, to toggle the player.

Miscellaneous

For BoardValue and Player, I would derive Eq in addition to PartialEq.

Is there a specific reason why X is the default player? I would leave Default out if the selection is arbitrary.

For Game::new, you can derive(Default) and then call it.

\$\endgroup\$
2
\$\begingroup\$

Nicely organized code! In addition to what L.F. suggested:

  • The fn player(self) looks like a questionably useful function; it basically wraps normal pattern matching of an enum type, and since it can panic it is likely to obfuscate rather than clarify wherever it is used. Indeed, closer inspection reveals you only use this function once, and you can replace this, for example with (I've renamed BoardValue to Cell following L.F.'s idea):

    fn get_game_status(&self) -> GameStatus {
    for slice in self.board.get_winnable_slices() {
        if let Cell::Filled(player) = slice[0] {
            if slice[0] == slice[1] && slice[1] == slice[2] {
                return GameStatus::PlayerWins(*player);
            }
        }
    }

  • Defining Cell before Player is difficult to follow. Better to put pub enum Player at the top and then define Cell afterwards.

  • The use of impl Deref and impl DerefMut for Board is clever! One thing to consider is that this makes type conversions implicit, so if you do something weird (like calling a function on the underlying [[Cell; 3]; 3] that you didn't know existed), the compiler won't give you a type error. I think it is probably worth it for convenience in this case. But an alternative would be to write getters and setters for grid cells, like fn get_cell(&self, i: usize, j: usize) -> Cell and fn set_cell(&mut self, i: usize, j: usize). This could be more extensible later if you add more fields to the Board struct.

  • Your error type pub enum PlayError is great design; extensible for other error types. A problem, though, is that pub fn play seems to implicitly panic if given an out-of-range row and column. Since you are already returning Result, I think it would be better to include an out-of-bounds error: i.e.

    pub fn play(&mut self, row: usize, column: usize) -> Result<GameStatus, PlayError> {
    if row > 2 || column > 2 {
        return Err(PlayError::OutOfBounds)
    }
    if matches!(self.board[row][column], Cell::Filled(_)) {
        return Err(PlayError::AlreadyFilled);
    }

    This may also simplify and clarify the logic in main as you don't can handle the Result type instead of checking the bounds yourself.

  • In your get_all_cells function, instead of returning a vector you can return an iterator. That works like this:

    fn iter_cells(&self) -> impl Iterator<Item = &Cell> {
    self.iter().flatten()
}

    This eliminates a needless iterator-to-vector-to-iterator conversion, since you are only using get_all_cells in order to iterate anyway. So in get_game_status you would just use if self.board.iter_cells().all(...) { ... }

  • The board length 3 is a magic constant. Since it's only used in a few places I would prefer const BOARD_ROWS: usize = 3 and const BOARD_COLS: usize = 3. This is also fun because you can then try out tic-tac-toe on other board dimensions like 2-by-5 or something, but you would have to re-implement get_winnable_slices in a more general way.

  • Minor but worth thinking about: I am overall not sure that having both Game and Board types is worth it. It is not clear that there is functionality specific to Board that isn't just the fact that it's a grid of cells. If you move impl fmt::Display, get_winnable_slices, and iter_cells to Game, then you can get rid of Board entirely and avoid the indirection of having to implement Deref and DerefMut. Or you could still keep these functions separate, but more in a functional style rather than object-oriented.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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