I somewhat over-engineered the solution for Dec 2, 22 of Advent of Code : https://github.com/pmeier/aoc-2022-rust/blob/main/src/bin/02.rs
I'm trying to focus on writing idiomatic Rust as if this was not an one-off task, but rather a production library. Relatively new to Rust, so I'm looking for all kind of improvements even nitpicks if something can be done cleaner. Thanks in advance!
Your link leads to a 404, maybe your repo is private?
My bad, sorry. Should work now.
That is quite beautifully over-engineered indeed! Overall, looks like normal rust code to me (besides the fact that you use too many types for such a small problem). Our solutions actually look somewhat similar.
This line unwraps your StrategyParseError, which kind of defeats your error handling. I'd try to propagate the error to main, making your library functions (everything except main) free of panics. Instead of solve returning Option<u32>, I'd return Result<u32, StrategyParseError> like this:
fn solve<P>(input: &str, parse: P) -> Result<u32, StrategyParseError>
where
P: Fn(&str) -> Result<Score, StrategyParseError>,
{
let mut sum = 0;
for line in input.lines() {
sum += parse(line)?.value;
}
Ok(sum)
}
Also a great tool if you want to know if your code is idiomatic is rust's linter, clippy. I recently started using it in pedantic mode and discovered a lot I could improve. Simply run cargo clippy to see what lints your code is violating. If you want see more aggressive lints, run cargo clippy -- -W clippy::pedantic, or, if you would like to return errors instead of warnings (useful for your CI pipeline, for example), run cargo clippy -- -D warnings -D clippy::pedantic. Just note that pedantic lints can contain false positives (e.g. lot's of #[must_use]).
aargh, i always forget that one can do neat matches on patterns/what's that called like tuples, not one single value only. it bautifully shortens the code e.g. on your line here, instead of two nested matches in my code here... 
One thing that jumps out to me is the nested matches in
You could clean that up a ton by doing something like
use HandShape::*;
let outcome = match (player, opponent) {
(Rock, Rock) | (Paper, Paper) | (Scissors, Scissors) => Outcome::Draw,
(Rock, Scissors) | (Scissors, Paper) | (Paper, Rock) => Outcome::Win,
(Scissors, Rock) | (Paper, Scissors) | (Rock, Paper) => Outcome::Defeat,
};
EDIT: ... err, like RustyJoeM was also typing, apparently.
Thanks a ton @jofas!
Fair point although there is a caveat: with the template I'm using, I only have to implement part_one and part_two and thus never touch the main function. Since they have a predefined Option<u32> return type, the only thing I can do is panicking in there. I've moved the unwrap into the outer most functions that I "have control over".
Maybe I'll adjust the template to account for that going forward.
Thanks for the hint! I am aware of and already using clippy, but the pedantic mode is new to me. TBH, I've never read the documentation, but saw cargo clippy somewhere and figured that was all there is to it.
I've run with -W clippy::pedantic -A clippy::must_use_candidate, because I could hardly find the "real" lints in there. In my code, I didn't find a single case where this was useful, but that might be different for larger code bases. I'll keep that in mind.
Apart from your suggestions, I also learned two new things from looking at your code:
clippy didn't suggested it.unreachable!(...) macro is what I needed before I added the First and Second enums. That seems really helpful without compromising readability.Thanks again! I've added all suggestions in this PR.
Apart from the tuple matching, I particularly like the
use HandShape::*;
beforehand. In case the scope is narrow, this removes quite a bit of noise. Thanks for the suggestion!
As small nit on your implementation though:
let outcome = match (player, opponent) {...};
should probably be
let outcome = match (&player, &opponent) {...};
It seems just using match borrows automagically, but putting the values into a tuple moves them.
A couple of YMMV things related to
For a short function like that, you could consider just letting inference know what to parse, since it knows it from your return type:
-Ok((columns[0].parse::<First>()?, columns[1].parse::<Second>()?))
+Ok((columns[0].parse()?, columns[1].parse()?))
You could consider using slice patterns instead of indexing to check for exactly two things:
let [first, second] = &columns else {
return Err(StrategyParseError(StrategyParseFailure::NumColumns(columns.len())));
};
Ok((first.parse()?, second.parse()?))
Alternatively, you could consider avoiding the Vec altogether, via something like
let mut columns = s.split_whitespace().fuse();
let (Some(first), Some(second), None) = (it.next(), it.next(), it.next())
else { return Err(... wrong number of columns ...); };
Ok((first.parse()?, second.parse()?))
Yeah, locally the cost of * is substantially reduced.
Depending how domain-critical and distinctively-named the enum variants are, it can sometimes even make sense to non-publicly use them at file scope, though it's probably better to not use * for that, and list them out instead.
Actually, instead it should be
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
enum HandShape {
Rock,
Paper,
Scissors,
}
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
enum Outcome {
Win,
Defeat,
Draw,
}
There's no point in dealing with the borrow checker for trivial 1.585-bit values. Make them Copy.
Super-unimportantly, I'd suggest Win/Draw/Loss instead of using Defeat. That's the opposition of terms I usually see (see Win–loss - Wikipedia), and conveniently gives distinct first letters.
You might even consider
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
enum Outcome {
Loss = -1,
Draw = 0,
Win = 1,
}
impl std::ops::Not for Outcome {
type Output = Self;
fn not(self) -> Self {
match self {
Self::Loss => Self::Win,
Self::Draw => Self::Draw,
Self::Win => Self::Loss,
}
}
}
This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.