Conway's game of life in the terminal [beginner needs review]

code review
1

Hello, I'm new to Rust (discovered this amazing language last week coming from little experience with C++) coming back in programming(it's been a month) from a few years without programming, as I plan to become a professional programmer (starting CS/IT studies in France in September).
I made a Conway's game of life in the terminal as a first Rust project, it works on both Linux and Windows.
I would please like some code review on what I should or shouldn't have done. Thank you :wink:
Here is the code: terminal-game-of-life/main.rs at main · KrySoar/terminal-game-of-life · GitHub

2

Well, for one I would probably add a #[derive(PartialEq, Eq)] on your State enum so you can do upper_left == State::Alive rather than matches!(upper_left,State::Alive).

Other than that, you have a few warnings that I would fix. I would also try running clippy on it, which gives a few more warnings.

3

Comments are not in order of importance, but in the order they popped into my mind:

  • You are using signed 16-bit integers for sizes; that's inappropriate. Okay, 16 bits may be enough for all practical uses, but why signed? You can't have a negative-sized grid. You end up always casting the indexes/sizes to usize anyway, so why aren't you using usize in the first place? This is what usize is for.
  • You should be #[derive]ing standard traits such as Default and Debug for your Grid.
    • Relatedly, lines 19–22 are just a very verbose way of writing vec![State::Dead; height * width].
  • There is no good reason why the get_state() function returns a reference. Primitive types and pure data in general should really just be passed around by value.
    • Maybe you were thinking to implement Index and IndexMut instead?
    • There is also an inconsistency between set_state() and get_state() regarding out-of-bounds access. The former will simply panic, while the latter simply returns State::Dead. Decide which one you want to do and go with one, but don't mix the two behaviors, it will confuse others reading/using your code.
  • You are not using formatting, whitespace, etc. consistently. Sometimes, you completely omit whitespace around punctuation, at other times, you are writing 2 spaces (e.g. the end of line 54). Relatedly, it's weird that every function but clear_screen() is above main, while clear_screen() is below main.
  • count_neighbors() is way too complicated. So is next_gen(). I don't know why many sources formulate the GoL updating rule in terms of many cases – it really doesn't help. There is a simpler closed formula that counts the cell itself as a "neighbor".
    • Relatedly, in main, while initializing the grid, you are calling the set_state() function many times with slightly different arguments, which is ugly (unnecessarily verbose). You should pull out those arguments in an array and use a single function call in a loop instead.
  • The delay() function is plainly unnecessary. It merely takes away call-side information by not including the unit of time it is delaying for. This looks like a bad habit borrowed from languages like… IIRC Pascal? (Or what language has a function called delay()?)
  • Clearing the screen on Unix-like systems can (and should) be done without calling the shell 16 times a second. Google "ANSI terminal control codes".
  • I'm not sure how necessary the State enum is; since the algorithm is a well-known one and the cells don't have any particular semantic meaning, they might as well be bool, which has the added benefit that you get much of a primitive type's rich API for free (e.g. your State is not comparable for equality, not sortable, does not have a default value, etc.)
  • Since you know the size of the grid, you should preallocate space for the new vector in next_gen() using Vec::with_capacity(). By the way, lines 96–105 are a very convoluted way of writing let mut newvec = self.cells.clone().

All in all, I would have written it like this.

3 Likes
4

This might be a feature (albeit not very obvious), since querying the out-of-bounds cell is OK - it will contribute to the calculations as if it's dead, but writing to it is a programming error.

5

That's possible, but it's still bad style.

6

I don't think this is especially bad, but maybe it would be clearer to have get_state return an Option<State>. The caller can then explicitly pick a default value like this:

let upper_left = self.get_state(x-1,y-1).unwrap_or(State::Dead);

The documentation for unwrap_or can be found here.

2 Likes
7

Thank you I didn't know that but it is much better.

9

-I don't see why I should derive the Default and Debug trait for grid ?
-For the vec! macro I didn't know it because I a have not yet finished to read the Rust book i'm at the enum chapter.

-I rectified the get_state returning a reference
for the clear_screen() function I've tried some codes but I don't know how to format them in Rust because it doesn't seem to work,and I think calling the clear command is not very demanding ?

I almost rectified all of what you said. Thank you for the time you took to review my code !

10

There is also an inconsistency between set_state() and get_state() regarding out-of-bounds access. The former will simply panic, while the latter simply returns State::Dead .

It was intended to do as if a cell outside the grid were dead. So when it checks for neighbors cell it does not generate an error when verifying for example (x-1,y-1) when x or y is 0

11

You should generally try to implement standard traits if they make sense to exist on your types, this will make others' life easier to work with your code.

You can look at how I did it. It only requires printing an escape character and a bunch of other control codes directly. It does work in the code I wrote – I tried it myself.

12

Thank you but for me it doesn't clear the terminal, if I go up the screen I can see the past generations.

13

That is generally how most terminal emulators work.

14

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.