Absolutely new in Rust

code review
1

Hi !
As the title says, I'm a pure novice in Rust, and in this kind of programming language in general with strict types and compiling process.
I wanted to learn something like C or derivative, but I remembered about Rust, and after some readings I thought it could be a good choice.
So I started from scratch, with a medium project about creating mazes or labyrinths. But even if I conscientiously started to read the book, I think I need some feedback to know if I'm in the good way.

After some reading, I've wrote some basic stuff, but I need some kind of an approval I'm in the good direction : does what I wrote is a good way to design in Rust ? Does it is a more efficient way ? If so, could you explain me ?

enum CardinalPoint {
	North,
	East,
	South,
	West,
}

#[derive (Debug)]
struct Cell {
	north_wall: Wall,
	east_wall: Wall,
	south_wall: Wall,
	west_wall: Wall,
}

#[derive (Debug, Default)]
struct Wall {
	built: bool,
	breakable: bool,
}

impl Cell {
	fn new () -> Self {
		Cell {
			north_wall: Wall { built: false, ..Default::default() },
			east_wall: Wall { built: false, ..Default::default() },
			south_wall: Wall { built: false, ..Default::default() },
			west_wall: Wall { built: false, ..Default::default() },
		}
	}
	fn build_wall (mut self, c: Option<CardinalPoint>) -> Self {
		match c {
			None => return self,
			Some(CardinalPoint::North) => { self.north_wall.built = true },
			Some(CardinalPoint::East) => { self.east_wall.built = true },
			Some(CardinalPoint::South) => { self.south_wall.built = true },
			Some(CardinalPoint::West) => { self.west_wall.built = true },
		}
		self
	}
	fn has_wall (&self, c: CardinalPoint) -> bool {
		match c {
			CardinalPoint::North => self.north_wall.built,
			CardinalPoint::East => self.east_wall.built,
			CardinalPoint::South => self.south_wall.built,
			CardinalPoint::West => self.west_wall.built,
		}
	}
	fn is_wall_breakable (&self, c: CardinalPoint) -> bool {
		match c {
			CardinalPoint::North => self.north_wall.breakable,
			CardinalPoint::East => self.east_wall.breakable,
			CardinalPoint::South => self.south_wall.breakable,
			CardinalPoint::West => self.west_wall.breakable,
		}
	}
	fn is_dead_end (&self) -> bool {
		let walls = [self.north_wall.built, self.east_wall.built, self.south_wall.built, self.west_wall.built]
			.iter().filter(|&&x| !x).count();
		println!("There is {} walls built", walls);
		walls < 2
	}
}

fn main () {
	let mut cell_1 = Cell::new()
					.build_wall(None)
					.build_wall(Some(CardinalPoint::East))
					.build_wall(Some(CardinalPoint::South))
					.build_wall(Some(CardinalPoint::West));
	cell_1 = cell_1.build_wall(Some(CardinalPoint::North));

	println!("Cell Object --> {:?}", cell_1);
	println!("North Wall is built ? --> {:?}", cell_1.has_wall(CardinalPoint::North));
	println!("East Wall is beakable ? --> {:?}", cell_1.is_wall_breakable(CardinalPoint::South));
	println!("Dead End --> {:?}", cell_1.is_dead_end());
}
2

I don't really have a problem with any of the Rust here, but it's a bit hard to tell what you're going for from a general programming perspective.

I see you have a Cell object, and a Cell has 4 walls. Presumably these cells are going to be organized in a two dimensional grid. So my concern is what happens when you have a Cell with a north wall "built", but the Cell "north" of it does not have the south wall built.

You will have to maintain this invariant somehow, unless you plan to have your walls unidirectional. Which could be a thing I suppose.

3

Generally, a first good step to take is to run cargo fmt and cargo clippy on your project, the latter possibly with pedantic and nursery settings enabled.

Some observations from my side.

This looks like a good candidate for an implementation of Default:

	fn new () -> Self {
		Cell {
			north_wall: Wall { built: false, ..Default::default() },
			east_wall: Wall { built: false, ..Default::default() },
			south_wall: Wall { built: false, ..Default::default() },
			west_wall: Wall { built: false, ..Default::default() },
		}
	}

You derived it for other types, so you can do that here, too, since Wall implements Default and bool's default is false.

Here an Option seems pointless, since it doesn't do anything in this builder-style invocation:


	fn build_wall (mut self, c: Option<CardinalPoint>) -> Self {
		match c {
			None => return self,
			Some(CardinalPoint::North) => { self.north_wall.built = true },
			Some(CardinalPoint::East) => { self.east_wall.built = true },
			Some(CardinalPoint::South) => { self.south_wall.built = true },
			Some(CardinalPoint::West) => { self.west_wall.built = true },
		}
		self
	}

Consider this instead:

    fn build_wall(mut self, c: CardinalPoint) -> Self {
        match c {
            CardinalPoint::North => self.north_wall.built = true,
            CardinalPoint::East => self.east_wall.built = true,
            CardinalPoint::South => self.south_wall.built = true,
            CardinalPoint::West => self.west_wall.built = true,
        }
        self
    }

This looks like a bug in the logic:

	fn is_dead_end (&self) -> bool {
		let walls = [self.north_wall.built, self.east_wall.built, self.south_wall.built, self.west_wall.built]
			.iter().filter(|&&x| !x).count();
		println!("There is {} walls built", walls);
		walls < 2
	}

You print the amount of walls that do not have the built flag set.
Consider this instead:

    fn is_dead_end(&self) -> bool {
        let built_walls = [
            &self.north_wall,
            &self.east_wall,
            &self.south_wall,
            &self.west_wall,
        ]
        .iter()
        .filter(|wall| wall.built)
        .count();
        println!("There are {built_walls} walls built.");
        built_walls > 2
    }

Also consider documenting your code (i.e. types and functions) to explain to the reader what they're supposed to represent and/or do.

4

To be honest, I don't know yet. Learning Rust is also the occasion to learn new algorithms too :slight_smile: So I try and retry while reading the book, and at the same time I'm reading about Recursive Backtracker, Unicursal mazes and so on !
So for now, I don't really know if getting Cells with 4 walls is a good design. To be honest, I was thinking about discovering a path, and along the path creating Cells with corresponding walls and store them in a 2D array (or Vec ? or HashMap ?). That's why I thought of erecting walls, or destroying them (the breakable flag is for something else in my mind, I plan to have walls erected but breakable in the future for an exploration, I was just testing with the concept I just learned with the book).
Thank you for your feedback !

5

Thank you for your feedback.
To be honest, using Option was because I was at the chapter about Option in the book :slight_smile: and it was helpful because it helped me to understand the need to return self for None. But you're right, there is probably no need for it. In the first version it wasn't there, exactly as you corrected in your answer.

For the dead_end function, you're also right, it's a pure logic flaw ! And it is more readable your way !

The more annoying thing to me was the necessity to write again and again the same self.cardinalpointstuff.flag. It seems redundent. So recently I was thinking of storing these in an array or equivalent, to assign a 1..4 value to the CardinalPoints enum and define the flag this way or so :

fn build_wall(mut self, c: CardinalPoint) -> Self {
    self.walls[c].built = true;
    self
}
6

Don't recommend nursery to beginners. It is the category for lints that are known to be flawed or otherwise needing further work. Beginners are more likely to be misled by incorrect diagnostics.

5 Likes
7

once you get to traits and operator overloading, i would recommend implementing

impl Index(Mut)<CardinalPoint> for Cell { type Output = CardinalPoint; ... }

to lighten the boilerplate

although you can already just do

impl Cell { 
     fn wall_at(&self, dir : CardinalPoint) -> &Wall {  ... }
     fn wall_at_mut(&mut self, dir : CardinalPoint) -> &mut Wall {  ... }
}

which some might argue is better

8

Starting from the updated version of build suggested above:

fn build_wall(mut self, c: CardinalPoint) -> Self {
        match c {
            CardinalPoint::North => self.north_wall.built = true,
            CardinalPoint::East => self.east_wall.built = true,
            CardinalPoint::South => self.south_wall.built = true,
            CardinalPoint::West => self.west_wall.built = true,
        }
        self
    }

The name build_wall to me implies modifying an existing Cell, but your version takes one by value and returns another one. I understand that you use this for the builder pattern, so I would propose to:

  1. rename the existing build_wall to with_wall or maybe with_wall_built
  2. (in case you still need the mutating version) create a new build_wall and define it as follows:
    fn build_wall(&mut self, c: CardinalPoint) {
    match c {
        CardinalPoint::North => self.north_wall.built = true,
        CardinalPoint::East => self.east_wall.built = true,
        CardinalPoint::South => self.south_wall.built = true,
        CardinalPoint::West => self.west_wall.built = true,
    }
}
    or, if you implement overloaded indexing/.wall_at_mut(), use those to simplify it even further:
    fn build_wall(&mut self, c: CardinalPoint) {
   self.wall_at_mut(c).built = true;
}
    .. at which point you might consider getting rid of it altogether and using self.wall_at_mut(c).built = true; directly
9

Neither should beginners run Clippy with pedantic. Those lints are disabled by default for a reason, they are more likely to cause warning fatigue or worse code than do any good.

1 Like
10

It isn't really clear what you're trying to do. That's a toy code example, so it's hard to give any useful suggestions. That said, there are some organizational improvements possible.

  • Your current API allows to leave walls unbuilt, or to build the same wall several times. Is that really what you want? What should the behaviour be in those cases? Having an unbuilt wall probably makes sense, but building a wall twice does not. The simplest solution to that problem is to use assert! inside your building functions, which would panic if the wall is already built. Panics make sense here, since a double-built wall is likely a bug in the consuming code.

  • This struct definition implies that a wall can be breakable, but not built:

struct Wall {
	built: bool,
	breakable: bool,
}

I don't think that makes sense. If there is no wall, it can't have parameters. Instead, the Rust Way is to make invalid states unrepresentable. This means that the Wall struct should only contain properties of a valid built wall (in your example, only breakable), while the Cell should instead consist of Option<Wall> values. This way, the compiler will check for you that you don't confuse built and unbuilt walls, and that you can only set properties on a built wall.

This definition is very complex. You consider all walls as separate fields, even though they should all behave in the same way. Also, if you later want to add extra wall types (e.g. south_east), you'd have a lot of stucture updates to do. Instead, I suggest you represent the walls of a cell as a simple array.

// Note the derives: for a simple data type you are _extremely_ likely to
// end up needing those traits anyway. It's generally best to derive all
// that make sense (so at least Debug and Clone should almost always be derived).
// You'll be also saving yourself from some nasty compiler errors when you try
// to put your type in a HashMap or BTreeMap.

#[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash)]
enum CardinalPoint {
    // Setting a value here allows to use `as`-casts on the enum.
    // The other variants will get subsequent tags.
    North = 0,
    East,
    South,
    West,
}

// I'm not deriving other traits here, since I don't know whether you plan
// to add other fields to `Cell` which would make those traits non-sensical.
// Removing traits which are depended on by downstream code can be painful.
// But I still derive `Default`, since it's convenient and unlikely to be
// heavily depended upon.

#[derive(Debug, Clone, Default)]
struct Wall {
	breakable: bool,
}

#[derive(Debug, Clone, Default)]
struct Cell {
    // We'd like to compute the number of elements directly from the enum's
    // definition, but current stable Rust doesn't allow it. So, we need to
    // write the proper value manually and write tests.
    walls: [Option<Wall>; 4],
}

impl Cell {
    fn new() -> Self {
        Cell::default()
    }

    // The following two accessor functions will fully encapsulate
    // the implementation details of out wall representation (array, or
    // struct fields, or any other representation), and will significantly
    // simplify further code.
    // If we change the details of representaion (e.g. add more directions, or
    // use `HashMap<CardinalPoint, Wall>` instead of simple array), these are
    // the only functions which we'd have to change.
    fn wall(&self, dir: CardinalPoint) -> &Option<Wall> {
        &self.walls[dir as usize]
    }

    fn wall_mut(&mut self, dir: CardinalPoint) -> &mut Option<Wall> {
        &mut self.walls[dir as usize]
    }

    // The `&mut self` receiver is more convenient if you want to use that
    // method as a stand-alone call. Also, it doesn't make sense to pass
    // a `None` as `CardinalPoint`, you could just as well omit the method
    // call entirely, and the API becomes simpler. And pass the new `Wall`
    // explicitly, thank me later.
    fn build_wall(&mut self, dir: CardinalPoint, wall: Wall) -> &mut Self {
        let place = self.wall_mut(dir);
        assert!(place.is_none());
        *place = Some(wall);
        self
    }

    fn has_wall(&self, c: CardinalPoint) -> bool {
        self.wall(c).is_some()
    }

    fn is_wall_breakable(&self, c: CardinalPoint) -> bool {
        self.wall(c)
            .as_ref()
            .unwrap_or_else(|| panic!("no wall built at {c:?}"))
            .breakable
    }
    
    fn is_dead_end(&self) -> bool {
        let walls = self.walls.iter().filter(|w| w.is_none()).count();
        println!("There is {} walls built", walls);
        walls < 2
    }
}
11

You're totally right. Because I was only trying some things along the reading of the book, the logic ended to be not so logical.
I learned and tried your code rewrite, and to test it, I had to do it like this right ?

fn main () {
   let mut bind = Cell::new();
   let cell = bind.build_wall(CardinalPoint::North, Wall[breakable:false})
                  .build_wall(CardinalPoint::East, Wall{breakable:true});
   println!("Cell Object --> {:?}", cell);
   println!("North Wall is built ? --> {:?}", cell.has_wall(CardinalPoint::North));
}

With your changes it seems to be the only way to use the newly rewritten methods, because I can't no more chain the methods directly from the initial Cell::new(). But I have to admit, I don't really understand why, because it still returns self ?..

Following your advises, doesn't it better to write the following for the dead_end method ?

fn is_dead_end(&self) -> bool {
   /*let walls = self.walls.iter().filter(|w| w.is_none()).count();*/
   let walls = self.walls.iter().filter(|w| w.is_some()).count();
   println!("There is {} walls built", walls);
   /*walls < 2 */
   walls > 2
}

Otherwise, I have to admit that is a thing I don't really understand, is the absolute need of the derive Clone. Because we use references & inside the methods, why do we have to use Clone on the structs ?

And thank you for the time you took to rewrite the code and to explain it !
My goal is to be able to generate mazes and store the data in binary format. Then use the binary datas from a maze that please me to work with (probably a small and simple idle game).

Have a nice day !