Hey,
I would love to get some feedback on my first guess game implementation, focusing on testing.
@steveklabnik mentioned in the other post that
I'd flag it in code review if this were a "real" project.
So I'm curious to get some feedback and learn more on where I went wrong and what are some better ways to do it.
One of the biggest concern in the example for me is the main loop (which is currently untested and has at least 2 bugs actually).
Thanks in advance.
I guess I should comment given that my comment started this 
First issue: https://github.com/dnagir/rust_guessing_game/blob/master/src/main.rs#L16 accepting a Result this way feels bad to me. I'm also not a fan of inner functions. It feels like this is doing too much.
"process" is an extremely meaningless word. The two functions don't really say what they do.
format_guess feels it should be a From implementation to me. Or maybe Display.
Testing the literal errors here: https://github.com/dnagir/rust_guessing_game/blob/master/src/main.rs#L129-L135 feels fragile and error-prone.
Those are some few, short thoughts
There's no reason for GuessResult to not derive things like Copy, Clone, and Eq (Copy and Clone are especially useful).
While I really like match statements, use conditionals when appropriate. In lines 18-21, instead of:
match num {
x if x < number_to_guess => Bigger,
x if x > number_to_guess => Smaller,
_ => Guessed,
}
I'd just write:
if x < number_to_guess {
Bigger
} else if x > number_to_guess {
Smaller
} else {
Guessed
}
Alternatively, you could use cmp directly and match on Ordering but that's not necessarily an improvement.
In line 29, use map instead of and_then:
.map(|num| process_int(num, number_to_guess)) // Although I'd take steveklabnik's advice here and avoid the inner function (just use a closure).
I don't really see the value of the game_over function. Do you expect there to be a game over condition other than result == Guessed? This is also a case of a gratuitous match statement; the entire function body could be result == Guessed. Finally, after deriving Copy for GuessResult, you can just pass it by-value. Passing plain-old-data (especially enums like this) by-value is considered idiomatic in rust.
On line 64, you don't need that closure; just pass the function directly.
In lines 54-55, it's idiomatic to shadow the input variable here instead of creating a new lock variable.
I think it can be split into 2 functions: fetch_a_guess(io::Result) -> String and calculate_guess_result(String) -> GuessResult.
The problem here is that fetch_a_guess is accepting Result (reading from stdin), so in order to return a String there would have to be an unwrap.
That means we would have to assume the stdin read always succeeds.
How would you advise to deal with it?
They aren't literal errors at all. Those are the messages the the user will see, depending on what they type and deserve to be tested as this is the most user-facing part of the app.
In which way are those test fragile?
Thanks, I was fighting this in my head thinking of not applying too many derives.
Good to know that it's fine to do that as it would make my life a little easier in at least one other place (no need to pass by reference).
Yeah, I was reading on this but the match looked so much more readable to me because as I can see all conditions very easily.
Would if...then...else be more idiomatic Rust because of performance or other technical considerations? Or is it only because the match is being "abused"?
The actual docs on match state:
Often, a simple if/else isn’t enough, because you have more than two possible options. Also, conditions can get quite complex. Rust has a keyword, match, that allows you to replace complicated if/else groupings with something more powerful.
So it seems like appropriate use of match here.
How would you use it here?
map just converts a Result<A, E> into Result<B,E>. I'd still need to match on Ok/Err - back to square 1.
Yeah, that's the idea. The game_over is an important part of the application that I think deserves to have at least a function even if it's simple now. We may also say that game is over after a maximum number of attempts or similar.
But yeah, *result == Guessed is definitely more concise. I was too hooked on the matching business 
Good points. Thanks you 
It would be more idiomatic because you're not actually using pattern matching (the purpose of match). Here, you have an if-else chain disguised as a match.
and_then(...) does the same thing (converts Result<A, E> into Result<B, E>). The difference is that f in .and_then(f) must return a result of type Result<B, E> (could fail with Err(...)) and g in .map(g) directly returns a value of type A (can't fail).
So, if you use map, you won't need the extra Ok around the process_int call.
I usually try to split out error handling and actual computation for things like this. So I'd want to address the error before passing it into the function.
That they're actual output messages means that this should be a Display impl. They're fragile in the sense that they don't really buy you much; they just mirror the implementation. I find such tests to be a waste, personally, YMMV.
Thanks guys for the help.
I'm not convinced that we shouldn't test the string formatting (whether it's a Display imp or separate function - doesn't matter as much).
Any test is a duplicate of the implementation one way or another. Can then suggests that any test is a duplication and as such no need to write any tests
But at this stage my biggest struggle is testing the main loop.
Would appreciate the advise on how this can be done efficiently?
Thanks.