Skip to content

Implement Bowling #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Oct 26, 2016
Merged

Implement Bowling #213

merged 16 commits into from
Oct 26, 2016

Conversation

IanWhitney
Copy link
Contributor

@IanWhitney IanWhitney commented Oct 2, 2016

Implement the bowling exercise, following the canonical test suite (along with exercism/problem-specifications#418 which has not been merged at the time of this PR)

  • Tests -- details in 03ae9ec
  • Example Implementation
  • Placement in config.json
  • Placement in problems.md
  • Ignore all but first test

game.roll(0);
}

game.roll(11);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about checking that this is_err()?

let game = BowlingGame::new();

for x in (0..21) {
game.roll(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought that the first twenty would be is_ok and the last one would be is_err

game.roll(0);
}

assert!(game.score().is_err());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought that this game can be scored (its score is zero) as soon as the twentieth ball is rolled. The twenty-first ball should not be allowed and should not invalidate the game from being scored.

fn if_the_last_frame_is_a_spare_you_can_not_create_a_score_before_extra_roll_is_taken() {
let game = BowlingGame::new();

for x in (0..17) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that this is 17, the last frame isn't a spare. The ninth frame gets a 5, the tenth frame gets a 5.

fn if_the_last_frame_is_a_strike_you_can_not_create_a_score_before_extra_rolls_are_taken() {
let game = BowlingGame::new();

for x in (0..17) {
Copy link
Member

@petertseng petertseng Oct 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that this is 17, rolling a 10 immediately after this gives us a spare on the ninth frame instead of a strike on the tenth

game.roll(5);
game.roll(5);

assert!(game.score().is_err());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

below, you test that game.roll(10) will afterward allow game.score().is_ok(). Should a similar thing also be tested here?

fn if_the_last_frame_is_a_spare_you_get_one_extra_roll_that_is_scored_once() {
let game = BowlingGame::new();

for x in (0..17) {
Copy link
Member

@petertseng petertseng Oct 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17 zeroes followed by two 5's makes a 5 in the ninth frame and a 5 in the tenth frame, rather than a spare in the tenth.


game.roll(10);

for x in (0..17) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with 17 balls following the strike, the game is not yet finished

game.roll(5);
game.roll(3);

for x in (0..15) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with 2 (the 5 and 3) + 15 = 17 rolls following the strike, the game is not yet finished

game.roll(5);
game.roll(3);

for x in (0..11) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are 6 frames left at this point, so 11 rolls isn't enough for them (0..11 is exclusive of 11 so this rolls 11 balls not 12)

fn if_the_last_frame_is_a_strike_you_get_two_extra_rolls() {
let game = BowlingGame::new();

for x in (0..17) {
Copy link
Member

@petertseng petertseng Oct 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17 doesn't make the last frame a strike, this makes frame 9 a spare (0 then 10)

fn strikes_in_extra_rolls_after_a_strike_in_the_final_frame_do_not_get_the_bonus() {
let game = BowlingGame::new();

for x in (0..17) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17 followed by spare in 9th, strike in 10th, one extra roll - second extra roll hasn't been rolled yet so game wouldn't be complete

fn all_strikes_is_a_perfect_score_of_300() {
let game = BowlingGame::new();

for x in (0..11) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a perfect game is 12 strikes not 11

}

game.roll(5);
game.roll(6);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about checking that this is_err()?

@IanWhitney
Copy link
Contributor Author

IanWhitney commented Oct 2, 2016

In regards to error handling, there seem to be a couple options.

  1. score returns a result.

This is the approach I started with. Most of the boundaries tested deal with the game as a whole, not individual rolls, so I figured it made sense to put the result in the score function.

  1. Have roll return a result

As suggested here. I'm not sure what the idioms of Rust say here, but in my mind roll is a command, so having it return something seems to violate command-query separation

A way we could have roll return something would be to follow the approach we took in robot simulator and make the game immutable. Each call to roll would return a new Result<BowlingGame, Err>.

IanWhitney pushed a commit to IanWhitney/x-common that referenced this pull request Oct 2, 2016
This is based on my work implementing this problem for Rust
exercism/rust#213

I'm addressing a few things here.

- The description at the top is now about implementing the tests, not
about the problem domain.

- I've reordered the test to (hopefully) make things flow more smoothly.
I found that the previous set of tests introduced concepts in kind of a
mix, instead of one at a time.

They are now:

- Score a 0 point game
- Score a game with no spares/strikes
- Spares
  - Simple spare
  - Spare with bonus
  - Spare in last frame
- Strikes
  - Simple strike
  - Strike with bonus
  - Strike in last frame
- Exceptions

- Updated descriptions. My goal here was to try to clearly state what
bit of scoring logic was being tested in each test, without relying too
much on bowling-specific terms. 'strike', 'spare' and 'frame' are
unavoidable, but other terms were replaceable.

- Remove unnecessary tests. There were a few different tests of games
without spares & strikes when only one is really necessary. My goal is
that each test should lead the student to make one change to their code.
If a bunch of tests can pass because of the same change then some of
those tests can be removed.

- Explicit description of arity removed. I tried to cover the expected
API in the description text.

- Expectations of exceptions changed from specific error text to `-1`,
which is what I normally see for exception test definitions.
@petertseng
Copy link
Member

I hope the above options aren't presented as mutually exclusive. It could be the case that both should return Result!

But before we judge whether that should be true, let me state my reasoning and motivations, then we can better decide.

So! Can we fail to take a score? Yes that can fail if the game is not complete yet. So score could return Option<uint>, or Result<uint, E> where the E explains why we can't take the score (the something is likely not checked in the tests; we would likely just test is_err).

Okay, what about individual rolls. Can individual rolls be invalid? It seems so.

  • If I try to roll 11
  • If I roll a 7 in the same frame as a 5 (there aren't that many pins there)
  • If I roll anything after twenty rolls (and there are no extra rolls)

Well, given that there are some invalid individual rolls, how do we deal with them? How is that reflected in the API of this bowling game? Let's say that roll returns nothing (and therefore returns no indication of whether the last roll was valid). So let's suppose that I call roll(5), roll(6), then roll(0) 18 times. Okay, that's 20 rolls, that's a full game, so I can take the score now, right? Now I try to take the score and find that I get an error. Argh! In the absolute best case, the error result from score would be so kind to tell me that my roll(6) way back there was invalid, but in the worst case it might not, and I would be left to my own devices to figure out what happened, since there was no feedback on any of the individual rolls.

I think the general principle I'm trying to discuss is that it is less confusing if errors are reported as soon as possible, rather at some unspecified time in the future. This makes it easier for me to figure out what went wrong, instead of having to figure out the source of a seemingly-unrelated failure to score.

So what does this say about what roll should return? To me I did think this meant it should return Result<(), E> (again, the contents of the E would not be checked during testing).

But nothing I've said about score or roll needs to be set in stone - anything that meets the goals described here would do!

@IanWhitney
Copy link
Contributor Author

If roll returned a Result, what would the test look like? In my mind, they look like:

fn you_can_not_roll_more_than_ten_pins_in_a_single_roll() {
 let game = BowlingGame::new();
 assert!(game.roll(11).is_err());
}

But maybe you have something else in mind.

The advantage, as you mentioned, is that we'd know right away that we have an error.

But, I think that if we went this way then the first time students would see this behavior is in the exception tests. I don't think our earlier tests would have code like:

 for x in (0..20) {
  game.roll(0).expect("Invalid Roll"); //or some other way of handling Result.
}

Or do you think they should?

This approach would also combine two behaviors into one method call: mutating state and asserting if the mutation was successful. There definitely are methods in Rust that do this (write for example), so it's certainly idiomatic. But I think it's also idiomatic to consistently wrap those method calls in something that handles the Result, so we'd want to do it throughout the tests.

@petertseng
Copy link
Member

petertseng commented Oct 3, 2016

assert!(game.roll(11).is_err());`

Yeah, that's what I think as well.

But, I think that if we went this way then the first time students would see this behavior is in the exception tests.

Ah, this is a bit unfortunate. And we can't even write plain game.roll(0) because Result is must_use, so the least intrusive thing we might conceivably do is let _ = game.roll(0)

I remember we had similar conversations in #89.

Is there a way in Rust to have students avoid using a Result value until they reach the test that introduces error handling? Because that's what I think would be best in terms of learning curve. I'd love students' initial implementations to be simple,

That from #89 (comment) seems very cromulent here. Perhaps it's worth revisiting that and seeing whether there's anything that's applicable to this exercise. I don't have the answers, but I will have a think when I can.

@petertseng
Copy link
Member

So I guess what happened back then was a similar thing - after deciding that it was possible for queen creation to fail, but queen creation will be used in all of the other tests, it made sense then to reorder the tests to test queen creation first.

A way we could have roll return something would be to follow the approach we took in robot simulator and make the game immutable. Each call to roll would return a new Result<BowlingGame, Err>

This would be interesting too! A fun part is the act of "roll zero 18 times", taking the previous Result and turning into the next. We could make foldM ! (If you ask me whether I'm serious about that, my answer is always "it's as serious as you'd like it to be")

@IanWhitney
Copy link
Contributor Author

In a hectic pre-release period for a project, so I don't have a bunch of time to think about Bowling right now. I'm thinking the Queen Attack approach makes the most sense here.

@IanWhitney
Copy link
Contributor Author

A couple of thoughts on how to write the tests, now that roll will return a result

Reading the Must Be Used doc that was linked earlier, the tests could read

for x in (0..20) {
  game.roll(0).expect("roll failed");
}

or

for x in (0..20) {
  game.roll(0).is_ok(); //when I tried this in playground it was fine, even though the docs suggest putting this in an `assert!`
}

The let _ = game.roll(0) approach also works, but I don't think it's the way I want to go. I'd rather use the standard Result API.

@petertseng
Copy link
Member

I imagine (but have not verified) that if using game.roll(0).is_ok() and the roll function unexpectedly returns an err, it will go by and not panic, since is_ok only convert it a bool. Whether we are all right with that can depend, but there are probably a few tests for which we will care.

IanWhitney added a commit to exercism/problem-specifications that referenced this pull request Oct 16, 2016
* Update bowling canonical-data

This is based on my work implementing this problem for Rust
exercism/rust#213

I'm addressing a few things here.

- The description at the top is now about implementing the tests, not
about the problem domain.

- I've reordered the test to (hopefully) make things flow more smoothly.
I found that the previous set of tests introduced concepts in kind of a
mix, instead of one at a time.

They are now:

- Score a 0 point game
- Score a game with no spares/strikes
- Spares
  - Simple spare
  - Spare with bonus
  - Spare in last frame
- Strikes
  - Simple strike
  - Strike with bonus
  - Strike in last frame
- Exceptions

- Updated descriptions. My goal here was to try to clearly state what
bit of scoring logic was being tested in each test, without relying too
much on bowling-specific terms. 'strike', 'spare' and 'frame' are
unavoidable, but other terms were replaceable.

- Remove unnecessary tests. There were a few different tests of games
without spares & strikes when only one is really necessary. My goal is
that each test should lead the student to make one change to their code.
If a bunch of tests can pass because of the same change then some of
those tests can be removed.

- Explicit description of arity removed. I tried to cover the expected
API in the description text.

- Expectations of exceptions changed from specific error text to `-1`,
which is what I normally see for exception test definitions.
@IanWhitney
Copy link
Contributor Author

I imagine (but have not verified) that if using game.roll(0).is_ok() and the roll function unexpectedly returns an err, it will go by and not panic, since is_ok only convert it a bool. Whether we are all right with that can depend, but there are probably a few tests for which we will care.

This is correct

I think I can get this finished up today, now that the new canonical tests are merged into master.

IanWhitney pushed a commit to IanWhitney/xrust that referenced this pull request Oct 16, 2016
Errors
----

I think I caught all of the logical errors in my use of ranges. A second
set of eyes checking that will be much appreciated

Ordering
----

This mostly follows the new canonical_data.json file. I've moved some of
the exception tests up front so that students can quickly handle the
broad edge cases of the problem

- Rolls return results
- Score return results
- A single roll can't be more than 10
- A game needs 10 frames to be OK

The weirder edge cases are saved until the end as I don't think they
need to be handled up front.

Handle results
----

Based on the conversation here
exercism#213 (comment)

I think a bare `is_ok` is fine for all of the rolls that we expect be
ok. We could wrap it in `assert!`, but I think that is noisy as it
indicates we're testing something that we're not actually testing.
@IanWhitney
Copy link
Contributor Author

Ok. If those tests look good I can maybe finally get around to implementing this dang thing.

let game = BowlingGame::new();

for _ in 0..11 {
game.roll(0).is_ok();
Copy link
Member

@petertseng petertseng Oct 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest asserting is_err on ones rolled after the tenth frame. Since we are going to with erroring in roll as soon as we see an erroneous roll, and a roll after the tenth (when tenth wasn't strike or spare) is indeed erroneous

let game = BowlingGame::new();

for _ in 0..10 {
game.roll(0).is_ok();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this and others - the bool from is_ok is being discarded, and I think this is confusing because it makes the intent unclear. If we're not going to check whether it's OK or not, it seems clearer to say let _ = game.roll(0); so that there is no confusion about intent.

@petertseng
Copy link
Member

petertseng commented Oct 21, 2016

I think this gets into requirements beyond the problem. If we had an undo function, for example, we'd want to test that it worked.

Ah, you're right. undo certainly would increase the scope.

The proposed test, then, without undo:

assert!(game.roll(11).is_err());
assert!(game.roll(0).is_ok());

and/or:

let _ = game.roll(5);
assert!(game.roll(6).is_err());
assert!(game.roll(0).is_ok());

Such a test ("error followed by OK") is currently not expressible in canonical-data.json; it's never specified what an erroneous roll does to the game state. So it would still expand the scope.

@IanWhitney IanWhitney changed the title WIP: Implement Bowling Implement Bowling Oct 22, 2016
@IanWhitney
Copy link
Contributor Author

Is this good to go? There's the one example function I could clean up, but I can also live with it being in the example code.

true
}
} else {
true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this line (corresponding to "bonus done, but is neither a spare nor a strike") to be unreachable. Correspondingly, I'd prefer unreachable! to express that intention. What say you?

if self.is_spare() {
self.bonus_score() <= 10
} else if self.is_strike() {
if let Some(first) = self.bonus.iter().rev().last() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why .rev().last() instead of .next()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I thought the function was called first, which it's not. Thanks.

@petertseng
Copy link
Member

I was hoping work could be done on bonus_valid, but unfortunately the only improvement I could make was to move the bonus_done to own line:

        if !self.bonus_done() {
            return true;
        }   

        if self.is_spare() {
            self.bonus_score() <= 10
        } else if self.is_strike() {
            ...
        } else {
            unreachable!("Neither a spare nor a strike?");
        }  

I'm not sure that's even an improvement anyway so maybe we should let it be. I'll leave that one up to you.

I do think that unreachable should go there though.

Other than that I have nothing else to change. I look forward to working on this one.

self.bonus_score() <= 10
}
} else {
true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upon review, it appears that this line is also unreachable. Or at least, it is never reached in our tests. So, should it also become an unreachable? Maybe the if let should become an unwrap? Again, my motivation in making this suggestion is how the intent can be understood when reading the code. If I see true, I start to wonder when it will happen, and in code with this many branches it becomes more taxing on my feeble mind.

These tests basically follow the canonical tests, with some exceptions.

Exceptions
------

I've changed the descriptions to try to clarify what condition/rule/logic each test is trying to cover.

I've reordered the tests. The new order:

- Simplest case (all zeroes)
- Open frames
- Spares in non-final frame
- Spares in final frame
- Strikes in non-final frame
- Strikes in final frame
- Errors

I've left out some tests, for a couple of reasons:

- Won't work in Rust (e.g., passing a signed int to a function that
expects unsigned -- won't compile)
- Duplicative (e.g., multiple open frame tests that all exercise the
same logic)

I've altered test inputs from the canonical inputs. This is mostly in
the tests around bonus points for strikes/spares. The canonical tests
include rolls that aren't relevant to the logic being tested. I have
left those rolls out so that the students can focus on the important
inputs.
Errors
----

I think I caught all of the logical errors in my use of ranges. A second
set of eyes checking that will be much appreciated

Ordering
----

This mostly follows the new canonical_data.json file. I've moved some of
the exception tests up front so that students can quickly handle the
broad edge cases of the problem

- Rolls return results
- Score return results
- A single roll can't be more than 10
- A game needs 10 frames to be OK

The weirder edge cases are saved until the end as I don't think they
need to be handled up front.

Handle results
----

Based on the conversation here
exercism#213 (comment)

I think a bare `is_ok` is fine for all of the rolls that we expect be
ok. We could wrap it in `assert!`, but I think that is noisy as it
indicates we're testing something that we're not actually testing.
The changes to the for loops resolve warnings thrown by the compiler.
Since we're not using immutable games, I think the expectation here is
that `roll` will mutate the game in some way, so `game` should be
mutable in the tests.
Binding the return to _ is an idiomatic way to show that the return value is unimportant. 

exercism#213 (review)
This passes the tests. I want to work on it further before it becomes the official example, but getting working code in place will let us talk about what techniques the solution will require & where this exercise should be placed.
Based on exercism/problem-specifications#418

These tests should capture all of the edge cases of validating the two
balls that can come after a strike in the final frame
Other than the very hairy bonus validation code, I'm OK with this
example.

The approach of calling `add_roll` on every frame can be seen as
overkill, but I think it's the frame's responsibility to know what to do
with a roll, not the game's responsibility to decide which frame to add
a roll to. Yeah, it's always going to be the current frame & previous
frame, I realize. But adding it to each frame reads cleaner.
The Rust techniques necessary to solve bowling are pretty
straightforward. It's mostly just iterator functions, addition and a
Result. But the modeling of the domain is tricky. I think this pairs
well with Queen Attack, which is similarly easy to write but hard to
model.
With the unreachable branch marked
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants