Hacker News Reader

Comment by jupp0r | Hacker News Reader

Sorry, but I disagree. Although more verbose (as in more characters of source code), I could easily understand what the original code did. Your code has a high cyclomatic complexity and I have to keep all those nested for loops in mind when trying to figure out what you are doing.

I write C++ for most of my day job, and this would not pass code review because of readability problems in my team.

View on HN

I like to use (even abuse) lazy iterators in C++ as the next guy, but IMHO the imperative version in this case is not only shorter but significantly more readable.

The two loops contain a single statement and it is trivial to see what's going on. It is also easier to extend. And it is still using lazy iterators (I assume) where it makes sense, i.e. in the split_whitespace call.

I cannot upvote you enough.

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.

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))

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.

The "idomatic" version (I agree with that) simply works better. You can insert the typical `try!()` calls that short-circuit on error and return errors to the caller.

Retrofitting better error handling in the single expression version requires changing the shape of the whole thing, editing the expression throughout.

Reply on news.ycombinator.com