Converting a for loop with error handling into iterator

help
1

I'm trying to rewrite the following function using iterators but I'm not sure how to do it.

fn load_cheats<P: AsRef<Path>>(path: P) -> Result<Vec<Cheat>> {
    let mut cheats = Vec::new();

    for entry in std::fs::read_dir(path)? {
        let entry = entry?.path();
        if entry.is_file() {
            let data = std::fs::read_to_string(entry)?;
            cheats.push(data.parse()?);
        }
    }

    Ok(cheats)
}

I got it working but only if I use unwrap on the results which isn't good.

fn load_cheats<P: AsRef<Path>>(path: P) -> Result<Vec<Cheat>> {
    Ok(std::fs::read_dir(path)?
        .map(|e| e.unwrap().path())
        .filter(|p| p.is_file())
        .map(|p| std::fs::read_to_string(p).unwrap())
        .map(|data| data.parse().unwrap())
        .collect())
}

I can't figure out how to remove the unwraps when chaining multiple maps and filters that return Results.

I saw one reply on StackOverflow where there were multiple collects in the chain but this seems more complicated than it needs to be: rust - How to handle Result in flat_map - Stack Overflow

Another answer was to move the Result out of the iterator loop which looks goo but only seems to work when you're not chaining multiple iterators: rust - How do I stop iteration and return an error when Iterator::map returns a Result::Err? - Stack Overflow

2 
   std::fs::read_dir(path)?
        .map(|e| e.map(|e| e.path()))
        .filter(|p| p.map_or(true, |p| p.is_file()))
        .map(|p| p.and_then(|p| std::fs::read_to_string(p)))
        .map(|data| data.and_then(|data| data.parse()))
        .collect()

It's a bit unwieldy but it should work.

3

Thanks for the reply! I tried this but I get the following error:

error[E0277]: a value of type `std::result::Result<std::vec::Vec<cheat::Cheat>, anyhow::Error>` cannot be built from an iterator over elements of type `std::result::Result<_, std::io::Error>`
  --> src/main.rs:42:10
   |
42 |         .collect()
   |          ^^^^^^^ value of type `std::result::Result<std::vec::Vec<cheat::Cheat>, anyhow::Error>` cannot be built from `std::iter::Iterator<Item=std::result::Result<_, std::io::Error>>`
   |
   = help: the trait `std::iter::FromIterator<std::result::Result<_, std::io::Error>>` is not implemented for `std::result::Result<std::vec::Vec<cheat::Cheat>, anyhow::Error>`

I'm guessing it's because the read_to_string and parse methods return different error types

4

You'll have to be explicit about the type I guess:

   std::fs::read_dir(path)?
        .map(|e| e.map_err(anyhow::Error::from).map(|e| e.path()))
        .filter(|p| p.map_or(true, |p| p.is_file()))
        .map(|p| p.and_then(|p| Ok(std::fs::read_to_string(p)?)))
        .map(|data| data.and_then(|data| Ok(data.parse()?)))
        .collect()
5

Why? Is there something wrong with it? Or is this just an exercise?

Idiomatic Rust uses iterators where appropriate. You shouldn't rewrite every for loop as a chain of iterator adapters just because you can. This is a case where you probably shouldn't, because you have complex error handling and using iterator adapters with closures makes it harder to use ? to write the code in the most obvious way.

1 Like
6

I certainly think the code is perfectly easy to read with the for loop. I do not think the iterator gives you anything in this case.

1 Like
7

I started out writing it with iterators but then ran into issues and realised I didn't know how to handle Result inside iterators. After seeing the iterator version, I agree with you and alice:

I will keep the for loop as it seems more readable to me, so now it's more of an exercise for me to learn more about iterators!

Thanks everyone for the replies!

2 Likes
8

Thanks for that, I learned some useful stuff!

9

I'm glad I waited and you asked that question before I did.

Perhaps I am wrong but I would note a couple of things about this:

  1. The original code with it's for loop is a clear and concise description of what you want to do. And it copes with errors well.

  2. The "functional style" is a mess that hides what one wants to do in a lot of map, filter', more map, more map', collect, syntactic noise along the way. Not to mention the lambda syntax noise along the way.

  3. Most importantly, this kind of code can fail in at least three interesting ways: The read_dir can fail for many reasons, the file read can fail more many reasons, and so on. These kind of failures are often more common than the 'happy path'. Certainly not exceptional. Ones code should reflect that reality.

  4. The proposed solutions to the error handling problem make the code obscure what you actually want to do even more.

1 Like
10
  1. Totally agree. My original question isn't accurate though as I initially wrote it with iterators and unwraps, then tried to get rid of the unwraps. I didn't know how to do that so I rewrote it was a for loop. I was just curious if there was something simple I was missing that would make the iterator version the better choice.

2 - 4. Agree again. After seeing what it would require to get it working with iterators, I can see that the for loop is obviously the better option as it's much easier to see what's going on and where it could fail. This was mainly filling a knowledge gap I had with iterators =]

11

No worries.

I'm in the same boat. This is a whole new world to me.

Having said that, there have been some cases discussed here during my first year of Rust where the functional, iterator style turned out to be pretty clear and easy to comprehend. And more importantly outperformed the good old fashioned C style for loops.

12

Using Rust I started really loving the functional approach to chain operations and reduce the working surface inside the closures.

Unfortunately this is one of the cases that using normal iterator chaining brings to very unclean code (I ended up with something pretty similar to what @Kestrer showed).

However, the problem is not with the approach, but with the available abstractions: what we miss is a set of methods to work with Try-able things inside iterators. Seriously, look at the following code:

fn load_cheats<P: AsRef<Path>>(path: P) -> Result<Vec<Cheat>> {
    std::fs::read_dir(path)?
        .map_results(|entry| entry.path())
        .filter_ok(|path| path.is_file())
        .and_then(std::fs::read_to_string)
        .and_then(|data: String| data.parse())
        .collect()
}

I find it pretty simple and clean... But you will need Itertools and even more...

Bunch of do-not-use-in-production code 
mod iter_super_powers {
    pub trait IterSuperPowers {
        fn filter_ok<F, T, E>(self, f: F) -> FilterOk<Self, F>
        where
            Self: Sized + Iterator<Item = Result<T, E>>,
            F: FnMut(&T) -> bool,
        {
            FilterOk { iter: self, f }
        }

        fn and_then<F, T, U, E>(self, f: F) -> AndThen<Self, F>
        where
            Self: Sized,
            F: FnMut(T) -> Result<U, E>,
        {
            AndThen { iter: self, f }
        }
    }

    pub struct FilterOk<I, F> {
        iter: I,
        f: F,
    }

    impl<I, F, T, E> Iterator for FilterOk<I, F>
    where
        I: Iterator<Item = Result<T, E>>,
        F: FnMut(&T) -> bool,
    {
        type Item = <I as Iterator>::Item;

        fn next(&mut self) -> Option<Self::Item> {
            loop {
                let el = self.iter.next()?;
                match el {
                    Ok(el) => {
                        if (self.f)(&el) {
                            break Some(Ok(el));
                        }
                    }
                    err @ Err(_) => break Some(err),
                }
            }
        }
    }

    pub struct AndThen<I, F> {
        iter: I,
        f: F,
    }

    impl<I, F, T, U, EI, EO> Iterator for AndThen<I, F>
    where
        I: Iterator<Item = Result<T, EI>>,
        EI: Into<EO>,
        F: FnMut(T) -> Result<U, EO>,
    {
        type Item = Result<U, EO>;

        fn next(&mut self) -> Option<Self::Item> {
            self.iter.next().map(|el| match el {
                Ok(el) => (self.f)(el),
                Err(err) => Err(err.into()),
            })
        }
    }

    impl<I: Iterator> IterSuperPowers for I {}
}

I am not totally sure why we don't have these methods in the std, maybe because abstracting Iterator functions over the Try trait is not the best at the current state... :man_shrugging:

Said that, I think that in this particular case using a for loop is much better, it does not make any sense to add 93 LoC to abstract away something that is already clean and maintainable :blush:

3 Likes
13

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.