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);
 }
}
200_success
146k22 gold badges190 silver badges478 bronze badges
asked Apr 10, 2022 at 15:16
\$\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.

answered Mar 9, 2024 at 17:02
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.