General Fizzbuzz

Question

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?

Answer 1

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