Rust Battleship - console game

help
1

Hi every one! I'm Rust beginner. Could somebody make a small review of my code ? https://github.com/SerhiiBilyk/Battleship
I will appreciate your help.

2

Howdy! I'm not going to fine comb through your code but I will recommend a few things you can do to improve and catch things:

  • Run clippy with cargo clippy
  • You have a few folders:
    /src/field/
/src/meditator/
/src/player/
/src/structures/
/src/utils/
    where each folder only contains a mod.rs. This can be made more idiomatic by changing the following:
    /src/field/mod.rs
/src/meditator/mod.rs
/src/player/mod.rs
/src/structures/mod.rs
/src/utils/mod.rs
    to
    /src/field.rs
/src/meditator.rs
/src/player.rs
/src/structures.rs
/src/utils.rs
  • It's rather odd to have both a lib.rs and a main.rs, any reasoning behind it?
  • Your tests should most likely be local to each relevant file, so tests for, say field.rs are in field.rs like so:
    //field.rs

<Your code>

#[cfg(test)]
mod tests {
   #[test]
   fn it_works() {}
}

Individual files:

  • utils.rs:
    • Although some may argue that generate_all_empty_points is mostly idiomatic, I'd say that the added verbosity is unnecessary and nested for loops would work fine.
    • status_u8 should be a function in an impl Status instead of a separate function
  • structures.rs:
    • Ship owns a &'static ShipDirection which seems odd, why doesn't it just have a ShipDirection?
    • Ship::get_all returns a HashMap<u8, u8> which is only populated with the keys [1, 2, 3, 4]. An array would suffice.
    • This is a bit of a nitpick, but could Draw have a better name?
    • PatrialEq can be derived for Point so you don't have to provide a naive implementation manually.
    • RandomNumber can be a misleading name, perhaps RandomNumberGenerator or RandomNumberFn?
  • player.rs:
    • Not much to say here about idiomatic code
  • mediator.rs:
    • read_line allocates a new String every time, while okay for user input, this could probably be better if we took a &mut String instead.
  • field.rs:
    • GameField once again with its HashMap<u8, u8> which could probably be avoided with a smaller structure
    • Your get_random_point function doesn't make very much sense to me, you initialize a Vec but then try to immediately break if it doesn't contain some other point (Which will always be the case since it's empty).
3 Likes
3

Thank you very much for such detailed comment. I'm using clippy-preview. Dont know what the diff between both. Rustup add clippy component - does not work on my linux distribution. I've created lib.rs and main.rs for learning purpose. I have frontend written in yew framework. Thank you again for your comment and help!

1 Like
4

I have seen this pattern used quite commonly in e.g. CLI tools, to seperate out the business logic (lib.rs) and the user interface (main.rs). That way if you want to use the logic as a library in another program further down the line, you're already set.

Not sure if that makes sense in this case, but there are valid reasons you might do it :slight_smile:

4 Likes
5

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.