Update bowling canonical-data #391
Conversation
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.
| "Students should implement roll and score methods.", | ||
| "Roll should accept a single integer.", | ||
| "Score should return the game's final score, when possible", | ||
| "For brevity the tests all the rolls in an array;", |
There was a problem hiding this comment.
This sentence might be missing a word, and I can't figure out what it should be. Holds? Keeps? Stores?
There was a problem hiding this comment.
the tests contain all the rolls in an array, probably.
| "For brevity the tests all the rolls in an array;", | ||
| "each element of the rolls array should be passed to the roll method", | ||
| "The final tests define situations where the score can not be returned", | ||
| "The expection for these tests is -1, indicating an exception", |
There was a problem hiding this comment.
Perhaps s/exception/error state. Some languages will implement this as an exception, but others might not (e.g. Go doesn't have exceptions).
| }, { | ||
| "description": "should not allow two normal rolls better than strike in last frame", | ||
| "rolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 6], |
There was a problem hiding this comment.
any value in keeping these? it did catch an oversight in my code.
There was a problem hiding this comment.
If these tests caught an edge case, then they should be kept. Thanks.
There was a problem hiding this comment.
I was indeed treating the bonus rolls in a separate place from the normal ten frames, so I do say it is worth keeping.
| }, { | ||
| "description": "should not allow rolls after the tenth frame", | ||
| "description": "A game with more than ten frames can not be scored", |
There was a problem hiding this comment.
The change in the description seems to change the implication of where the error should occur.
Before, to me this sounds like: The roll should be rejected before there is any attempt to take the score.
After, it sounds like: The roll should be accepted, and only when taking the score do we realize there is a failure.
And I definitely think the roll should be rejected as soon as possible.
But this is a difficult proposition when the only expectation this test file expresses is the score at the end, instead of noting where the error should occur. That's a difficult concept to express though. The first idea that comes to mind (and the idea may be crazy) is:
"rolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
"roll_should_fail": 0,
If the idea is accepted, it can also be used for the [5, 6] roll.
There was a problem hiding this comment.
For those of you who aren't Peter, we've been talking about this in the Rust PR as well.
For Rust, at least, we're going to have the roll method fail if the roll is invalid.
There was a problem hiding this comment.
I can update the description to talk about this decision. I think the json file can mention it, but leave it up to the implementation.
There was a problem hiding this comment.
I can update the description to talk about this decision. I think the json file can mention it, but leave it up to the implementation.
Seems good.
exercism#391 (comment) And spelled idiomatic idiomatically
Since it did catch at least one edge case exercism#391 (comment)
|
I think I've covered the comments to date. |
Corrected numbering for step 5
This is based on my work implementing this problem for Rust
exercism/rust#213
I'm addressing a few things here.
about the problem domain.
I found that the previous set of tests introduced concepts in kind of a
mix, instead of one at a time.
They are now:
Some other changes:
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.
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.
API in the description text.
-1,which is what I normally see for exception test definitions.