The unasked for code review
CAD97 opened this issue · 7 comments
This is my first time using Rust, so the code probably isn't going to be very idiomatic or whatever, so feel free to tell me if I am doing something "not rusty" :).
Don't mind if I do! This will mostly be high-level notes, but I'd like to point out a few things for at least surface-level improvements.
Clippy
cargo clippy should be your friend. Do you like rustc's pickiness, and wish it would be more opinionated in an attempt to guide you to better code? That's what clippy is. With the magic of rustup and cargo, it's as easy to use on your project as:
$ rustup install nightly
$ rustup component add clippy-preview --toolchain=nightly
$ cargo +nightly clippy
clippy-preview is currently only available on the nightly toolchain, but is riding the trains to stable, just as happened with rustfmt-preview.
To overview a few of the lints it currently spits for this project:
warning: using `println!("")`
--> src\main.rs:15:5
|
15 | println!("");println!("");
| ^^^^^^^^^^^^ help: replace it with: `println!()`
|
= note: #[warn(println_empty_string)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#println_empty_string
warning: using `println!("")`
--> src\main.rs:15:18
|
15 | println!("");println!("");
| ^^^^^^^^^^^^ help: replace it with: `println!()`
|
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#println_empty_string
There's no reason to give an empty string to println. In this case I might actually write print!("\n\n"), as println!("\n") is probably a bit too interesting of a behavior.
warning: returning the result of a let binding from a block. Consider returning the expression directly.
--> src\game\game_state\state_explore.rs:36:9
|
36 | state
| ^^^^^
|
= note: #[warn(let_and_return)] on by default
note: this expression can be directly returned
--> src\game\game_state\state_explore.rs:31:21
|
31 | let state = StateExplore {
| _____________________^
32 | | player: Player::new(),
33 | | last_action: Action::NoAction,
34 | | maps: MapManager::new()
35 | | };
| |_________^
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#let_and_return
Asciimon/src/game/game_state/state_explore.rs
Lines 30 to 37 in 0d09090
This is more simply written as:
StateExplore {
player: Player::new(),
last_action: Action::NoAction,
maps: MapManager::new()
}The intermediate binding adds no information. Rust's grammar is expression-oriented, and single-expression functions are quite common in idiomatic code, especially for ::new() functions.
Some self-explanatory warnings for some things:
warning: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise
--> src\graphics\renderer.rs:106:20
|
106 | let sect = self.render_sections.get(section).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&self.render_sections[section]`
|
= note: #[warn(get_unwrap)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#get_unwrap
warning: the variable `line_num` is used as a loop counter. Consider using `for (line_num, item) in data.enumerate()` or similar iterators
--> src\graphics\renderer.rs:187:21
|
187 | for line in data {
| ^^^^
|
= note: #[warn(explicit_counter_loop)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#explicit_counter_loop
warning: identical conversion
--> src\graphics\colour.rs:26:9
|
26 | / String::from(format!(
27 | | "\x1b[{};2;{};{};{}m", id, r, g, b
28 | | ))
| |__________^ help: consider removing `String::from()`; `format!` returns a `String`
|
= note: #[warn(identity_conversion)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#identity_conversion
warning: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices.
--> src\game\game_state\mod.rs:16:44
|
16 | fn handle_input(&mut self, input_args: &Vec<&str>) -> ReturnResult;
| ^^^^^^^^^^ help: change this to: `&[&str]`
|
= note: #[warn(ptr_arg)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#ptr_arg
warning: use of `expect` followed by a function call
--> src\game\map.rs:44:18
|
44 | .expect(&format!("Unable to open file for map {} {}", x, y));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!("Unable to open file for map {} {}", x))`
|
= note: #[warn(expect_fun_call)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#expect_fun_call
warning: usage of `contains_key` followed by `insert` on a `HashMap`
--> src\game\map_manager.rs:32:17
|
32 | / if !self.maps.contains_key(&pos) {
33 | | let map = match Map::load(map_x, map_y) {
34 | | None => continue,
35 | | Some(map) => map
... |
38 | | self.maps.insert(pos, map);
39 | | }
| |_________________^ help: consider using: `self.maps.entry(pos)`
|
= note: #[warn(map_entry)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#map_entry
warning: you seem to want to iterate on a map's values
--> src\game\map_manager.rs:43:25
|
43 | for (_, map) in &self.maps {
| ^^^^^^^^^^
|
= note: #[warn(for_kv_map)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#for_kv_map
help: use the corresponding method
|
43 | for map in self.maps.values() {
| ^^^ ^^^^^^^^^^^^^^^^^^
warning: single-character string constant used as pattern
--> src\game\mod.rs:76:68
|
76 | let input_args: Vec<&str> = input.trim().split(" ").collect();
| -------------------^^^- help: try using a char instead: `input.trim().split(' ')`
|
= note: #[warn(single_char_pattern)] on by default
= help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#single_char_pattern
(I fixed the help string on identity_conversion; it's broken currently, and I submitted an issue for it.)
Actual human thought
Lines 11 to 17 in 0d09090
This applies to a few areas, and is something easily missable if you're used to other languages with a one-import-per-line design. You can (and it's typical to) import multiple names from a module in one use statement:
use self::game_state::{ReturnResult, GameState, state_explore::StateExplore};
use std::io::{Write, stdin, stdout};Another bit seen here is that in game/game_state/mod.rs, it might make sense to have pub use self::state_explore::StateExplore; then you can refer to it as game::game_state::StateExplore instead of game::game_state::state_explore::StateExplore.
Asciimon/src/game/game_state/state_explore.rs
Lines 103 to 114 in 0d09090
This could more cleanly be written using Rust's expression-orientedness:
fn update(&mut self) -> ReturnResult {
match self.last_action {
Action::NoAction => ReturnResult::None,
Action::MovePlayer(x, y) => {
self.handle_move_player(x, y);
ReturnResult::Redraw
}
}
}Lines 92 to 97 in 0d09090
This is because finding the nth character in a UTF-8 string is an O(n) operation. If you're sure that you've only got ASCII (single byte characters) in the string, you can convert it through a byte string (line.as_bytes()[x as usize] as char) or slice to the correct location (line[x as usize..].chars().next().unwrap_or('.')).
Asciimon/src/graphics/renderer.rs
Line 197 in 0d09090
Typically getters in Rust are named without a get_ prefix. This doesn't cause any problems: function and value namespaces are separate (for better or for worse).
The last note I have is that you tend to document your functions with /* */ mutliline comments: if you use /// doc comments, the documentation will show up in tooling (I'm unsure about RLS in vscode, but cargo doc for sure (for public functions of a library, internal binary documentation is... rough)). Everything else is me being overly picky about formatting since your formatting doesn't line up perfectly with cargo fmt.
I hope you've been enjoying Rust, it's definitely my favorite language right now. Every language has its use cases, but I certainly enjoy Rust because for me, it allows me to have the productivity I have in Java/Kotlin but with much more control. Rust's borrow checker has made me a better programmer as I know much more about how my code treats data now. And fearless concurrency is nice too 😛.
This is a hopefully friendly member of the rust evangelicalism strike force signing off! I hope you enjoy this project and continue to use Rust in the future!
Disclaimer: I didn't actually attempt to compile any of the code, so I may have missed something. Of course, much of style is personal, and clippy's lints are intended to be ones that are opinionated in order to help you, but be turned off in some cases.
Ahh thank you very much, I will address these rusty issue now :)
I'll let you know if i have any questions and also when I have finished, thanks.
Thank you very much! The code is already feeling a lot better.
There was a couple of points I was unable to address though.
The first one was the one with iterating over a vector, while explicitly counting lines at the same time:
let mut line_num = 0;
for line in data {
self.draw_string(section, line, &Vector2D::new(position.x, position.y + line_num));
line_num += 1;
}Clippy suggested to change it into this:
for (line_num, line) in data.enumerate() {
self.draw_string(section, line, &Vector2D::new(position.x, position.y + line_num));
}But this gives the error:
error[E0599]: no method named `enumerate` found for type `&std::vec::Vec<std::string::String>` in the current scope
--> src\graphics\renderer.rs:186:38
|
186 | for (line_num, line) in data.enumerate() {
| ^^^^^^^^^
|
= note: the method `enumerate` exists but the following trait bounds were not satisfied:
`&mut &std::vec::Vec<std::string::String> : std::iter::Iterator`
`&mut std::vec::Vec<std::string::String> : std::iter::Iterator`
`&mut [std::string::String] : std::iter::Iterator`
and I am not entirely sure how to fix that.
And the only other issue is just some Rusty problem solving 😛
//To do: Improve the cotains key followed by the insert.
if !self.maps.contains_key(&pos) {
let map = match Map::load(map_x, map_y) {
None => continue,
Some(map) => map
};
self.maps.insert(pos, map);
}
}Clipy warns about having the insert statement after checking the key here, and suggests using self.maps.entry(pos) instead, but I am not sure how to go about doing this incase of the map failing to load, and then returning None
But apart from this, it's all good and thank you for the code review 😄
That for-loop over the vector data implicitly turns it into an iterator. To be able to call methods on the iterator, like .enumerate() you'll need to call .iter() on it explicitly.
for (line_num, line) in data.iter().enumerate() {
self.draw_string(section, line, &Vector2D::new(position.x, position.y + line_num));
}That should fix that bit.
Edit:
After some mangling I did this to the entry bit:
use std::collections::hash_map::Entry::Vacant;
for (x, y) in (x - 1..x + 2).zip(y - 1..y + 2) {
let pos = Vector2D::new(x, y);
if let Vacant(entry) = self.maps.entry(pos) {
if let Some(map) = Map::load(x, y) {
entry.insert(map);
}
}
}Weather or not it's better, I have little idea. I do like my if-let blocks. 😀
@ThomasLesmann I think it would be better to use contains_key instead of entry because it looks simpler and even takes 16 bytes less in a release binary (not a big deal, but it's a fact). Here's your code, but without entry:
for (x, y) in (x - 1..x + 2).zip(y - 1..y + 2) {
let pos = Vector2D::new(x, y);
if !self.maps.contains_key(&pos) {
if let Some(map) = Map::load(x, y) {
self.maps.insert(pos, map);
}
}
}Perfect, using iter() worked :) thanks
This is handled, closing