I have a relatively extensive programming background, but I am new to Rust.
In recent days, I have written a library and a program for Nav Graph pathfinding with A*.
This program compiles without warning, and Clippy does not report one either:
My questions are now:
Does the Code in any part deviate from common Rust programming best practices, and if so, where?
Do you see any parts that are very inefficient concerning the Rust programming style?
Besides the core A* implementation, the library also contains a lot of stuff to make an interactive application of it, to demonstrate and test the functionality. If something like this were submitted to crates.io, would it be advisable to keep the interaction test/demonstration part, or would it be preferred to separate this and only submit the core part with the unit tests?
Thanks for any info on these subject matters,
Christoph
This review is focused on matters of making a good library as opposed to algorithms and efficiency.
In your Cargo.toml, you are using path configuration. Don't do this unless you have a specific reason for it. Instead, place your source files in the standard locations where target auto-discovery will find them.
Don't name public modules vague things like math_helper. A good name for that module might be something like vector instead. (Or, keep the module private; see the next point.)
This is a matter of API design “taste” more than correct Rust style, but I recommend making use of the ability to make modules private and export only their contents. In a small, single-purpose library like this, the items related to the purpose of the library should be at the top level so users can refer to them concisely.
mod a_star;
mod math_helper;
pub use a_star::{NodeState, NavGraph};
pub use math_helper::{Vec2, Line};
Regardless of whether you like this design principle, run cargo doc --open and review the documentation pages of your library; then, make changes so that it looks good from that view — not just whether it is well-organized, but also so the documentation makes sense when viewing only the documentation and not the code.
Speaking of documentation looking good, documentation comments for modules and items in modules should have their first paragraph be concise and clear, so that it shows up well in the item list in the containing module. Don’t write boilerplate phrases like “This module contains”:
//! This module contains the core a star algorithm
Instead write:
//! The A\* algorithm.
When documenting things, try to avoid only reiterating the name of the item, and instead explain what that name means. For example,
pub enum NodeState {
/// The node has been closed.
Closed,
This documentation provides no value. It should be replaced with an explanation of what a node being “closed” means. In the rare situation where the meaning is completely obvious to anyone who could possibly be using this library, then it is better to have no documentation than documentation that simply repeats the name of the item.
NavNode::reset() has no documentation. Maybe this method should not be public?
NavGraph::reset_graph_search() doesn't look like it needs to be public, because search_graph() calls it. If it does need to be public, the documentation should explain when it needs to be called.
You should consider keeping the Vec2 type private, and making your public API be in terms of [f32; 2] instead. This has two advantages:
It is easier for your users to convert from/to their own vectors, because most vector types implement conversions from/to arrays.
When you, for your own library’s purposes, have reason to change or remove a method from Vec2, then if the type is private, this is not a breaking change requiring publishing a new major version.
Since you are writing a pure algorithm that doesn't need to interact with the operating system, consider making your library no_std compatible. This means that it can be used in a much wider range of environments — for example, pathfinding in a game written for a console that has no std available for it. no_std is not that hard to do for this kind of code:
If tests need std (if you get errors), conditionally add it back:
#[cfg(test)]
#[macro_use]
extern crate std;
It is entirely normal to have example code in your package, but you should make sure that the library can be used without compiling the example’s dependencies. Instead,
The interactive application should be changed from the bin target type to the example target type. This means placing its crate root file at examples/astar_test/main.rs instead of src/bin.rd. Modules for the interactive application should go next to that file (your graph_constructor and graphics modules should be moved there).
The dependencies used by it (which appear to be all of your library’s dependencies) should be listed under [dev-dependencies], not [dependencies]. This allows them to be automatically ignored when using the library.
It's not critical that the library contains absolutely no related to the interactive application, but it should be kept to a minimum. Rust users value fast compile times, so you should avoid the library compiling code that will never be needed when using it as a library. (For example, your Vec2::get_combined_as_array() is a reasonable kind of thing to have for the sake of the example code.)
I think I have addressed all the issues and tips mentioned. Along the way, I stumbled upon some problems. The NavGraph now uses [f32;2] in its interfaces, so the struct Vec2 can be made private. The problem is now that I use Vec2 in the example. After digging a little into the issue, I found that some projects solve this by excluding this from the documentation:
#![no_std]
pub mod a_star;
#[doc(hidden)]
pub mod vector;
pub use a_star::{NodeState, NavGraph};
Using the no_std, brought up the issue with the operators and with the Vec.
For the Vec issue I also had to add an extern declaration:
extern crate alloc;
use alloc::vec::Vec;
Now the bizarre issue is that the sqrt function does not work anymore with the no_std library. The reason is that this function is platform-specific and optimized based on the assembly instruction set of the processor. I solved this now by including the libm library, which also contains a square root function. Am I correct in the assumption that this is the lesser evil?
And when none are enabled, the compilation should fail with a useful error message (or, if it was possible, it will remove the functions that are using math)
the compilation failed. As I was advised not to use std by kpreid I opted for libm. I think the idea of not using std was to reduce the dependency on external libraries and therefore compilation time. The question now is, if libm is better than std lib. in this respect.
Not at all. Not using std does not reduce compilation time, because std has already been compiled. As I said, the purpose of no_std is to make your library compatible with situations where std is not available at all.
I would suggest skipping it for now due to the sqrt problem. It can be a future goal, or not done at all — I was just suggesting it because it might be easy.