5 ms·
I actually don't care as much for iterators as I thought I would. Beyond some simpler map().reduce() stuff they tend to fall over pretty fast, and I end up spen
by throwaway894345 4y ago
I actually don't care as much for iterators as I thought I would. Beyond some simpler map().reduce() stuff they tend to fall over pretty fast, and I end up spending too much time trying to make iterator chains work before defaulting to for loops. I also dislike how anemic Rust's stdlib is. I'm sure there are good reasons, but I like that I can just reach for Go's stdlib for annotating errors or dealing with JSON. Other than that, I like the API design in Rust's stdlib.
Here's an example where the iterator-chain version is way more complex:
let posts = read_dir(source_directory)?
.filter_map(|result| {
(|| {
let entry = result?;
let os_file_name = entry.file_name();
let file_name = os_file_name.to_string_lossy();
if Self::is_bundle(&entry)? {
self.parse_post_bundle(&file_name, &entry.path())
.map(Some)
} else if file_name.ends_with(MARKDOWN_EXTENSION) {
self.parse_post(
file_name.trim_end_matches(MARKDOWN_EXTENSION),
&mut File::open(&entry.path())?,
)
.map(Some)
} else {
Ok(None)
}
})()
.transpose()
})
.collect::<Result<Vec<_>>>()?;
Here's the imperative version:
let mut posts = Vec::new();
for result in read_dir(source_directory)? {
let entry = result?;
let os_file_name = entry.file_name();
let file_name = os_file_name.to_string_lossy();
if Self::is_bundle(&entry)? {
posts.push(self.parse_post_bundle(&file_name, &entry.path())?);
} else if file_name.ends_with(MARKDOWN_EXTENSION) {
posts.push(self.parse_post(
file_name.trim_end_matches(MARKDOWN_EXTENSION),
&mut File::open(&entry.path())?,
)?);
}
}
- SoftTalker 4y agoI guess that's subjective, not knowing rust I see a few more lines in the first example but beyond that it does not jump out at me as being "way more complex."
- throwaway894345 4y agoHah, it took me and a bunch of other people in the Rust discord a fair amount of time to figure out how to get the first example to compile correctly in the first place, and I find the “transpose()” business to be particularly convoluted. I think even Rust people would prefer the imperative version. I also suspect there’s some hindsight benefit at play in that it’s easier to make sense of the first snippet than it was to write it.
- tialaramex 4y agoYes, the imperative version looks like the sane choice to me. Rust's iterator documentation does suggest that this is not a language which favours iterator chains everywhere. However one reason to prefer an iterator chain in some cases is that the chain might imply an optimisation you'd otherwise have to write manually and might not think of. For example if I have N things, and I map them, perhaps more than once, but I'm definitely getting N of whatever the output of the last map was, the iterator chain might very well see that I don't actually have any way to change N and so when I collect() that into a Vec the Vec it gives me is constructed with capacity N, saving the (amortized but non-zero) cost of growing it during insertions. Imperatively I can remember to write Vec::with_capacity(N), and that's safe of course but it's an extra step to remember. The imperative approach does particularly shine on the opposite edge of this, if I know that I'm getting no more than 100 items out of this pipeline, despite putting N in, Rust almost certainly won't see that and won't make Vec::with_capacity(100) for a collect() call whereas my imperative code can just explicitly write Vec::with_capacity(100) or whatever.
- sgeisenh 4y agoDepending on how you write it, iterators are much better at this since most iterator methods produce iterators with size hints. If you use "take" for example, collect should do the right thing: https://doc.rust-lang.org/src/core/iter/adapters/take.rs.html#58 https://doc.rust-lang.org/src/core/iter/adapters/take.rs.htm...
- tialaramex 4y ago
- mamcx 4y agoThe thing is in Rust this is totally ok. Is NOT un-idiomatic to mix imperative + functional paradigms in the same block.
- throwaway894345 4y agoAgreed, but I think people make too big a deal about iterators. The simple for loops that they’re fit to replace aren’t that hard to read or write, so they aren’t causing real problems, and on the more complex end you’re writing imperative stuff anyway. Iterators certainly aren’t a bad thing, but they’re not the game changer that Go-critics made them out to be in all of the Rust-vs-Go debates.
- nomel 4y agoI disagree. If I can avoid an index variable, I’m going to have less bugs since I won’t have to index.
- throwaway894345 4y agoI'm not sure what you mean. You don't have to deal with index variables in a for loop: for item in iterator { vector.push_back(foo(item)); } You're not explicitly indexing the source iterator or the destination vector at any point.
- nomel 4y agoAgreed. That's why I enjoy iterators, and use them whenever possible. I think they're important.
- throwaway894345 4y agoBut earlier you claimed the for-loop version required index variables and indexing into iterators/vectors/etc ... ?
- drran 4y agoNow, compare a parallel version of both.
- throwaway894345 4y agoIt's not obvious to me that the parallel version of the first is going to be cleaner (or more performant) than a parallel version of the second, but even if it is, I don't agree with mangling (to put it nicely) single-threaded code on the off chance that it might be made parallel one day.
- thinkharderdev 4y agoYep, agree wholeheartedly with this. I came to Rust from Scala where writing an explicit for loop is looked at as one step above using goto :) and it took me a while to just give in and write the imperative version.
- nu11ptr 4y agoThere is no need to force something iterator style. By all means use a loop if it is more readable. Also, if your iterator style closure body gets very verbose, you can always pull the logic out into a function or closure and pass it in. In the end, both styles are idiomatic, and the best choice is which makes it more readable (subjective of course). I personally use iterator-style for 1-3 simple ops, but go into a loop for anything more complicated. The functional part of me wants to force iterator style but I fight it for complex use cases because it is irrational on my part, and I find loops easier to read for stuff like that.
- lijogdfljk 4y agoFair, but i'd probably write your iterator example differently. Though, besides the point perhaps - as i agree there are plenty of cases where imperative approaches are cleaner. I however will often have a dozen maps/filters/etc and that style in imperative is what, nesting repeatedly or using `continue`. Meh. If your Iterator consists of a single `filter_map` i can see why you don't get much value out of them in that case. It's because.. well, there's not much value to get out of it.
- 0x457 4y agoIterator-chain version would be a lot better looking if it was actually was a chain and not everything pushed into a single `filter_map`. "Ifs" are weird looking, one branch is using `entry`(I take it's a `DirEntry`?) and another is using `file_name` I suspect both could use the and look like `Self::is_` ? If you're going to write an iterator chain in an imperative way, then you might as well right it imperatively. A few notes: - you can compare `&OsStr` with `&str` without `to_string_lossy` any explicit conversion (`&str -> &OsStr` done in `PartialEq` is basically free) - you can get extension from `Path` by using `Path::extension` - you can get filename without extension by using `Path::file_stem` or `Path::file_prefix` - making your parse functions take the same arguments makes it a lot cleaner Something like this: read_dir("")? .map(|e| e.map(|e| e.path())) .map(|path| { path.and_then(|path| { if Self::is_bundle(&path) { self.parse_post_bundle(&path.file_name().to_string_lossy(), &path).map(Some) } else if Self::is_markdown(&path) { self.parse_post(&path.file_stem().to_string_lossy(), &path).map(Some) } else { Ok(None) } }) }) .filter_map(Result::transpose) .collect::<Result<Vec<()>, std::io::Error>>() // Ignore this, just an example fn is_bundle(path: &Path) -> bool { path.extension().map(|ext| ext == "bundle").unwrap_or(false) } fn is_markdown(path: &Path) -> bool { path.extension().map(|ext| ext == "md").unwrap_or(false) }
- Already__Taken 4y agoForgive me for asking instead of looking, I'm still getting into rust; why do you map instead of getting the result? path.extension().map(|ext| ext == "bundle").unwrap_or(false) vs (path.extension() == "bundle").unwrap_or(false) or further up .map(|e| e.map(|e| e.path())) vs .map(|e| e.path()) Is the .map keeping any value wrapped in an iterator so you can map again? I thought it was interesting what you wrote looks a lot like what I'd write in powershell.
- steveklabnik 4y agoNot your parent, but I do know the answer. extension() returns an Option<&OsStr>, because not every file has an extension https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.extension https://doc.rust-lang.org/stable/std/path/struct.Path.html#m... map on option says "run this closure on the value if it's Some, or do nothing if it's None https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.map https://doc.rust-lang.org/stable/std/option/enum.Option.html... So after the map, we now have an Option<bool>. The unwrap_or method will say "give me the value if it's Some, and give me the argument if it's not https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.unwrap_or https://doc.rust-lang.org/stable/std/option/enum.Option.html... That means we finally end up with a bool. (path.extension() == "bundle").unwrap_or(false) This code wouldn't work because you're comparing an Option<&OsStr> to a &str. Even if that worked, you'd have a boolean, and unwrap_or doesn't make sense on booleans, you'd just leave off that part.