Lash me hard（Help me improve）

code review
1

In reference to A* Pathfinding for Beginners - Artificial Intelligence - Tutorials - GameDev.net，I programmed code below. As i'm a beginner, there must be some ugly parts and parts that i dont know how but became. Care to lash me hard?
Most cared:

  1. Why .as_ref().borrow() can access items within Rc<RefCell>?(Cuz as_ref() refers to where Rc<> points to? And borrow() just for the convenience that one can use the value without owneship taken?)
  2. About getting the Rc of a Node.parent::Option(Rc<RefCell>), in search_func() 's final path recording,(line:160),when i want to access cur_node's parent using match or unwrap(), new errors would occur in other place. Even though i 've solved "None"case by "match",it still occurs(eg, in function g ,the "e" becomes unkonwn type). don't know why
  3. other places that is redundant ,not fitting to rust coding rule, can be more simple, or anything you want to lash for
2

......forget to paste playground

3

playground

I've refactored your code a bit, removed Rc<RefCell<Node>>, but kept an indirection using indexies, you can take a look. Hopefully it functions like before, but there were no tests so I can't be sure.

I did not implement it, but here are additional notes:

  • You have map as Vec<Vec<T>>, but for matrices it is better to have a single Vec<T> and access index would be something like col * COL_COUNT + row. That way you have just one contiguous allocation.
  • If you cannot avoid shared mutable state, instead of wrapping the whole struct in RefCell, you can wrap fields that will change in Cell or RefCell, with all methods accepting &self. This is because in Rust & / &mut are about shared / unique access, not really about mutation, so it is perfectly fine to mutate struct in a &self method. Note that it would greatly decrease your ability to reason about the code and introduce subtle bugs like in JavaScript or C# that Rust is preventing (wrapping struct in RefCell as a whole is making it even more difficult to reason about, clatters the code and introduces more crashes).
  • Use rustfmt and clippy extensively
  • Write regression tests
  • In my implementation nodes are stored in vector and space in vector is not deallocated until function exits. If that behavior is not desirable and you want to deallocate it gradually (which is rarely what you want), you can use something like slotmap - Rust intead of a Vec
1 Like
4

Marvelous, your way of refering a parent node saved a lot of trouble,had i known it. Quite good notes, things would never occur to me.
Though, 'bout the 2nd note, i can accept that " in Rust & / &mut are about shared / unique access, not really about mutation".But i can't figure out the difference in using either RefCell or &self method since we do want mutate a variable. Is this for a good coding habit ?Or?
Nice notes :kissing_closed_eyes:

5

But i can't figure out the difference in using either RefCell or &self method since we do want mutate a variable.

struct Foo {
    a: String
}

impl Foo {
    fn mutate(&mut self) {
        self.a.push_str("hello");
    }
}

fn bar(foo: &RefCell<Foo>) {
   foo.borrow_mut().mutate();
}

vs

struct Foo {
    a: RefCell<String>,
}

impl Foo {
    fn mutate(&self) {
        self.a.borrow_mut().push_str("hello");
    }
}

fn bar(foo: &Foo) {
   foo.mutate();
}

Usually you are not passing control flow to other code inside your methods (like calling other functions, that may access RefCell while you are holding BorrowMut handle), while it is easy to do outside of them, so it may greatly reduce risk of double borrow runtime errors / crashes. Note that it should not be applied in every case, you as a programmer have a responsibility of figuring out, which way to use in your code.

1 Like
6

got it. Quite good habit. I met borrow problem while coding ,and your note hit it.

7

As you are a beginner, it is more beneficial for you to struggle with borrow checker, type system and lifetimes instead of using shortcuts like Rc, Cell/RefCell, thread_local, Box<dyn Trait>, Clone etc

1 Like
8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.