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](http://doc.rust-lang.org/book/documentation.html), but I certainly wouldn't expect them in code at this level. ^_^ I do have some minor nits, of course! 1. There's no space between `impl` and `<`. `impl<T>`, not `impl <T>`. 2. 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. 3. The `vec!` macro idiomatically uses `[]`. `vec![]`, not `vec!()`. 4. There's no space between a generic type and `:` when defining constraints. `T: Clone`, not `T : Clone`. 5. I prefer to use `where` clauses for constraints. They read better and are more flexible to modification. 6. Not only can you say `r.mirror()` instead of `(*r).mirror()`, it's definitely preferred. Rust will [automatically dereference](https://stackoverflow.com/q/28519997/155423) for you. 6. 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 you 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`](http://doc.rust-lang.org/std/ops/trait.Deref.html): 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...