Code Review Asked on December 4, 2021
Good evening, I’m new to Rust and would like some help refactoring this small app I made so that it’s more efficient, even if it may be unnecessary.
Basically I’m trying to figure out better ways to do stuff, and if I could analyze how someone would do the same thing, but better, I think it could help me learn a bit more about the language.
I haven’t got too far into the language yet, but I would like to nip any bad habits I might run into before they become a problem.
What this app basically does, is it asks for how many cards are in a Magic: The Gathering deck, and then a few questions about the cards, then determines how much and what types of lands you should have in the deck.
use std::io;
fn main() {
let deck_size = get_deck_size();
let spells = get_spell_count(deck_size);
let total_cmc = get_total_cmc();
calculate_mana(deck_size, spells, total_cmc);
fn get_deck_size() -> f32 {
loop {
let mut input = String::new();
println!("How many cards are in your deck?");
io::stdin()
.read_line(&mut input)
.expect("Failed to read input.");
let input = input
.trim()
.parse::<f32>()
.expect("Please enter a valid number.");
if input >= 40.0 {
break input;
}
println!("Your deck is too small, the minimum amount of cards in a deck is 40, please enter a new deck size.")
}
}
fn get_spell_count(deck_size: f32) -> f32 {
loop {
let mut input = String::new();
println!("How many spells are in your deck?");
io::stdin()
.read_line(&mut input)
.expect("Failed to read input.");
let input = input
.trim()
.parse::<f32>()
.expect("Please enter a valid number.");
if input <= deck_size {
break input;
}
println!("You have more spells in your deck than the amount of cards in your deck, please enter a new number of spells.")
}
}
fn get_total_cmc() -> f32 {
loop {
let mut input = String::new();
println!("What's the total converted mana cost of all your spells?");
io::stdin()
.read_line(&mut input)
.expect("Failed to read input.");
let input = input
.trim()
.parse::<f32>()
.expect("Please enter a valid number.");
if input >= 0.0 {
break input;
}
println!("Something is wrong here.")
}
}
fn calculate_mana(deck_size: f32, spells: f32, total_cmc: f32) {
let mut symbol_count = Vec::new();
let total_lands = deck_size - spells;
let colors = ["white", "blue", "green", "red", "black", "colorless"];
println!("Now we need to get all the mana symbols throughout your deck (not just in the cmc, but in the cards abilities as well).");
for i in colors.iter() {
let mut input = String::new();
println!("How many {} symbols are in the deck?", i);
io::stdin()
.read_line(&mut input)
.expect("Failed to read input.");
let color = (
input
.trim()
.parse::<f32>()
.expect("Please enter a valid number."),
i,
);
symbol_count.push(color);
}
println!("Your average CMC is: {}", total_cmc / deck_size);
println!("You should have {} total land", total_lands);
for i in symbol_count {
if i.0 > 0.0 {
let x = ((total_lands / spells) * i.0).round();
println!("{} of those lands should be {}", x, i.1);
}
}
}
}
Your code passes rustfmt
and clippy
— good job!
Here's my suggestions.
Move the functions out of main
. The local functions make main
unnecessarily long. Having free functions instead clarifies the
semantics of the program, and reduces the indentation.
In my understanding, the number of cards contained in a deck is always
a nonnegative integer, so it is probably better to use u32
instead
of f32
to store the card counts. u32
can be losslessly converted
to f64
when doing floating point arithmetic.
When the user input cannot be deciphered, the program panics with an error message. Failing to read a line is probably an irrecoverable error, but for parse errors, a better alternative is to ask the user to re-enter data, so that users don't have to repeat everything they typed because of a typo.
This pattern occurs many times:
let mut input = String::new();
println!("<prompt>");
io::stdin()
.read_line(&mut input)
.expect("Failed to read input.");
let input = input
.trim()
.parse::<f32>()
.expect("Please enter a valid number.");
We can make a dedicated function:
fn input(prompt: &str) -> Result<u32, std::num::ParseIntError> {
println!("{}", prompt);
let mut input = String::new();
io::stdin()
.read_line(&mut input)
.expect("cannot read input");
input.trim().parse()
}
Now, the functions that ask for input can be simplified:
fn get_deck_size() -> u32 {
loop {
match input("How many cards are in your deck?") {
Ok(deck_size) if deck_size >= 40 => return deck_size,
Ok(_) => println!("The minimum number of cards in a deck is 40."),
Err(_) => println!("Invalid number."),
}
}
}
fn get_spell_count(deck_size: u32) -> u32 {
loop {
match input("How many spells are in your deck?") {
Ok(spell_count) if spell_count <= deck_size => return spell_count,
Ok(_) => println!("You cannot have more spells than cards."),
Err(_) => println!("Invalid number."),
}
}
}
fn get_total_cmc() -> u32 {
loop {
match input("What's the total converted mana cost of all your spells?") {
Ok(total_cmc) => return total_cmc,
Err(_) => println!("Invalid number."),
}
}
}
Note that the total_cmc >= 0
check can be elided.
Here's my version:
const COLORS: [&str; 6] = ["white", "blue", "green", "red", "black", "colorless"];
fn calculate_mana(deck_size: u32, spells: u32, total_cmc: u32) {
let total_lands = deck_size - spells;
println!("Now we need to get all the mana symbols throughout your deck (not just in the cmc, but in the cards abilities as well).");
let symbol_counts = get_symbol_counts();
println!(
"Your average CMC is: {}",
f64::from(total_cmc) / f64::from(deck_size)
);
println!("You should have {} total land", total_lands);
// FIXME: rename to fit game nomenclature
let land_percentage = f64::from(total_lands) / f64::from(spells);
for (&symbol_count, &color) in symbol_counts.iter().zip(COLORS.iter()) {
if symbol_count > 0 {
// FIXME: rename to fit game nomenclature
let land_count = land_percentage * f64::from(symbol_count);
println!("{} of those lands should be {}", land_count, color);
}
}
}
Changes I made:
rename i
to color
;
rename symbol_count
to symbol_counts
, since it's a vector;
use .copied()
, so that the vector stores the colors directly,
rather than references to elements within the local array;
promotes colors
to a global const
;
use a separate function to read the data;
remove the .0
and .1
, and store colors only once;
use Vec::with_capacity
to avoid unnecessary reallocations.
I also reported the result as a floating point number, since they may be helpful for, say, probabilistic analysis. This is purely subjective.
Putting everything together:
use std::io;
const COLORS: [&str; 6] = ["white", "blue", "green", "red", "black", "colorless"];
fn main() {
let deck_size = get_deck_size();
let spells = get_spell_count(deck_size);
let total_cmc = get_total_cmc();
calculate_mana(deck_size, spells, total_cmc);
}
fn input(prompt: &str) -> Result<u32, std::num::ParseIntError> {
println!("{}", prompt);
let mut input = String::new();
io::stdin()
.read_line(&mut input)
.expect("cannot read input");
input.trim().parse()
}
fn get_deck_size() -> u32 {
loop {
match input("How many cards are in your deck?") {
Ok(deck_size) if deck_size >= 40 => return deck_size,
Ok(_) => println!("The minimum number of cards in a deck is 40."),
Err(_) => println!("Invalid number."),
}
}
}
fn get_spell_count(deck_size: u32) -> u32 {
loop {
match input("How many spells are in your deck?") {
Ok(spell_count) if spell_count <= deck_size => return spell_count,
Ok(_) => println!("You cannot have more spells than cards."),
Err(_) => println!("Invalid number."),
}
}
}
fn get_total_cmc() -> u32 {
loop {
match input("What's the total converted mana cost of all your spells?") {
Ok(total_cmc) => return total_cmc,
Err(_) => println!("Invalid number."),
}
}
}
fn get_symbol_counts() -> Vec<u32> {
let mut symbol_counts = Vec::with_capacity(COLORS.len());
for color in COLORS.iter().copied() {
let symbol_count = loop {
// FIXME: could be written more clearly
print!("How many {} symbols are in the deck?", color);
match input("") {
Ok(symbol_count) => break symbol_count,
Err(_) => println!("Invalid number."),
}
};
symbol_counts.push(symbol_count);
}
symbol_counts
}
fn calculate_mana(deck_size: u32, spells: u32, total_cmc: u32) {
let total_lands = deck_size - spells;
println!("Now we need to get all the mana symbols throughout your deck (not just in the cmc, but in the cards abilities as well).");
let symbol_counts = get_symbol_counts();
println!(
"Your average CMC is: {}",
f64::from(total_cmc) / f64::from(deck_size)
);
println!("You should have {} total land", total_lands);
// FIXME: rename to fit game nomenclature
let land_percentage = f64::from(total_lands) / f64::from(spells);
for (&symbol_count, &color) in symbol_counts.iter().zip(COLORS.iter()) {
if symbol_count > 0 {
// FIXME: rename to fit game nomenclature
let land_count = land_percentage * f64::from(symbol_count);
println!("{} of those lands should be {}", land_count, color);
}
}
}
Answered by L. F. on December 4, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP