Imperartive or functional? Any improvements?

code review
1

I have a function I implemented in the imperative style #1:

pub fn get_attachment_dir() -> PathBuf {
    PathBuf::from(match env::var("ATTACH_DIR") {
        Ok(dir) if PathBuf::from(&dir).is_dir() => dir, 
        _ => match env::current_dir() {
            Ok(dir) => dir.into_os_string().into_string().unwrap(),
            _ => ".".to_string(),
        },
    })
}

What do not I like in the code? PathBuf is created twice. So I decided to switch to the functional style #2:

pub fn get_attachment_dir() -> PathBuf {
    env::var("ATTACH_DIR")
        .map(|dir| PathBuf::from(&dir))
        .and_then(|dir| {
            if dir.is_dir() {
                Ok(dir)
            } else {
                Err(VarError::NotPresent)
            }
        })
        .or_else(|_| env::current_dir())
        .or_else(|_| Ok::<PathBuf, VarError>(PathBuf::from(".")))
        .unwrap()
}

However, I do not like the artificial error, and using unwrap at the end. How would you write the function in more elegant and concise way?

2

Why not just unwrap_or_else at the end instead?

pub fn get_attachment_dir() -> PathBuf {
    env::var("ATTACH_DIR")
        .map(|dir| PathBuf::from(&dir))
        .and_then(|dir| {
            if dir.is_dir() {
                Ok(dir)
            } else {
                Err(VarError::NotPresent)
            }
        })
        .or_else(|_| env::current_dir())
        .unwrap_or_else(|_| PathBuf::from("."))
}
3

Awesome!

4 
pub fn get_attachment_dir() -> PathBuf {
    if let Some(dir) = env::var_os("ATTACH_DIR") {
        let dir = PathBuf::from(dir);
        if dir.is_dir() {
            return dir;
        }
    }
    
    env::current_dir().unwrap_or_else(|_| PathBuf::from("."))
}
  • var_os skips a potentially panicking UTF8 check
  • The conversion to PathBuf can be done on dir vs &dir
5

Or functional style.

pub fn get_attachment_dir() -> PathBuf {
    env::var_os("ATTACH_DIR")
        .map(PathBuf::from)
        .filter(|dir| dir.is_dir())
        .or_else(|| env::current_dir().ok())
        .unwrap_or_else(|| ".".into())
}
6

A returnless variant can be:

pub fn get_attachment_dir() -> PathBuf {
    if let Some(dir) = env::var_os("ATTACH_DIR")
        && let Ok(dir) = PathBuf::from(dir).canonicalize()
        && dir.is_dir()
    {
        dir
    } else {
        env::current_dir().unwrap_or_else(|_| PathBuf::from("."))
    }
}

However, I am not sure that canonicalize is a right operation, because it's expensive. Kotlin has something as also to perform some additional operations in row, but I didn't find an equivalent in Rust.

7

If "." is sufficient, you're presumably not changing working directories and don't need absolute paths, so you probably don't need canonicalize either.

8

I don't know kotlin, but looking at the documentation, I think for "wrapped" types, including Iterator, Result or Option, .inspect() should be the closest rust analog to also, but for a generic T, you'll have to use an external crate like tap.

9

Right, because I didn't know how to create PathBuf in if without a creation of some condition. Luckily,

the sentence gave me an idea, how I can create a condition using some wrapper, as an Option:

pub fn get_attachment_dir() -> PathBuf {
    if let Some(dir) = env::var_os("ATTACH_DIR")
        && let Some(dir) = Some(PathBuf::from(dir))
        && dir.is_dir()
    {
        dir
    } else {
        env::current_dir().unwrap_or_else(|_| PathBuf::from("."))
    }
}

I think we have reached the perfection, although a philosopher will say as it's unreachable.

10

I did also a small benchmark, and the imperative version is a bit faster, about 2%. So I left it in the production code. But I like more the functional version because it's more concise.

11

Irrefutable patterns are allowed in let chains.

 pub fn get_attachment_dir() -> PathBuf {
     if let Some(dir) = env::var_os("ATTACH_DIR")
-        && let Some(dir) = Some(PathBuf::from(dir))
+        && let dir = PathBuf::from(dir)
         && dir.is_dir()
     {
         dir
     } else {
         env::current_dir().unwrap_or_else(|_| PathBuf::from("."))
     }
 }
12

It's what I did from the beginning and got a compilation error. Maybe it's because --edition=2024 wasn't set. Thank you for the clarification.

13

you can mix and match (pun not intended), there's no single best style. e.g.

pub fn get_attachment_dir() -> PathBuf {
	match env::var_os("ATTACH_DIR").map(PathBuf::from) {
		Some(dir) if dir.is_dir() => dir,
		_ => env::current_dir().unwrap_or_else(|_err| PathBuf::from(".")),
	}
}
14

Clippy usually hits my hands when I tried to use match , although match based version was my first imperative try.

15

Question, if that is allowed... So why the env::current_dir().unwrap_or_else(|_| PathBuf::from(".")) when PathBuf::from(".") would skip a test and a step?

16

If you look in current_dir implementation:

pub fn current_dir() -> io::Result<PathBuf> {
    os_imp::getcwd()
}

It is a platform specific, for example, if you request current_dir in Linux service, then you will get Err, unless you used WorkingDirectory in the service manifest. So behavior for '.' isn't determined, especially on Windows, because there is no root for the file system. But I do not want to deal with errors, hoping that OS will do something with '.' or simply any file operation will fail, that it's good too.