patternrustMinor
General Fizzbuzz
Viewed 0 times
fizzbuzzgeneralstackoverflow
Problem
I'm learning Rust now and created a program to solve the task General Fizzbuzz on Rosetta Code.
For example, given:
where the first line indicates the upper bound and every line after that indicates the divisor and the corresponding word, the program should return:
The code is as follows:
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?
For example, given:
20
3 Fizz
5 Buzz
7 Baxxwhere 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
BuzzThe code is as follows:
use std::io;
use std::io::BufRead;
fn parse_entry(l: &str) -> (i32, String) {
let params: Vec = l.split(' ').take(2).collect();
let divisor = params[0].parse::().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::().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?
Solution
First, I added some stub variables so I could run the code in the Playpen:
Next, I have a dislike for specifying the type in a function call, and prefer to just specify the type of the variable:
I also wanted to see if I could remove the temporary variable. I did, but not so sure it makes it better:
I then replaced manual loop management with a functional-style
I did a similar thing for the inner loop of joining strings:
I then wanted to give a bit of structure (pun half intended) to the data:
Which makes the inner loop look like:
I then moved
The "downside" is that parsing an entry can now fail, and so we have to deal with that. Really, there's an
Here's my final code:
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::().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. _ 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 {
let params: Vec = s.split(' ').take(2).collect();
let divisor = params[0].parse::().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, ()> = 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 {
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, ()> = 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);
}
}
}Code Snippets
let z = ["20", "3 Fizz", "5 Buzz", "7 Baxx"];
let mut lines = z.iter().map(|s| s.to_string());// let high = l.parse::<i32>().unwrap();
let high: i32 = l.parse().unwrap();let high: i32 = lines.next().and_then(|l| l.parse().ok()).unwrap();// 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();// 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();Context
StackExchange Code Review Q#85865, answer score: 3
Revisions (0)
No revisions yet.