#How ca I make this code better? [Code review]

40 messages · Page 1 of 1 (latest)

livid tide
livid tide
#

I feel like there's too many explicit loops in the program.

inland sorrel
#

this might be a bit more idiomatic: ```rust
fn get_available_cells(field: &Vec<Vec<char>>) -> Vec<(usize, usize)> {
field
.iter()
.enumerate()
.map(|(y, row)| {
row.iter()
.enumerate()
.filter_map(move |(x, &c)| (c == '_' || c == ',').then_some((x, y)))
})
.flatten()
.collect()
}

#

however, if you're dealing with 2D arrays, you might want to pull in

#

?crate ndarray

bronze bronzeBOT
#
ndarray

An n-dimensional array for general elements and for numerics. Lightweight array views and slicing; views support chunking and splitting.

Version

0.15.6

Downloads

10 101 531

inland sorrel
#

though it might be a bit overkill since performance doesn't look to be a concern

#

you can also reduce get_neighbors() to ```rust
fn get_neighbors(field: &Vec<char>, index: usize) -> (Option<char>, Option<char>, Option<char>) {
(
(index != 0).then(|| field[index - 1]),
Some(field[index]),
(index != field.len() - 1).then(|| field[index + 1])
)
}

#

however, I would actually change the return type to (Option<char>, char, Option<char>), or potentially even have it not return that middle field, since it will always be Some

#

(condition).then[_some]() is a very powerful pattern though

#

also towards the end of your main function, you can do this: ```rust
if event::poll(std::time::Duration::from_millis(16))? {
if let event::Event::Key(KeyEvent {
kind: KeyEventKind::Press,
code: KeyCode::Char('q'),
..
}) = event::read()?
{
break;
}
}

#

this might also be an alternate way to initialize original_field, but I think yours works fine too ```rust
// Initialize the original field
// This will never change
let mut original_field: Vec<Vec<char>> = iter::repeat_with(|| {
iter::repeat_with(|| if rng.gen_bool(1.0 / 3.0) { ',' } else { '_' })
.take(FIELD_COLUMNS)
.collect()
})
.take(FIELD_ROWS)
.collect();

#

also, an alternate way to do the setting of display_field: ```rust
let mut display_field = work_field
.iter()
.enumerate()
.map(|(y, row)| {
row.iter()
.enumerate()
.filter_map(|(x, c)| match get_neighbors(row, x) {
(, Some(), Some('o')) => Some('('),
(Some('o'), Some(_), ) => Some(')'),
(, Some('('), ) | (, Some('o'), ) | (, Some(')'), _) => {
Some(original_field[y][x])
}
_ => Some(*c),
})
.collect()
})
.collect();

livid tide
#

Thank you very much @inland sorrel, I think I understand more or less the fixes you propose, now I just need to have a better grasp on ownership/borrowing. I have some difficulty understanding the flow of data without the compiler.

inland sorrel
#

Overall though, your original code is pretty good, and made a lot of sense to read through.

livid tide
#

I have a question: what is the "move" keyword used for? Do comparison take ownership of c and thus the reference would be invalid?

inland sorrel
#

and a closure is basically just a function, with a struct that the compiler builds that holds all the stuff it captures

#

so by default, that "captured stuff" struct holds a bunch of references

#

if you specify move, it means, capture that stuff by value instead

#

i.e., "move the captured items out of the surrounding environment and into the closure"

inland sorrel
# inland sorrel this might be a bit more idiomatic: ```rust fn get_available_cells(field: &Vec<V...

here: ```rust
.map(|(y, row)| {
row.iter()
.enumerate()
.filter_map(move |(x, &c)| (c == '_' || c == ',').then_some((x, y)))
})

if I hadn't specified `move`, then y would be captured by reference. But iterators are lazy, and because of that, this particular closure might run after the closure that contained it has already returned, and so a reference to `y` would be dangling. So we instead specify to `move` it, which (since `y` is `Copy`) copies `y` into the captured scope of the inner closure so it can be used even after the outer closure returns
#

however

#

most of the time the compiler will have your back on this. It'll usually even specify exactly where you need to put move if you need it

#

but it is good to understand what it actually does

livid tide
#

So @inland sorrel if I understand correctly, "move" doesn't act on x and &c but more so on y and row?

inland sorrel
#

correct

#

move acts on the values that the closure captures from its environment (those variables that are defined outside but used inside of the closure), not its arguments

inland sorrel
#

it's this: ```rust
1 error[E0373]: closure may outlive the current function, but it borrows y, which is owned by the current function
--> src/main.rs:119:29
|
119 | .filter_map(|(x, &c)| (c == '' || c == ',').then_some((x, y)))
| ^^^^^^^^^ may outlive borrowed value y - y is borrowed here
|
note: closure is returned here
--> src/main.rs:117:13
|
117 | / row.iter()
118 | | .enumerate()
119 | | .filter_map(|(x, &c)| (c == '' || c == ',').then_some((x, y)))
| |______________________________________________________________________________^
help: to force the closure to take ownership of y (and any other referenced variables), use the move keyword
|
119 | .filter_map(move |(x, &c)| (c == '' || c == ',').then_some((x, y)))
| ++++

For more information about this error, try rustc --explain E0373.
error: could not compile rain (bin "rain") due to previous error

livid tide
#

Thx. Do you know any resources to have a better understanding of Rust ownership/borrowing and data flow? I've read through the entire Rust book but I still don't really get it.

inland sorrel
#

yeah, probably

#

one sec

#

what lang are you coming from?

#

I'd recommend different resources based on what a good comparison point is

livid tide
#

I mainly a web and mobile dev, so JS and Dart.

inland sorrel