3
\$\begingroup\$

I've been learning Rust for a few weeks in my free time, and as a first project I decided to make a simple terminal Game of Life program (without UI, that might come later). It just starts with a glider and loops indefinitely (Torus topology). I decided to keep it simple so I can learn from early mistakes before I start to make it more complex. Here are the 3 files of the project (The only dependency I'm using is termion for colored terminal output):

grid.rs

use std::fmt;

pub struct Grid {
    rows: usize,
    cols: usize,
    data: Vec<bool>,
    aux_data: Vec<bool>,
}

impl Grid {
    pub fn new(rows: usize, cols: usize) -> Self {
        Grid {
            rows: rows,
            cols: cols,
            data: vec![false; rows * cols],
            aux_data: vec![false; rows * cols],
        }
    }

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

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

    pub fn get_cell(&self, row: usize, col: usize) -> bool {
        self.data[row*self.get_cols() + col]
    }

    pub fn set_cell(&mut self, value: bool, row: usize, col: usize) {
        let ncols = self.get_cols();
        self.data[row*ncols + col] = value;
    }

    fn count_alive_neighbours(&self, row: usize, col: usize) -> i32 {
        let offsets: [(usize, usize); 8] = [
            (self.get_rows()-1, self.get_cols()-1), (self.get_rows()-1,  0), (self.get_rows()-1,  1),
            (                0, self.get_cols()-1),                          (                0,  1),
            (                1, self.get_cols()-1), (                1,  0), (                1,  1)
        ];

        let mut count = 0;
        
        for offset in offsets.iter() {
            let is_alive = self.get_cell(
                (row + offset.0)%self.get_rows(),
                (col + offset.1)%self.get_cols()
            );
            if is_alive {
                count += 1
            }
        }

        count
    }

    pub fn update(&mut self) {
        use std::mem::swap;

        self.aux_data.resize(self.data.len(), false);
        let ncols = self.get_cols();
        
        for row in 0..self.get_rows() {
            for col in 0..self.get_cols() {
                let is_alive = self.get_cell(row, col);
                let alive_neighbours = self.count_alive_neighbours(row, col);
                self.aux_data[row*ncols + col] = match (is_alive, alive_neighbours) {
                    (true, n) if n < 2 || 3 < n => false,
                    (false, 3) => true,
                    (cell, _) => cell,
                };
            }
        }

        swap(&mut self.data, &mut self.aux_data);
    }
}

impl fmt::Display for Grid {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use termion::color;
        for index_row in 0..self.get_rows() {
            for index_col in 0..self.get_cols() {
                match self.get_cell(index_row, index_col) {
                    false => write!(f, "{}\u{25FC} ", color::Fg(color::White))?,
                    true => write!(f, "{}\u{25FC} ", color::Fg(color::Red))?,
                }
            }
            write!(f, "\n")?
        }

        Ok(())
    }
}

lib.rs

pub mod grid;

main.rs

extern crate game_of_life;

use game_of_life::grid::Grid;
use std::{thread, time};

fn main() {
    let mut grid = Grid::new(5, 5);
    grid.set_cell(true, 1, 2);
    grid.set_cell(true, 2, 3);
    grid.set_cell(true, 3, 1);
    grid.set_cell(true, 3, 2);
    grid.set_cell(true, 3, 3);
    
    loop {
        print!("{}\n", grid);
        grid.update();
        thread::sleep(time::Duration::from_secs(1));
    }
}

I come from a very strong C++ background, so some C++ practices might leak in my Rust code, but I promise I want to learn about them and try to think in the Rust way.

Thank you everyone and have a nice day!

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Firstly, there are a few simple changes I would recommend (for future reference, these can be picked up by running cargo clippy):

  • In main, you have print!("{}\n", grid); - this can be replaced with println!("{}", grid);
  • In your Display implementation for Grid, you have write!(f, "\n")?; - this can be replaced with a writeln!(f)?;
  • In Grid::new, you have
Grid {
    rows: rows,
    cols: cols,
    ...
}

This can be replaced with a more succinct

Grid {
    rows,
    cols,
    ...
}
  • You have the condition if n < 2 || 3 < n in Grid::update - clippy recommends changing this to if !(2..=3).contains(&n) which is more clear to me.
  • I also recommend running cargo fmt which ensures you're using the standard rust formatting, but this removes your special formatting on offsets in Grid::count_alive_neighbours. If you want to keep this, you can add #[rustfmt::skip] to the line before the variable declaration.

Now for changes which cargo clippy cannot pick up:

  • Your type annotation on offsets is unneeded.
  • It isn't really common practice to have use statements in functions - most of the time you simply place them at the top of the file.
  • for offset in offsets.iter() can be written more succinctly as for offset in &offsets
  • In fact, that code block can be rewritten to completely avoid the need for a mutable variable, using iterators.
let count = offsets
    .iter()
    .filter(|offset| {
        self.get_cell(
            (row + offset.0) % self.get_rows(),
            (col + offset.1) % self.get_cols(),
        )
    })
    .count() as i32;

And in doing so, you can also remove the need for the count variable entirely - by just returning that iterator statement instead of assigning to a variable and then returning. It is up to you to decide if you prefer this, but in general it is good practice to avoid mutable variables where possible.

  • I'm not sure if self.aux_data.resize(self.data.len(), false); is required in Grid::update? aux_data and data are created with the same size, and new items are never added - so both the size and the capacity should remain the same.
  • I would also recommend adding doc comments and potentially doc tests. As an example, you could write Grid::new as
/// Initialises a new grid with the given size, with both data and aux_data filled with empty cells.
pub fn new(rows: usize, cols: usize) -> Self {
    Grid {
        rows,
        cols,
        data: vec![false; rows * cols],
        aux_data: vec![false; rows * cols],
    }
}

More information on doc comments & tests can be found at https://doc.rust-lang.org/rustdoc/what-is-rustdoc.html.

  • In general, simple getters & setters (like get_cols) should not be used in other methods - so you could replace
pub fn set_cell(&mut self, value: bool, row: usize, col: usize) {
    let ncols = self.get_cols();
    self.data[row * ncols + col] = value;
}

with

pub fn set_cell(&mut self, value: bool, row: usize, col: usize) {
    self.data[row * self.cols + col] = value;
}

and so on for your other methods (such as count_alive_neighbours). The getters used here don't provide any extra functionality, and as such they're not really useful since you have access to private members anyways.

  • It might also be worth storing enum variants for cells instead of a boolean. For example, you could define an enum (the Clone derive is needed for the vector initialization, Copy for get_cell, and PartialEq for checking whether cells are alive/dead in count_alive_neighbours)
#[derive(Clone, Copy, PartialEq)]
enum Cell {
    Dead,
    Alive,
}

and then store that in data and aux_data, replacing all false with Cell::Dead and true with Cell::Alive. This helps to make it more immediately understandable. This enum and a bool also have the same size (1 byte) so it won't have performance impacts. As an example, the match part of your Display implementation might look like the following:

match self.get_cell(index_row, index_col) {
    Cell::Dead => write!(f, "{}\u{25FC} ", color::Fg(color::White))?,
    Cell::Alive => write!(f, "{}\u{25FC} ", color::Fg(color::Red))?,
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much for taking your time in reviewing my code! I've learnt a lot with this single review (didn't know about clippy, nor fmt). There's only one thing I don't really agree, and it's about the getters/setters. While I agree those don't do anything special in this case, I prefer to interface with them everywhere in case I want to change my underlyned representation. Maybe in the future I want to use an array2d crate and get rid of rows and cols variables. I know this is very object oriented and opinionated, but I prefer to make a change in just one place rather in many places. \$\endgroup\$ May 30, 2021 at 16:45
  • \$\begingroup\$ For the getters, I'd normally agree with you - I just pointed it out here since the logic for getting the number of columns/rows is very unlikely to change (in fact, the only way I can see it changing is if you change the underlying representation, and in that case you'd have more code to change anyways). So for me, I'd rather just use the variables directly, as that removes one layer of indirection, making it simpler to understand - but yes, like you said, this is very subjective and it's really up to you to decide. \$\endgroup\$ May 30, 2021 at 16:54

Your Answer

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

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