1

The following code removes the _ character from png files in a folder:

use std::fs;
use std::path::Path;

fn main() {
    let dir = Path::new("/home/alex/Desktop");
    for entry in fs::read_dir(dir).unwrap() {
        let entry = entry.unwrap();
        let path = entry.path();
        if path.is_file() && path.extension().unwrap() == "png" {
            let new_path = path.with_file_name(path.file_name().unwrap().to_str().unwrap().replace("_",""));
            fs::rename(path, new_path).unwrap();
        }
    }
}

As you can see unwrap() is used a lot. It is possible to remove them in this code, and use a cleaner approach?

alexchenco
  • 53,565
  • 76
  • 241
  • 413

1 Answers1

6

You're using unwrap for several different things here. Let's break them down.

fs::read_dir(dir).unwrap()

read_dir can fail if an IO error occurs. That's not something under your control, and it's not something you can deal with. Using the excellent vexing exceptions analogy, this error would be an exogenous one: not your fault and not something you can prevent. unwrap makes sense here. In a larger program, we might let our function return io::Result<_> and could write fs::read_dir(dir)? to let the caller try to recover from the error. But for a small main-only program, unwrap makes sense here.

let entry = entry.unwrap();

Same thing. It's an IO error out of your hands. In a larger program, you would write entry? to propagate the error to the caller, but here on this small scale, unwrap is fine.

path.extension().unwrap()

Here's where things get interesting. extension doesn't fail. It returns None in the completely normal, reasonable situation where the file doesn't have an extension. For instance, if the file is named Rakefile or .gitignore. Panicking in this case is really unfortunate. Instead, we simply want the if statement to fail. What your if statement says right now is "assert that the extension exists, and do something if it's png". What you really want is to say "if the extension exists and is png". No assertion necessary. Consider

if let Some(extension) = path.extension() {
  if extension == "png" {
    ...
  }
}

In future versions of Rust, it will be possible to write if let in conjunction with &&, so we'll be able to shorten this to

if let Some(extension) = path.extension() && extension == "png" {
  ...
}

But that feature is unstable right now.

Moving on, I'm skipping over the line with several unwrap calls right now. We'll come back to that in a minute.

fs::rename(path, new_path).unwrap();

fs::rename is an IO operation and can fail like any IO operation can. Let it fail, or propagate in case of a containing function, just like the first two.

Now let's talk about the last line.

path.with_file_name(path.file_name().unwrap().to_str().unwrap().replace("_",""));

file_name() returns None if there's no filename. In that case, we shouldn't even be trying to rename the file, so that should be something we check in an if let before we get here.

if let Some(filename) = path.file_name() {
  ...
}

Next, you're using to_str. The reason you need to do this is that filenames use OsStr, which may or may not be valid UTF-8. So if you want to panic on such filenames, that's fine. Personally (given how rare and bizarre that situation would be), I'd probably panic as well (or propagate, similar to the other IO exceptions). If you want to recover, you could use to_string_lossy, which replaces invalid UTF-8 sequences with U+FFFD.

If you want to propagate, you can convert Option into io::Result with ok_or_else.

Finally, since you do have a lot of IO going on here, I would actually recommend going ahead and factoring this out into a separate function that results an io::Result. Then main can call unwrap (or expect) once on the result to indicate any IO errors, but other callers could theoretically handle or recover from those same errors.

With all of that in mind, we get down to one expect call in main that deals (uniformly) with all of the IO errors as follows.

use std::fs;
use std::io;
use std::path::Path;

fn replace_files(dir: &Path) -> io::Result<()> {
  for entry in fs::read_dir(dir)? {
    let path = entry?.path();
    if let Some(extension) = path.extension() {
      if let Some(filename) = path.file_name() {
        if path.is_file() && extension == "png" {
          let filename_utf8 =
            filename.to_str()
            .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Non-UTF-8 filename"))?;
          let new_path = path.with_file_name(filename_utf8.replace("_",""));
          fs::rename(path, new_path)?;
        }
      }
    }
  }
  Ok(())
}

fn main() {
  let dir = Path::new("/home/alex/Desktop");
  replace_files(dir).expect("I/O error occurred!");
}
Silvio Mayolo
  • 62,821
  • 6
  • 74
  • 116
  • 2
    nit: you don't need an extra function to be able to use ?, `main` can have `io::Result<()>` as return type too and it's output on error is probably more informative than a simple "I/O error occured!". – cafce25 Jan 22 '23 at 18:47