5
\$\begingroup\$

I have been interested in trying Rust in a long time, and I finally got around to sit down and start reading "The Book". I have not finished yet, so there are probably a few features that I could have used, but I didn't know about.

Anyway, the purpose of this program is to generate a maze (using the simplest "Randomized DFS" algorithm).

Since the main goal for me was to practice the language, I tried to make the program as "idiomatic" as possible. The main thing that still bothers me is all those type conversions between the signed and unsigned types. Since this is my first Rust program, I am mainly just looking for feedback on anything that could be considered "bad code", or parts that could be improved in any way.

extern crate rand;
extern crate image;
use std::io::Write;
use rand::Rng;

#[derive(Clone, PartialEq)]
enum Cell {
    Blocked,
    Free,
}

fn make_odd(mut t: (usize, usize)) -> (usize, usize) {
    let o = |v: &mut usize| if *v%2==0{*v+=1};
    o(&mut t.0); o(&mut t.1); t
}

fn gen(s: (usize, usize)) -> Vec<Vec<Cell>> {
    let mut t = vec![vec![Cell::Blocked; s.1]; s.0];
    let mut stack = Vec::<(isize, isize)>::new();

    let c = make_odd((s.0/2, s.1/2));
    stack.push((c.0 as isize, c.1 as isize));
    t[c.0][c.1] = Cell::Free;

    let mut dirs: [(isize, isize); 4] = [(2, 0), (-2, 0), (0, 2), (0, -2)];
    let mut rng = rand::thread_rng();
    'o: while let Some(&(x, y)) = stack.last() {
        rng.shuffle(&mut dirs);
        for i in 0..4 {
            let (dx, dy) = dirs[i];
            let (nx, ny) = (x+dx, y+dy);
            if nx < 0 || ny < 0 || nx >= (s.0 as isize) || ny >= (s.1 as isize) { continue; }
            if t[nx as usize][ny as usize] != Cell::Free {
                stack.push((nx, ny));
                t[nx as usize][ny as usize] = Cell::Free;
                t[(x+dx/2) as usize][(y+dy/2) as usize] = Cell::Free;
                continue 'o;
            }
        }
        stack.pop();
    }

    t[0][1] = Cell::Free;
    t[s.0-1][s.1-2] = Cell::Free;
    t
}

fn print_usage() -> ! {
    let _ = writeln!(std::io::stderr(), "Usage: maze-gen width height [output.png]");
    std::process::exit(1);
}

fn main() {
    let args: Vec<_> = std::env::args().collect();
    if args.len() < 3 { print_usage(); }
    let mut nums = [0; 2];
    for i in 0..2 {
        match args[i+1].parse::<usize>() {
            Err(_) => { print_usage(); },
            Ok(v) => nums[i] = v,
        }
    }
    let s0 = (nums[0], nums[1]);
    let mut s = make_odd(s0);
    if s.0 < 3 { s.0 = 3; }
    if s.1 < 3 { s.1 = 3; }
    if s != s0 {
        let _ = writeln!(std::io::stderr(),
            "Warning: Adjusting sizes to {} and {}!", s.0, s.1);
    }
    let m = gen(s);
    let mut buf = image::ImageBuffer::new(s.0 as u32, s.1 as u32);
    for (x,y,px) in buf.enumerate_pixels_mut() {
        *px = image::Luma([
            match m[x as usize][y as usize] {
                Cell::Free => 255,
                Cell::Blocked => 0,
            }
        ]);
    }
    let filename = if args.len() >= 4 { args[3].as_str() } else { "out.png" };
    let ref mut file = std::fs::File::create(&std::path::Path::new(filename)).unwrap();
    image::ImageLuma8(buf).save(file, image::PNG).unwrap();
}

Sample output: (It's quite tiny, you might have to download it and zoom in to see it properly.) enter image description here

(Sorry for any bad grammar, English is not my native language.)

\$\endgroup\$
2
\$\begingroup\$
  1. Learn to love rustfmt.

    1. Spaces go around operators and after commas

      -    let c = make_odd((s.0/2, s.1/2));
      +    let c = make_odd((s.0 / 2, s.1 / 2));
      
      -    for (x,y,px) in buf.enumerate_pixels_mut() {
      +    for (x, y, px) in buf.enumerate_pixels_mut() {
      
    2. Don't put multiple statements on one line

      -    o(&mut t.0); o(&mut t.1); t
      +    o(&mut t.0);
      +    o(&mut t.1);
      +    t
      
  2. Make a type for whatever (usize, usize) is. At the very least, create a type alias.

  3. Actually, there are three places that want to apply a change to both components of the tuple. Promote the tuple to a type and add a map method.

  4. If you have to give your closure arguments types anyway, and there's nothing captured by the closure, just make an inline function.

  5. There's no real reason to take mutable reference to increment an integral value.

  6. You define stack then immediately push a value on. instead, just use the vec! macro to create it all at once..

  7. There's no need to specify the type of dirs

  8. Instead of re-coding the number of elements in dir and having to potentially do bounds-checking again, just iterate over dirs directly

  9. Characters are free. Use some longer variable names.

  10. If you don't need the program name, skip it to avoid remembering it's there and then adding numbers here and there to compensate.

  11. There's no need for the turbofish when parsing the maze size.

  12. Use unwrap_or_else to handle a failure to parse.

  13. Use enumerate to avoid indexing the args slice again.

  14. Use std::cmp::max to provide a lower value for the sizes.

  15. Don't ignore errors. If writing to stderr is important, use expect.

  16. Use Vec::get instead of checking the size and then using the index operator (which checks the size again)

  17. You don't need to contruct a Path, just pass in the &str

  18. Prefer expect to unwrap.

  19. You can use ImageBuffer::from_fn. Note that this removes the need to make your variable mutable.

  20. Dont' use ref in let bindings. Using & on the right hand side is more idiomatic. In this case, &mut at call is more idiomatic.

  21. To deal with the unsigned / signed transformations, remember that a + (-2) is the same as a - (+2). Add an enum to define the directions, a method to apply those directions to a point, use checked_add / checked_sub in that method to deal with under/overflow, then add your own "overflow" check for the size of the maze.

extern crate rand;
extern crate image;

use rand::Rng;
use std::cmp::max;
use std::io::Write;

#[derive(Clone, PartialEq)]
enum Cell {
    Blocked,
    Free,
}

#[derive(Debug, Copy, Clone)]
enum Direction {
    Up,
    Down,
    Left,
    Right,
}

#[derive(Debug, Copy, Clone, PartialEq)]
struct Point(usize, usize);

impl Point {
    fn map<F>(self, mut f: F) -> Self
        where F: FnMut(usize) -> usize
    {
        Point(f(self.0), f(self.1))
    }

    fn move_by(self, amount: usize, dir: Direction) -> Option<Self> {
        use Direction::*;
        match dir {
            Left  => self.0.checked_sub(amount).map(|x| Point(x, self.1)),
            Right => self.0.checked_add(amount).map(|x| Point(x, self.1)),
            Up    => self.1.checked_sub(amount).map(|y| Point(self.0, y)),
            Down  => self.1.checked_add(amount).map(|y| Point(self.0, y)),
        }
    }
}

fn next_odd_number(n: usize) -> usize {
    n + if n % 2 == 0 { 1 } else { 0 }
}

fn generate_maze(size: Point) -> Vec<Vec<Cell>> {
    use Direction::*;

    let center = size.map(|v| v / 2).map(next_odd_number);
    let mut stack = vec![Point(center.0, center.1)];

    let mut maze = vec![vec![Cell::Blocked; size.1]; size.0];
    maze[center.0][center.1] = Cell::Free;

    let mut dirs = [Left, Right, Up, Down];
    let mut rng = rand::thread_rng();

    let ensure_in_bounds = |z: Point| {
        if z.0 >= size.0 || z.1 >= size.1 {
            None
        } else {
            Some(z)
        }
    };

    'next_odd_number: while let Some(&point) = stack.last() {
        rng.shuffle(&mut dirs);

        for &dir in &dirs {
            let new_point = match point.move_by(2, dir).and_then(&ensure_in_bounds) {
                Some(new_point) => new_point,
                None => continue,
            };

            if maze[new_point.0][new_point.1] != Cell::Free {
                stack.push(new_point);
                maze[new_point.0][new_point.1] = Cell::Free;
                let middle_point = point.move_by(1, dir)
                    .expect("Middle point cannot be out-of-bounds");
                maze[middle_point.0][middle_point.1] = Cell::Free;
                continue 'next_odd_number;
            }
        }
        stack.pop();
    }

    maze[0][1] = Cell::Free;
    maze[size.0 - 1][size.1 - 2] = Cell::Free;
    maze
}

fn print_usage() -> ! {
    writeln!(std::io::stderr(), "Usage: maze-gen width height [output.png]")
        .expect("Unable to write to stderr");
    std::process::exit(1);
}

fn main() {
    let args: Vec<_> = std::env::args().skip(1).collect();
    if args.len() < 2 {
        print_usage();
    }

    let mut dimensions = [0; 2];
    for (i, arg) in args.iter().enumerate() {
        dimensions[i] = arg.parse().unwrap_or_else(|_| print_usage());
    }
    let requested_size = Point(dimensions[0], dimensions[1]);
    let valid_size = requested_size.map(next_odd_number).map(|v| max(v, 3));

    if valid_size != requested_size {
        writeln!(std::io::stderr(),
                 "Warning: Adjusting sizes to {} and {}!",
                 valid_size.0,
                 valid_size.1)
            .expect("Unable to write to stderr");
    }

    let maze = generate_maze(valid_size);

    let buf = image::ImageBuffer::from_fn(valid_size.0 as u32, valid_size.1 as u32, |x, y| {
        let luma = match maze[x as usize][y as usize] {
            Cell::Free => 255,
            Cell::Blocked => 0,
        };
        image::Luma([luma])
    });
    let filename = args.get(2).map(String::as_str).unwrap_or("out.png");

    let mut file = std::fs::File::create(filename).expect("Couldn't open the file");
    image::ImageLuma8(buf).save(&mut file, image::PNG).expect("Coulding write the file");
}

I might next look into implementing a flat storage for the maze, one that can be directly indexed by a Point. That would allow this change:

-maze[middle_point.0][middle_point.1] = Cell::Free;
+maze[middle_point] = Cell::Free;

I'll be honest and say that I strongly dislike the usage of the labelled loop. I don't think I've ever needed them, and they are one of the rarest constructs in Rust, based on my experience. I'd probably spend a lot of time (more than I already did...) trying to rewrite that in a different manner.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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