7 ms·

More readable, and IMHO more idiomatic version: (Disclaimer: never tested) let mut idx = HashMap::new(); for fname in &args { let f = match fs::F

More readable, and IMHO more idiomatic version: (Disclaimer: never tested) let mut idx = HashMap::new(); for fname in &args { let f = match fs::File::open(fname) { Ok(f) => f, Err(e) => panic!("input file {} could not be opened: {}", fname, e), }; let f = io::BufReader::new(f); let mut words = HashSet::new(); for line in f.lines() { for w in line.unwrap().split_whitespace() { if words.insert(w.to_string()) { // new word seen idx.entry(w.to_string()).or_insert(Vec::new()).push(fname); } } } } People who was introduced to the functional approach for the first time seems to enjoy it so much that everything becomes a hard-to-read mess of functions. I had similar experiences with Python list comprehension and C# LINQ.

bjz_ 10y ago
That one looks to be more efficient too - all those `collect`s in the original are a tad concerning...
- Animats 10y ago
  Do the "collects" mean that memory is allocated and the whole collection stored temporarily, or is this all iterative generators? (I thought I knew Rust, but I haven't used it for a year, and its usage has changed a lot even if the core language hasn't.)
  - steveklabnik 10y ago
    collect will ... collect it all into some kind of data structure, so yeah, that version would be doing a bunch of allocation.
ngrilly 10y ago
Definitely more readable to my eyes.
- wott 10y ago
  Yes, it is both much shorter and immediately readable and understandable (without comments!), unlike all the Functional Programming style versions which were proposed in the comments.
  - ngrilly 10y ago
    Yes. Functional programming is really useful and powerful in some situations, but sometimes the old school imperative programming is what you need. There is a reason why all cooking recipes are written in an imperative style.
  - T-R 10y ago
    This is maybe true, given that Rust is intentionally multi-paradigm, but it's a pretty unfair comparison: My FP-style version deliberately made almost no changes to the original code aside from adding names - I even stated in the comment that I don't even know Rust (haven't touched it beyond knowing what "Affine types" means, and once having gotten someone else's "hello world" to compile for ARM). The imperative version you're comparing it to completely changes the semantics of the program, and is written by someone very familiar with the language.
jupp0r 10y ago
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.
- lifthrasiir 10y ago
  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.
  - arielb1 10y ago
    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))
  - T-R 10y ago
    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.
k_bx 10y ago
In my taste, it's also more correct to write "for" in a code which is more like statements (not expressions), and only use map/fold for stuff which is pure.
lmm 10y ago
You can have it all though - at least, you can in Scala, Rust ought to support something similar. Something like (hybrid syntax/pseudocode): (for { fname <- args f = fs::File::open(fname).orElse( e => panic!("input file {} could not be opened: {}", fname, e)) r = io::BufReader::new(f) w <- f.lines().flatMap { line => line.unwrap().split_whitespace() } .distinct } yield Map(fname -> Vec(w.toString))).sum Just as concise as yours (more concise even), but easier to reason about and safer to refactor. Possibly you might need to do the mutable version for performance if this was a hot loop, but you don't get to even ask that question until you've profiled it and found out that it was the bottleneck; 99.9% of the time it isn't.
Jonhoo 10y ago
I think it'd actually be good to show both to show how versatile Rust can be in this regard. I added it to the article with a link back here. Thanks!
agumonkey 10y ago
The upper map-full code is also badly formatted and has comments everywhere. It's true that chaining functional abstractions is tempting at first; but with a bit of balance: let words = fn (f, file) => { file.lines().flat_map( |line| { line.unwrap() .split_whitespace() .map(|w| w.to_string()) .collect::<Vec<_>>() .into_iter() }) .collect::<HashSet<_>>() // prune duplicates .into_iter() .map(move |word| (word, f)) // and emit inverted index entry } let idx = args.iter() .map(|fname | (fname.as_str(), fs::File::open(fname.as_str()))) //+ (str, fd) .map(|(fname, f)| { f.and_then(|f| Ok((fname, f))).expect(&format!("input file {} could not be opened", fname)) }) .map(|(fname, f)| (fname, io::BufReader::new(f))) //+ (str, buf) .flat_map(words) //+ {str} // absorb all entries into a vector of file names per word .fold(HashMap::new(), |mut idx, (word, f)| { idx .entry(word) .or_insert(Vec::new()) .push(f); }); //+ {word:[filename]}

bjz_ 10y ago

That one looks to be more efficient too - all those `collect`s in the original are a tad concerning...

Animats 10y ago
Do the "collects" mean that memory is allocated and the whole collection stored temporarily, or is this all iterative generators? (I thought I knew Rust, but I haven't used it for a year, and its usage has changed a lot even if the core language hasn't.)
- steveklabnik 10y ago
  collect will ... collect it all into some kind of data structure, so yeah, that version would be doing a bunch of allocation.

ngrilly 10y ago

Definitely more readable to my eyes.

wott 10y ago
Yes, it is both much shorter and immediately readable and understandable (without comments!), unlike all the Functional Programming style versions which were proposed in the comments.
- ngrilly 10y ago
  Yes. Functional programming is really useful and powerful in some situations, but sometimes the old school imperative programming is what you need. There is a reason why all cooking recipes are written in an imperative style.
- T-R 10y ago
  This is maybe true, given that Rust is intentionally multi-paradigm, but it's a pretty unfair comparison: My FP-style version deliberately made almost no changes to the original code aside from adding names - I even stated in the comment that I don't even know Rust (haven't touched it beyond knowing what "Affine types" means, and once having gotten someone else's "hello world" to compile for ARM). The imperative version you're comparing it to completely changes the semantics of the program, and is written by someone very familiar with the language.

jupp0r 10y ago

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.

lifthrasiir 10y ago
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.
- arielb1 10y ago
  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))
- T-R 10y ago
  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.

k_bx 10y ago

In my taste, it's also more correct to write "for" in a code which is more like statements (not expressions), and only use map/fold for stuff which is pure.

lmm 10y ago

You can have it all though - at least, you can in Scala, Rust ought to support something similar. Something like (hybrid syntax/pseudocode): (for { fname <- args f = fs::File::open(fname).orElse( e => panic!("input file {} could not be opened: {}", fname, e)) r = io::BufReader::new(f) w <- f.lines().flatMap { line => line.unwrap().split_whitespace() } .distinct } yield Map(fname -> Vec(w.toString))).sum Just as concise as yours (more concise even), but easier to reason about and safer to refactor. Possibly you might need to do the mutable version for performance if this was a hot loop, but you don't get to even ask that question until you've profiled it and found out that it was the bottleneck; 99.9% of the time it isn't.

Jonhoo 10y ago

I think it'd actually be good to show both to show how versatile Rust can be in this regard. I added it to the article with a link back here. Thanks!

agumonkey 10y ago

The upper map-full code is also badly formatted and has comments everywhere. It's true that chaining functional abstractions is tempting at first; but with a bit of balance: let words = fn (f, file) => { file.lines().flat_map( |line| { line.unwrap() .split_whitespace() .map(|w| w.to_string()) .collect::<Vec<_>>() .into_iter() }) .collect::<HashSet<_>>() // prune duplicates .into_iter() .map(move |word| (word, f)) // and emit inverted index entry } let idx = args.iter() .map(|fname | (fname.as_str(), fs::File::open(fname.as_str()))) //+ (str, fd) .map(|(fname, f)| { f.and_then(|f| Ok((fname, f))).expect(&format!("input file {} could not be opened", fname)) }) .map(|(fname, f)| (fname, io::BufReader::new(f))) //+ (str, buf) .flat_map(words) //+ {str} // absorb all entries into a vector of file names per word .fold(HashMap::new(), |mut idx, (word, f)| { idx .entry(word) .or_insert(Vec::new()) .push(f); }); //+ {word:[filename]}