Day 2 of Rust. How close am I to idiomatic?

As an exercise, I wrote a little graph data structure with undo/redo that would be suitable for my app (hence the Node/Wire/Patch terminology instead of Node/Edge/Graph).

I saw this: https://rust-leipzig.github.io/architecture/2016/12/20/idiomatic-trees-in-rust/, which makes me think it isn’t particularly practical to maintain a compact array of Nodes (client code might want some identifier for a Node which is stable under graph edits), but that doesn’t seem like a style thing per se.

Just a few things that I’ve caught (I’m not going to analyze your algorithm for max efficiency though) - Just going to look for idiomatics

  1. Use usize and isize instead of types with predetermined size. This is because we can then index properly, and prove that the maximum index is actually achievable.
  2. Think about adding a new function to your types to avoid explicit instantiation.
let mut patch = Patch{ nodes: Vec::new(), wires: Vec::new() };
//becomes the following:
let mut patch = Patch::new();
//or the following if you also implement `Default`
let mut patch = Patch::default();
  1. Please, please, please, use the dyn keyword where necessary to annotate trait objects. For example, Box<Fn()> becomes Box<dyn Fn()>. Add a #![deny(bare_trait_objects] to the top of your main file (lib.rs or main.rs) to turn this into a hard error throughout the module (Which includes the rest of your project).
  2. Use rustfmt please. Run with cargo fmt
  3. And, one last nitpick, just as how fn foo() automagically becomes fn foo() -> (), dyn Fn() also becomes dyn Fn() -> (), so using that, we can change your Box<dyn Fn() -> ()> into Box<dyn Fn()>.
  4. Rust has an amazing testing suite. You can either use something like the following:
//mod.rs or main.rs
/*
the rest of your code
*/

fn main() {
    //Your main code, omitting assertions for correctness
}

#[cfg(test)]
mod tests {
    #[test]
    fn it_works() {
        assert!(MyPublicAndOrPrivateApi::new().works());
    }
}

//In <Cargo.toml directory>/tests/filename.rs
#[test]
fn it_works() {
    assert!(MyPublicApi::new().works());
}
  1. Another bit of random advice: If you don’t want to allow your users to have state within their closures (Currently your dyn Fn() objects) you can use a bare function pointer fn() (Note capitalization) which could possibly make your code faster, but this is a small nitpick in the eyes of usability.
  2. You can make your Fn calls look nicer:
//Change this, like on line 91
(*top)(patch, self); 
//To 
top(patch, self);
  1. You don’t need the extra curly braces {} around your for loops from line 54 to 72.

Otherwise, you may want to run clippy to get some more details, but otherwise this is looking great!

P.S. Welcome to the Rust users forum!

6 Likes

Here’s a fork of your gist which implements most of the suggestions above.

1 Like
self.wires.retain(|w| w.from != from || w.to != to);

I’d write:

self.wires.retain(|w| (w.from, w.to) != (from, to));

// or alternatively, because Wire: Copy:

self.wires.retain(|&w| w != Wire { from, to });

for w in &self.wires {
  if w.from == node || w.to == node {
    to_disconnect.push(Wire{from: w.from, to: w.to});
  }
}

I’d write:

for w in &self.wires {
  if w.from == node || w.to == node {
    to_disconnect.push(w.clone());
  }
}

// or, because Wire: Copy

for &w in &self.wires {
  if w.from == node || w.to == node {
    to_disconnect.push(w);
  }
}

Random aside: I don’t think you ever clear your redo buffer?


Eh. I often see usize taken as an easy way out of deciding an integer type. It’s pretty hard to come across integers that couldn’t potentially be used as an index, outside of hashes and cryptography; but not all of these things that we’re indexing are able to grow to such large proportions.

I say it’s a good suggestion if the author didn’t consider it, but u32 is not necessary unidiomatic or something to be discouraged. (Just something to be used properly)

Yes, all of these.


which makes me think it isn’t particularly practical to maintain a compact array of Nodes (client code might want some identifier for a Node which is stable under graph edits), but that doesn’t seem like a style thing per se.

Yeah, that’s an entirely different matter from being idiomatic. As I see it, a stable graph and an unstable graph are different beasts. Both have their strengths and weaknesses, and neither is perfect for every situation. Use what’s most useful to you!

5 Likes

Thanks so much @OptimisticPeach and @ExpHP for the excellent replies!

Would the decision of u32 vs usize be made more clear by needing to serialize the graph?

1 Like

Yes, unless you were serializing to a format where the types of things aren’t needed (eg JSON) because then, it’s just text and not a size thing. Either way, usize or u32 should work unless you have more than 2^32 - 1 elements.