I know there are obviously lots of rooms for improvements. Indeed, if the code would become more complex I would immediately refactor. I can list some problems with my example:
- The error handling should really have been refactored. `try!` would make this easier. (I haven't used it since it is in the `main` function and the code was a direct replacement.)
- Error during reading words is not accurately handled. Again, `try!` would make this easier.
- `words` and `idx` look coupled to each other, which ideally shouldn't have been.
- `words.insert` is quite an opaque method; not everyone is sure if it returns true on a duplicate key or not. I've added a comment but frankly I'm not satisfied of that. <deleted> One alternative is to use `words.entry` instead, which gives a named enum variant. </deleted> Oops, `HashSet` does not have `entry`...
- Ultimately, the body of the outermost loop should go to a function.
That said, do you really think that `line.unwrap().split_whitespace().map(|w| w.to_string()).collect::<Vec<_>>().into_iter()` is a chunk of code which can be read at a glance? It at least has to be named (like T-R's example). Functional approach means that you can split functions and individually review them; the original code, IMHO, didn't.
I think the issue with procedural loops (not to be critical, just in general) is that there's no abstraction - it's not clear what's getting mutated, or what the result is (or its type), it's harder to look at the individual steps, and it's harder to refactor.
The nice thing about functions like "map", "filter", "fold", "flatmap", etc., is that they describe intent - when you see a "map", you know it's not aggregating things, just applying a function: "map" has a distinct purpose from "fold" (and depending on how pure things are, you may not even be allowed to do anything crazy).
Aside from that, the higher-order functions have pretty intuitive algebraic laws for refactoring (like with "compose" mentioned in my other comment) - It's not clear how you'd factor code out of a nested loop without understanding the whole thing, whereas "flatmap" (a.k.a. "bind" for the list monad) has laws for refactoring it.
I agree to you with the general sentiment. That said, Rust is not a functional language per se; the degree of functional decomposition is thus fundamentally limited. This is partly because it tries to be efficient, and as arielb1 pointed out, ownership sometimes makes it worse.
> It's not clear how you'd factor code out of a nested loop without understanding the whole thing, [...]
You are right, loops are particularly hard to refactor. I still argue that my code is better (barring any future expansion) because it fits within handful lines; you cannot easily understand 100 lines of code with the cyclomatic complexity of 1, but you can often easily understand 10 lines of code with the cyclomatic complexity of 8 (e.g. triple loops). The size of code, syntax and contextual information matters as much as algebraic laws.
That part is certainly extreme because of ownership. It can be refactored to
let mut words = HashSet::new();
for line in file.lines() {
let line_words = line.unwrap().split_whitespace();
words.extend(
line_words.map(|s| s.to_string())
);
}
words.into_iter().map(move |word| (word, f))
- The error handling should really have been refactored. `try!` would make this easier. (I haven't used it since it is in the `main` function and the code was a direct replacement.)
- Error during reading words is not accurately handled. Again, `try!` would make this easier.
- `words` and `idx` look coupled to each other, which ideally shouldn't have been.
- `words.insert` is quite an opaque method; not everyone is sure if it returns true on a duplicate key or not. I've added a comment but frankly I'm not satisfied of that. <deleted> One alternative is to use `words.entry` instead, which gives a named enum variant. </deleted> Oops, `HashSet` does not have `entry`...
- Ultimately, the body of the outermost loop should go to a function.
That said, do you really think that `line.unwrap().split_whitespace().map(|w| w.to_string()).collect::<Vec<_>>().into_iter()` is a chunk of code which can be read at a glance? It at least has to be named (like T-R's example). Functional approach means that you can split functions and individually review them; the original code, IMHO, didn't.