0
\$\begingroup\$

I just stated learning Rust (again...), here is a short bit of code I wrote:

#![allow(non_snake_case)]

/**
 * takes a 2D Vec and returns a 90° rotated version 
 */
pub fn createRotated90(arr: &Vec<Vec<bool>>) -> Vec<Vec<bool>> {
    let newWidth = arr[0].len();
    let newHeight = arr.len();
    let mut newRetArr = createNewFilled2DVec(newWidth as u8, newHeight as u8, false);

    forEachXY(
        newHeight as u8, 
        newWidth as u8, 
        &mut |x: u8, y: u8| newRetArr[y as usize][newHeight - x as usize - 1] = arr[x as usize][y as usize]
    );

    newRetArr
}

pub fn createNewFilled2DVec<T: std::clone::Clone>(width: u8, height: u8, filler: T) -> Vec<Vec<T>> {
    vec![vec![filler; height as usize]; width as usize]
}

pub fn forEachXY<F: FnMut(u8, u8)>(width: u8, height: u8, mut func: F) {
    let mut x: u8 = 0;
    let mut y: u8 = 0;

    while x < width {
        while y < height {
            func(x, y);

            y += 1;
        }

        x += 1;
    }
}

I am hoping for some general suggestion on how to improve my "Rusting".
Some points I noticed:

  • a lot of as usize
  • It seems Rust doesn't want me to use "normal" for loops (see forEachXY)
  • I am using Vec's instead of arrays, even though they don't change in size
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to Code Review. Please add an explanation of the purpose of your code. \$\endgroup\$
    – L. F.
    May 17, 2020 at 2:21
  • \$\begingroup\$ @L.F. my code has no real purpose, I just wrote it to get to know Rust \$\endgroup\$
    – Teiem
    May 17, 2020 at 11:44

1 Answer 1

3
\$\begingroup\$

Correctness

In

let mut x: u8 = 0;
let mut y: u8 = 0;

while x < width {
    while y < height {
        println!("{}, {}", x, y);
        func(x, y);

        y += 1;
    }

    x += 1;
}

y increments up to height, but then never gets reset. That means that the inner loop only runs once. To fix this, move the initialization of y inside the loop, or use a "normal loop" (see below).


Formatting and convention

Right now, it looks like you're trying to write Rust in a different language. Whenever possible, use the conventions suggested by the Rust compiler and the linter clippy (run cargo clippy). In particular, using #![allow(non_snake_case)] to set your own convention is discouraged.

A few other things:

  • Use /// for documentation. /* */ comments are fairly rare and /// comments will automatically be included in the documentation produced by cargo doc.
  • Keep lines fairly short. It's OK to go over the traditional limit of 80 characters sometimes, but whenever possible, break up lines for readability. For example, with a function signature or call, the arguments can be placed on separate lines. cargo fmt will do this automatically to the best of its ability.
  • Vec<T> is itself a pointer (and length and capacity), so &Vec<T> has no advantage over &[T]. In fact, due to automatic deref coercion, using the latter in a function signature allows that function to take either kind of argument. See this question for more.

Use for loops.

When iterating over a collection, for loops are usually the best way. There's no need to keep track of mutating variables. If an index is needed, the enumerate method in the Iterator trait can be used.

Here's how I'd rewrite your loop.

for (x, col) in arr.iter().enumerate() {
    for (y, &elem) in col.iter().enumerate() {
        new_arr[y][new_height - x - 1] = elem
    }
}

Add tests

Before I even touched your code, I made sure to add at least one test case. That alone caught the bug that I mentioned above. Adding more was very helpful when figuring out how the width and height worked (see the first item in Miscellaneous below). Just as a template, you can use

#[cfg(test)]
mod tests {
    use super::*;

    fn my_test() {
        // Use things like `assert!` and `assert_eq!` to verify the output of your functions
        // ...
    }
}

Tests can be run with cargo test.


Miscellaneous

This may just be personal preference, but when I see a 2-dimensional array like

vec![
    vec![1, 2, 3],
    vec![4, 5, 6],
]

I think of this has having a width of 3 and a height of 2. That is, the first row is [1, 2, 3] and the second row is [4, 5, 6]. Indexing at (x, y) is arr[y][x] (first access yth row, then xth column). From what I can tell, you've reversed this. For example, vec![vec![filler; height as usize]; width as usize] would have a height of width and a width of height in my convention.

Note that rotating the vector switches the role of height and width, so new_width is the height of the original and new_height is the width of the original.


The u8/usize problems were largely solved by rewriting the loop, but in general, I'd suggest just sticking with one type. There's no reason for create_new_filled_2d_vec or for_each_xy to take u8 arguments. That just leads to unnecessary conversions. Since the length of a vector is always usize, it makes sense to just use usize everywhere.


If you want to use arrays, you'll eventually be able to use const generics to accomplish this. For now, they'll need to be dynamically allocated as Box<[T]>. You can produce a Box<[T]> from a Vec<T> using the as_boxed_slice method. Since the only difference between this and Vec<T> is the capacity field, Box<[T]> is fairly rare in practice. The benefit isn't very large.

\$\endgroup\$

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