Tic-tac-toe implementation

Question

I created a tic-tac-toe game implementaiton in rust. What I can do to make this tic-tac-toe code a better code?

game.rs

pub struct Board {
    board: Vec<Vec<char>>,
}
pub struct Cell {
    x: usize,
    y: usize,
}

pub enum ErrCoordinates {
    InvalidSub,
}

pub enum ErrBoard {
    OutOfBounds,
    PossitionTaken,
}

impl Cell {
    pub fn new() -> Cell {
        Cell { x: 0, y: 0 }
    }
    pub fn index_to_coordinates(&mut self, i: usize) -> Result<(), ErrCoordinates> {
        let i = i.checked_sub(1);
        match i {
            Some(u) => {
                self.x = u / 3;
                self.y = u % 3;
                Ok(())
            }
            None => Err(ErrCoordinates::InvalidSub),
        }
    }
}

impl Board {
    pub fn new() -> Board {
        Board {
            board: vec![vec!['\u{25A2}'; 3]; 3],
        }
    }
    pub fn print_a_board(&self) {
        for i in 0..3 {
            for j in 0..3 {
                print!("{:2}", self.board[i][j]);
            }
            println!();
        }
    }
    pub fn place_a_sign(&mut self, cell: &mut Cell, sign: &mut char) -> Result<(), ErrBoard> {
        match self.board.get(cell.x) {
            Some(_) => match self.board[cell.x].get(cell.y) {
                Some(_) => {
                    if self.board[cell.x][cell.y] == '\u{25A2}' {
                        self.board[cell.x][cell.y] = *sign;
                        if *sign == 'X' {
                            *sign = 'O';
                        } else {
                            *sign = 'X';
                        }
                    } else {
                        return Err(ErrBoard::PossitionTaken);
                    }
                    Ok(())
                }
                None => Err(ErrBoard::OutOfBounds),
            },
            None => Err(ErrBoard::OutOfBounds),
        }
    }
    pub fn check_for_win(&mut self) -> Option<char> {
        //check for a horizontal win
        for row in self.board.iter() {
            if !row.iter().any(|i| *i == '\u{25A2}') && row.iter().all(|&x| x == row[0]) {
                return Some(row[0]);
            }
        }
        //check for a vertical win
        for (i, enumerator) in self.board.iter().enumerate() {
            if !self.board.iter().any(|row| row[i] == '\u{25A2}')
                && self.board.iter().all(|x| x[i] == enumerator[i])
            {
                return Some(enumerator[i]);
            }
        }
        //check for a diagonal(upper left to lower right) win
        for (i, enumerator) in self.board.iter().enumerate() {
            if !self
                .board
                .iter()
                .enumerate()
                .any(|(i_inner, row)| row[i_inner] == '\u{25A2}')
                && self
                    .board
                    .iter()
                    .enumerate()
                    .all(|(i_iner, row)| row[i_iner] == enumerator[i])
            {
                return Some(enumerator[i]);
            }
        }
        //check for a diagonal (lower left to upper right) win
        for (i, enumerator) in self.board.iter().rev().enumerate() {
            if !self
                .board
                .iter()
                .rev()
                .enumerate()
                .any(|(i_inner, row)| row[i_inner] == '\u{25A2}')
                && self
                    .board
                    .iter()
                    .rev()
                    .enumerate()
                    .all(|(i_inner, row)| row[i_inner] == enumerator[i])
            {
                return Some(enumerator[i]);
            }
        }
        //ceck for a tie
        if self
            .board
            .iter()
            .all(|x| x.iter().all(|&y| y != '\u{25A2}'))
        {
            return Some('T');
        }
        None
    }
}

main.rs

use std::io;

mod game;

use game::*;

fn main() {
    println!("\tTIC-TAC-TOE");
    println!(
        "The rulse:
            \rEvery turn you asked where you want to place your sign(O or X).
            \rIf you fill any row column or horizontal with your signs you win."
    );

    let mut b = Board::new();
    let mut sign: char = 'X';
    let mut cell = Cell::new();
    loop {
        b.print_a_board();

        eprint!("Enter a cell number:");

        let mut cell_index = String::new();

        io::stdin()
            .read_line(&mut cell_index)
            .expect("Error reading the input!");

        match cell_index.trim().parse::<usize>() {
            Ok(i) => match cell.index_to_coordinates(i) {
                Ok(a) => a,
                Err(ErrCoordinates::InvalidSub) => {
                    eprintln!("Enter a valid number!");
                    continue;
                }
            },
            Err(_) => {
                eprintln!("Enter a valid number!");
                continue;
            }
        }

        match sign {
            'X' => match b.place_a_sign(&mut cell, &mut sign) {
                Ok(a) => a,
                Err(ErrBoard::PossitionTaken) => {
                    eprintln!("This possition is already taken! Try another one!");
                    continue;
                }
                Err(ErrBoard::OutOfBounds) => {
                    eprintln!("Enter a valid number!");
                    continue;
                }
            },
            'O' => match b.place_a_sign(&mut cell, &mut sign) {
                Ok(a) => a,
                Err(ErrBoard::PossitionTaken) => {
                    eprintln!("This possition is already taken! Try another one!");
                    continue;
                }
                Err(ErrBoard::OutOfBounds) => {
                    eprintln!("Enter a valid number!");
                    continue;
                }
            },
            _ => panic!("Unpresented player "),
        }

        if let Some(winer) = b.check_for_win() {
            b.print_a_board();
            if winer == 'T' {
                println!("It's a tie!");
                break;
            }
            println!("{} won the game!", winer);
            break;
        }
    }
}

Answer 1

I see you've absorbed many things I mentioned in my last review — well done!

Coincidentally (or perhaps not, since tic-tac-toe is a common exercise for beginners), I wrote my version of tic-tac-toe last year. I still considered myself a nauplius at that time, but I hope you find certain aspects of my code worth learning from. Notably, there was a lot of overengineering, since I was trying to make the most out of the exercise.

Let's look for more opportunities for improvement. This time, we'll start from the big picture and proceed to the details after.

Using enums

The current player, which you represent with the char values 'X' and 'O', is best represented as an enum:

enum Player {
    Cross,
    Nought,
}

with a Display implementation that converts the values into their corresponding signs.

The status of the game can be represented as an Option<Event>, where Event records events that require action:

enum Event {
    Win(Player),
    Tie,
}

The same goes for each cell of the board:

enum Cell {
    Occupied(Player),
    Vacant,
}

With these enum definitions, you will see that match expressions seamlessly match the logic of your code.

Managing game state

The game has, explicitly or implicitly, two values to keep track of: the contents of the board and the current player. It is a good idea to group the mutable game state into a struct:

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

and define methods that operate on a &mut Game.

The main function

As was the case for last time, the biggest problem of the main function is that its general structure is not clearly represented. Let's sort out the logic first:

• print the welcome message;

• enter the game loop:

  • input the position;

  • place a mark at the specified location;

  • check for a win or a tie.

(See my last review for an example function that inputs a value.)

Compare your main function to my Session::run function, which plays a similar role:

pub fn run(&mut self) -> Result {
    let mut current_player = self.first_player;

    loop {
        self.process_move(current_player);

        if let Some(player) = self.resigned {
            utility::clear_screen();
            print!("{}", self.board);

            let winner = player.toggle();
            println!("{} wins by resignation.", winner);
            return Result::Win(winner);
        } else if self.board.wins(current_player) {
            utility::clear_screen();
            print!("{}", self.board);
            println!("{} wins.", current_player);
            return Result::Win(current_player);
        } else if self.board.is_draw() {
            utility::clear_screen();
            print!("{}", self.board);
            println!("It's a draw.");
            return Result::Draw;
        }

        current_player = current_player.toggle()
    }
}

There is less clutter in my code, but the logic would have been even clearer if the whole if chain were extracted into a dedicated function check_status.

The two arms of match sign are identical — combine them into a single 'X' | 'O' => { ... } arm. Alternatively, the match can be eliminated if you use an enum, as mentioned before.

struct Board

Vec<Vec<char>> is a very inefficient way to represent the board state — consider [char; 9], or, better, [Cell; SIZE], where Cell is a dedicated enum and SIZE is a const.

struct Cell

I can't say this is a cell:

pub struct Cell {
    x: usize,
    y: usize,
}

Instead, it is a position, or a coordinate. Since there isn't really a default position in the board, just leave out fn new.

index_to_coordinates, renamed to from_index by convention, is considered a constructor. Instead of modifying the value of a mutable variable, constructors create a new value and return it by value. It also checks whether the coordinate is indeed a valid one, not just that it is >= 1. The equivalent from my version is, after optimization:

// a position on the board
// 1 2 3
// 4 5 6
// 7 8 9
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct Pos(usize);

impl Pos {
    pub fn new(pos: usize) -> Option<Pos> {
        if (1..=Board::SIZE).contains(&pos) {
            Some(Pos(pos))
        } else {
            None
        }
    }
}

A Result<Pos, PosError> would be fine too.

Status check

fn check_for_win looks rather convoluted — methods like step and take will come in handy if you use a flat representation [Cell; 9].

Error handling

I highly recommended the anyhow and thiserror crates for handling errors and creating error types — see their documentation for details.

Magic numbers

Replace magic numbers with named consts.

const BOARD_SIZE: usize = 3;
const VACANT_CELL: char = '\u{25A2}';
// etc.

Miscellaneous

Typos: possition => position, winer => winner.

It is unnecessary to repeat the type in the name of method: instead of b.print_a_board(), it is more natural to rename b to board and write board.print().

The indoc crate helps to write multiline string literals in an aesthetically pleasing manner without hacks like \r.

The distinction between print! vs. eprint!, I think, is less important this time, since the user interface isn't designed for automation anyway.

---

There are some details that I didn't go into because of time constraint, so feel free to ping me if you need any clarification.