Proper idiomatic way to write nested if let's (beginner qn)

[jda]

The exercise I set myself was to construct an index of files, like the unix find command. For now it just collects (filename,size,mtime) into a vector. (Later as a further exercise I'd like to e.g. dump as JSON, or put into a database, either mongo or mariadb or postgresql or all.)

I've come up with the following, which does what I want, but doesn't look to my eye like the right way to do this.

use std::fs;
use std::path::Path;
use std::time;
use walkdir::WalkDir;

#[derive(Debug)]
struct FileInfo {
    name: String,
    size: u64,
    mtime: time::SystemTime,
}

fn main() {
    let mut files_info: Vec<FileInfo> = Vec::new();
    for entry in WalkDir::new(".")
        .follow_links(false)
        .into_iter()
        .filter_map(|e| e.ok()) {
            let path = entry.path();
            if let Ok(metadata) = entry.metadata() {
                if ! entry.path_is_symlink() {
                    if let Ok(mtime) = metadata.modified() {
                        let size = metadata.len();
                        if let Some(name) = path.as_os_str().to_str() {
                            files_info.push( FileInfo { 
                                name: name.to_string(), 
                                size,
                                mtime } )
                        }
                    }
                }
            }
        }
                                
    for entry in files_info {
        println!("{:?}",entry);
    }
}

So questions:

1. What is the right way to avoid excessive nesting?
2. What is the right way to deal with both Result returning functions, and Option returning functions? That is, if I were to refactor this out of main, what should my function return? (In particular, I should factor the construction of files_info into its own function, and factor the logic inside the filter_map into a separate function. But I'm not sure the right way to do this so far as return types is concerned? I could probably return None if anything returns an error.)

[vague]

To flatten the nested if-lets, try using let-else
Rust Playground ^[1]

Update: if else { return } is too verbose, a macro can help Rust Playground

macro_rules! let_else {
    ($p:pat = $e:expr) => {
        let $p = $e else { return; };
    }
}

---

1. need a closure or function to early return though ↩︎

[H2CO3]

@jda

• What is the right way to deal with both Result returning functions, and Option returning functions? That is, if I were to refactor this out of main, what should my function return?

In general, if anything you call is fallible (returns Result), and you can't/don't want to handle errors, then your function should also return a Result. It is a bad idea to simply and unconditionally ignore them using .ok().

For handling fallibility, you can just make the closure in map return a Result<Option<FileInfo>> and propagate everything using ?, then use a separate pass of .filter_map(transpose) to turn the iterator over Result<Option<FileInfo>> into an iterator over Result<FileInfo>, like this. Early returns also help reduce nesting:

fn walk_dir<P: AsRef<Path>>(path: P) -> impl Iterator<Item = io::Result<FileInfo>> {
    WalkDir::new(path)
        .follow_links(false)
        .into_iter()
        .map(|entry| {
            let entry = entry?;
            let path = entry.path();
            let metadata = entry.metadata()?;

            if entry.path_is_symlink() {
                return Ok(None);
            }

            let mtime = metadata.modified()?;
            let size = metadata.len();
            let name = path
                .to_str()
                .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "non-UTF-8 path"))?;

            Ok(Some(FileInfo {
                name: name.to_string(),
                size,
                mtime,
            }))
        })
        .filter_map(Result::transpose)
}

[nerditation]

@jda

What is the right way to avoid excessive nesting?

as pointed out by @vague , early return with if else can be used.

@jda

What is the right way to deal with both Result returning functions, and Option returning functions?

there's no universal answer to this question, it all depends on context, but Result is more general than Option, since converting from Option to Result doesn't lose information while converting fromResult to Option discards information about the Err variant. besides, the caller can choose to ignore Err and convert a Result to an Option easily with result.ok(); and, you can attach extra context when returning Result using custom error type as the Err variant (e.g. anyhow::Context and snafu::ResultExt, snafu::OptionExt) but not when returning Option.

@jda

I could probably return None if anything returns an error.

this can be a perfectly valid solution, if the the intention is only to distinguish successful call from failed call.

also, as a side note, main function can return Result. typically you'll find people use Box<dyn Error + 'static + 'Send> as the Err variant, or anyhow::Error and the like. see documentation for the Termination trait for technical details:

Termination in std::process - Rust

[Spindel]

So, I recently went and did this exercise while learning the iterators, so the timing coincidence is a bit striking for me.

Thus, my first thing to do would be to invert some of your logic. the "path" part isn't used until nearly in the end of the logic, so move it there. This doesn't flatten anything, but makes it more visible.

Next up is to move the pure boolean checks out of the fallible one, so move "entry.is_path_symlink()" test before the "metadata". And then one step further, by making it part of the iterator, as a .filter(|e| !e.path_is_symlink())

Next, since we don't do anything with the error cases of metadata, lets move that up as well, adding another filter_map statement

.filter_map(|e| e.metadata().ok().map(|meta| (e, meta)))
This one is a bit obtuse, but e.metadata().ok() turns Result into Option, and .map(|meta| (e, meta) then inserts the pair (e, meta) into the Some of the Option, Thus, if e.metadata() is Ok, we now have a pair (e, metadata) otherwise a None, which gets filtered away by "filter_map".

Then, lets repeat that again, this time, for the mtime of the metadata.
.filter_map(|(e, meta)| meta.modified().ok().map(|mtime| (e, meta, mtime)))

Exactly the same pattern, but this time our iterator will be ( DirEntry, Metadata, Mtime), and anything that mtime failed for, will be discarded.

And since we're already in functional-lala-I-cant-hear-you-laa-la-land, lets make the length and filename parts of it.

.map(|(e, meta, mtime)| (e, meta.len(), mtime))

This is a simple map, turning it from (DirEntry, Metadata, Mtime) to (DirEntry, size, mtime), discarding the metadata.

And finally, filenames:
.map(|(e, size, mtime)| (e.into_path(), size, mtime))

This is different from your ".path()" as the e.into_path() gets an owned reference and consumes the DirEntry, thus making our final iterator:

(PathBuf, size, mtime)

Note that from a code standpoint, the map that replaces meta with size is unnecessary and can be folded into the line that gets the mtime, but that would make this even more obtuse.

Final result:

use std::fs;
use std::path::Path;
use std::time;
use walkdir::WalkDir;

#[derive(Debug)]
struct FileInfo {
    name: String,
    size: u64,
    mtime: time::SystemTime,
}

fn main() { 
    let mut files_info: Vec<FileInfo> = Vec::new();
    for (path, size, mtime) in WalkDir::new(".")
        .follow_links(false)
        .into_iter()
        .filter_map(|e| e.ok())
        .filter(|e| !e.path_is_symlink())
        .filter_map(|e| e.metadata().ok().map(|meta| (e, meta)))
        .filter_map(|(e, meta)| meta.modified().ok().map(|mtime| (e, meta, mtime)))
        .map(|(e, meta, mtime)| (e, meta.len(), mtime))
        .map(|(e, size, mtime)| (e.into_path(), size, mtime))
        {
            files_info.push( FileInfo {
                name: path.display().to_string(),
                size, 
                mtime } )
        }

    for entry in files_info {
        println!("{:?}",entry);
    }
}