Overall, your code seems pretty spot-on. It was easy to read and understand. The most confusing to me was the mirror
method, as that's not a common method for trees in my experience. You may want to consider adding some doc-comments, but I certainly wouldn't expect them in code at this level. ^_^
I do have some minor nits, of course!
- There's no space between
impl
and <
. impl<T>
, not impl <T>
.
- For such a common and easily-understood function like
max
, I would go ahead and use
the function to allow using it unqualified. For less common functions, I'd use
the module, allowing you to skip typing the fully-qualified path.
- The
vec!
macro idiomatically uses []
. vec![]
, not vec!()
.
- There's no space between a generic type and
:
when defining constraints. T: Clone
, not T : Clone
.
- I prefer to use
where
clauses for constraints. They read better and are more flexible to modification.
- Not only can you say
r.mirror()
instead of (*r).mirror()
, it's definitely preferred. Rust will automatically dereference for you.
- Instead of using
allow(unused_variables)
, you can use a leading underscore _
to indicate that you are aware the variable is unused. You could also just not bind it to a variable at all. allow
is a big hammer, and has the possibility of hiding variables to meant to use.
Beyond the stylistic issues, there's one performance-related point. Your into_in_order
method will allocate N vectors, one for each node in the tree. That will cause a lot of memory churn. Instead, I'd recommend breaking it up into two functions: one that allocates a vector and another that traverses the tree. This way, you can minimize allocations. Of course, benchmarking would be a good idea, but I didn't do that! ^_^
It's also possible you might want to create an iterative version of this, as recursive code does have the possibility of hitting stack limits. Doing this would also allow you to write an Iterator
implementation that traverses in-order. Then your method basically becomes a collect()
. That might be fun to try next!
Now for your questions:
In a few places, I'm defining methods by passing self by reference and then matching on its dereferenced form. I then have to borrow the fields with ref
in the destructuring. Is this the intended form?
Yes, this is pretty typical.
Is into_in_order
using Vec
properly/optimally?
Ah, yes, good question. I think I touched on this above.
Am I using Box
es correctly?
Looks fine to me.
When should I use Box
es vs. * const
s?
You almost never want to use a raw pointer. The compiler cannot help you whatsoever with raw pointers and reading from them requires unsafe
code. These will generally come into play when you are writing FFI code or if you are writing the code that underlies a safe abstraction.
Why do (*r).mirror()
and r.mirror()
do the same thing? I would have expected the latter to throw because Box
es don't have a mirror
method.
This was touched on above, but this specific case works because Box
implements Deref
:
impl<T> Deref for Box<T>
where T: ?Sized
{
type Target = T
fn deref(&self) -> &T
}
Is there a less verbose alternative to the Branch(Box::new(Branch(…)))
syntax?
I was wondering the same thing as I read through your tests. They get pretty gnarly as they grow. I don't have any great suggestion, but I wonder if some kind of builder would help. Or perhaps some clever macro implementation that could reduce the visual clutter...