Review of dvshapkin/toolbox

review.md

Review of dvshapkin/toolbox

Context: https://users.rust-lang.org/t/please-make-a-code-review/28907
Source Code: https://github.com/dvshapkin/toolbox/tree/1de0ecc5596811bbc0a725f7773584e8ae2cf099

Constructor

Constructors are usually on a struct and not free standing functions.

This:

pub fn new<P: AsRef<Path>>(root: P) -> Option<VirtualFileSystem> {
    // ...
}

Source

Can be rewritten as:

impl VirtualFileSystem {
    pub fn new<P: AsRef<Path>>(root: P) -> Option<Self> {
        // ...
    }
}

As the constructor returns an Option it might make sense to highlight that, by renaming the constructor to try_new.

This also clarifies the invokation of the constructor:

Before:

let vfs = new(/* ... */).expect("Root directory not found");

After:

let vfs = VirtualFileSystem::try_new(/* ... */).expect("Root directory not found");

Results and Options

Results and Options have a lot of useful combinators available, so matching can often be replaced by a suitable combinator.

I

The following match expression can be simplified using Result::map and Result::unwrap_or.

This:

match self.absolute(path) {
    Ok(path) => path.exists(),
    Err(_) => false,
}

Source

Can be rewritten as:

self.absolute(path)
    .map(|path| path.exists())
    .unwrap_or(false)

Since bools implement the Default trait this can be simplified further using Result::unwrap_or_default:

self.absolute(path)
    .map(|path| path.exists())
    .unwrap_or_default()

II

Results and Options can be converted to each other. For example with Result::ok, which turns Ok into Some and Err into None.

So this:

match Path::new(root.as_ref()).canonicalize() {
    Ok(path) => Some(VirtualFileSystem { root: path }),
    _ => None,
}

Source

Can be rewritten as:

Path::new(root.as_ref())
    .canonicalize()
    .map(|root| VirtualFileSystem { root })
    .ok()

Code duplication

The following code is used a few times across the file and could be extracted into an internal function:

if path.as_ref().is_absolute() {
    do_something(self.root.join(self.relative(path)?))
} else {
    do_something(self.root.join(path.as_ref()))
}

Source

Example:
(The naming of my example function can surely be improved 😁)

fn convert_to_absolute_path(&self, path: &Path) -> Result<PathBuf> {
    let relative_path = if path.is_absolute() {
        self.relative(path)?
    } else {
        path
    };
    Ok(self.root.join(relative_path))
}

Errors

The errors defined in errors.rs all hold a String containing the error message assembled in the constructor. Instead of allocating Strings one could move the string formatting into the Display impl.

Note: The write! macro supports the same syntax for formatting as the format! macro.

This:

pub struct NotAbsolutePathError {
    pub message: String,
}

impl NotAbsolutePathError {
    pub fn new() -> NotAbsolutePathError {
        NotAbsolutePathError {
            message: "Argument is not absolute path.".to_string(),
        }
    }
}

impl Display for NotAbsolutePathError {
    fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
        write!(f, "{}", &self.message)
    }
}

Source

Can be rewritten as:

pub struct NotRelativePathError;

impl NotRelativePathError {
    pub fn new() -> Self {
        Self
    }
}

impl Display for NotRelativePathError {
    fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
        write!(f, "Argument is not absolute path")
    }
}

Self

Self can be used in impl blocks instead of the struct's name.

So this:

impl PathNotBelongsError {
    pub fn new(path: &Path) -> PathNotBelongsError {
        PathNotBelongsError {
            // ...
        }
    }
}

Source

Can be rewritten as:

impl PathNotBelongsError {
    pub fn new(path: &Path) -> Self {
        Self {
            // ...
        }
    }
}

Comments

//////////////////////////////////////////////////////////////////////

Source

Visual separator comments are usually not a good idea, since they just add overall noise. Split large files into smaller modules instead.

Visibility

A lot of items are using pub when they do not necessarily need to be public. There are more restrictive modifiers like pub(crate) that only makes something accessible from within the current crate, pub(super) which makes an item only visible from the parent module, etc.

See Visibility in the "Rust by Example" guide.