6
\$\begingroup\$

I'm learning Rust now and created a program to solve the task General Fizzbuzz on Rosetta Code.

For example, given:

20
3 Fizz
5 Buzz
7 Baxx

where the first line indicates the upper bound and every line after that indicates the divisor and the corresponding word, the program should return:

1
2
Fizz
4
Buzz
Fizz
Baxx
8
Fizz
Buzz
11
Fizz
13
Baxx
FizzBuzz
16
17
Fizz
19
Buzz

The code is as follows:

use std::io;
use std::io::BufRead;

fn parse_entry(l: &str) -> (i32, String) {
    let params: Vec<&str> = l.split(' ').take(2).collect();

    let divisor = params[0].parse::<i32>().unwrap();
    let word = params[1].to_string();
    (divisor, word)
}

fn main() {
    let stdin = io::stdin();
    let mut lines = stdin.lock().lines().map(|l| l.unwrap());

    let l = lines.next().unwrap();
    let high = l.parse::<i32>().unwrap();

    let mut entries = Vec::new();
    for l in lines {
        let entry = parse_entry(&l);
        entries.push(entry);
    }

    for i in 1..(high + 1) {
        let mut line = String::new();
        for &(divisor, ref word) in &entries {
            if i % divisor == 0 {
                line = line + &word;
            }
        }
        if line == "" {
            println!("{}", i);
        } else {
            println!("{}", line);
        }
    }
}

I would like feedback on improving the readability of this code and making it more concise. Is there a better way to parse the input, or to access the entries?

\$\endgroup\$
  • 1
    \$\begingroup\$ Have you seen the excellent blog post by Chris Morgan about FizzBuzz in Rust? \$\endgroup\$ – Shepmaster Apr 5 '15 at 21:24
  • \$\begingroup\$ @Shepmaster I have not, thanks for that link. \$\endgroup\$ – wei2912 Apr 6 '15 at 11:49
3
\$\begingroup\$

First, I added some stub variables so I could run the code in the Playpen:

let z = ["20", "3 Fizz", "5 Buzz", "7 Baxx"];
let mut lines = z.iter().map(|s| s.to_string());

Next, I have a dislike for specifying the type in a function call, and prefer to just specify the type of the variable:

// let high = l.parse::<i32>().unwrap();
let high: i32 = l.parse().unwrap();

I also wanted to see if I could remove the temporary variable. I did, but not so sure it makes it better:

let high: i32 = lines.next().and_then(|l| l.parse().ok()).unwrap();

I then replaced manual loop management with a functional-style map. This has the great benefit of not requiring the entries vector to be mutable. I find it's often a good idea to constrain the mutability to as small as possible. I also don't like specifying types that the compiler can infer for me. In this case, I use Vec<_> instead of Vec<(i32, String)>. _ is used to denote "I don't care", both in types and in variable names (such as for an unused variable):

// let mut entries = Vec::new();
// for l in lines {
//     let entry = parse_entry(&l);
//     entries.push(entry);
// }

let entries: Vec<_> = lines.map(|l| parse_entry(&l)).collect();

I did a similar thing for the inner loop of joining strings:

// let mut line = String::new();
// for &(divisor, ref word) in &entries {
//     if i % divisor == 0 {
//         line = line + &word;
//     }
//  }

let line: String = entries.iter()
    .filter(|&&(divisor, _)| i % divisor == 0)
    .map(|&(_, ref word)| &word[..])
    .collect();

I then wanted to give a bit of structure (pun half intended) to the data:

struct Entry {
    divisor: i32,
    word: String,
}

impl Entry {
    fn matches(&self, val: i32) -> bool { val % self.divisor == 0 }
    fn word(&self) -> &str { &self.word }
}

Which makes the inner loop look like:

let line: String = entries.iter()
    .filter(|entry| entry.matches(i))
    .map(Entry::word)
    .collect();

I then moved parse_entry to an implementation of FromStr. This will allow Entry to participate in the fun that is str::parse:

impl str::FromStr for Entry {
    type Err = ();

    fn from_str(s: &str) -> Result<Entry, ()> {
        let params: Vec<&str> = s.split(' ').take(2).collect();

        let divisor = params[0].parse::<i32>().unwrap();
        let word = params[1].to_string();
        Ok(Entry { divisor: divisor, word: word })
    }
}

The "downside" is that parsing an entry can now fail, and so we have to deal with that. Really, there's an unwrap call in there that truly does indicate failure, so that should be addressed:

let entries: Result<Vec<Entry>, ()> = lines.map(|l| l.parse()).collect();
let entries = entries.unwrap();

Here's my final code:

use std::{io,str};
use std::io::BufRead;

struct Entry {
    divisor: i32,
    word: String,
}

impl Entry {
    fn matches(&self, val: i32) -> bool { val % self.divisor == 0 }
    fn word(&self) -> &str { &self.word }
}

impl str::FromStr for Entry {
    type Err = ();

    fn from_str(s: &str) -> Result<Entry, ()> {
        let params: Vec<_> = s.split(' ').take(2).collect();

        let divisor: i32 = params[0].parse().unwrap();
        let word = params[1].to_string();
        Ok(Entry { divisor: divisor, word: word })
    }
}

fn main() {
    //let stdin = io::stdin();
    //let mut lines = stdin.lock().lines().map(|l| l.unwrap());

    let z = ["20", "3 Fizz", "5 Buzz", "7 Baxx"];
    let mut lines = z.iter().map(|s| s.to_string());

    let high: i32 = lines.next().and_then(|l| l.parse().ok()).unwrap();

    let entries: Result<Vec<Entry>, ()> = lines.map(|l| l.parse()).collect();
    let entries = entries.unwrap();

    for i in 1..(high + 1) {
        let line: String = entries.iter()
            .filter(|entry| entry.matches(i))
            .map(Entry::word)
            .collect();

        if line.is_empty() {
            println!("{}", i);
        } else {
            println!("{}", line);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for your excellent feedback! Just a question, why is the redeclaration of entries allowed, and what does Vec(_) mean? \$\endgroup\$ – wei2912 Apr 6 '15 at 11:59
  • \$\begingroup\$ @wei2912 It's very hard to answer why something is allowed. The "answer" is because Rust allows it. Unless you want me to make speculation about the hopes and desires of the Rust core team, perhaps you can reword your question? I'll update my answer to describe Vec<_>. \$\endgroup\$ – Shepmaster Apr 7 '15 at 16:47
  • \$\begingroup\$ Sorry, I wanted to ask about under what conditions is redeclaration allowed. Is it generally allowed in Rust or are there restrictions? \$\endgroup\$ – wei2912 Apr 8 '15 at 12:39
  • 1
    \$\begingroup\$ @wei2912 gotcha. Yes, redeclaration of variables is generally allowed. I'm always a little surprised that you can even redeclare something that is borrowed. Under the hood, the owned variable must still be tracked by the compiler, it's just not available to the programmer anymore. \$\endgroup\$ – Shepmaster Apr 8 '15 at 13:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.