bowling: be more explicit about when errors should happen #536

petertseng · 2017-02-06T08:50:06Z

The JSON file currently says "When to error is also left up to your
implementation", but we should instead be explicit.

Let's say I roll 5, then 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.

This commit proposes that invalid rolls be made explicit right when the
roll is made, not at some unspecified time in the future (when
attempting to take the score of the game). This is done via a
roll_should_fail key that is mutually exclusive with the expected
key.

Previous discussions I know of (since I was involved):

ErikSchierboom · 2017-02-07T11:53:11Z

I think it is desirable to fail as soon as possible, as in general that is a good strategy to follow. How to specify it, I'm not sure. That's a bit harder I think.

petertseng · 2017-02-12T21:26:55Z

OK, nobody gave a better suggestion, so I'm rolling with roll_should_fail.

petertseng · 2017-02-21T02:58:20Z

exercises/bowling/canonical-data.json

+    "If the `expected` key is present and non-negative",
+    "all rolls should succeed, and the final score should equal that value.",
+    "",
+    "If the `expected` key is present and negative,",


This is in violation of #401 and #551. I would like to suggest the errors from #240 be used.

petertseng · 2017-02-21T03:05:15Z

OK, now using the error object of #401.

The JSON file currently says "When to error is also left up to your implementation", but we should instead be explicit. Let's say I `roll 5`, then `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. This commit proposes that invalid rolls be made explicit right when the roll is made, not at some unspecified time in the future (when attempting to take the score of the game). This is done via a `roll_should_fail` key that is mutually exclusive with the `expected` key. Previous discussions I know of (since I was involved): * exercism/rust#213 (comment) * exercism/haskell#440

rbasso · 2017-02-21T03:37:03Z

Just a comment, that shouldn't prevent merging this:

The fact that you are proposing to use a shouldFail or an expected that could contain an error (two distinct encodings for exceptional conditions) suggests that restricting error to be a string is forcing alternative encodings of ~~error~~ exception-related data.

ErikSchierboom · 2017-02-21T08:06:55Z

The fact that you are proposing to use a shouldFail or an expected that could contain an error (two distinct encodings for exceptional conditions) suggests that restricting error to be a string is forcing alternative encodings of error exception-related data.

I think that's because there are two methods being tested here:

Recording a roll.
Getting the score.

With the PR almost being merged, we could be using the property field to indicate that a different property is being tested, right?

petertseng · 2017-02-21T08:43:05Z

So you are suggesting that the tests look something like the below. I gave one example for each of the three different cases represented:

score succeeds
score fails because game is incomplete
roll fails because it is invalid
not explicitly represented in current test cases: roll succeeds (since all rolls in previous_rolls are defined to succeed). We could add such a case - the expected value will probably just be null since what is there to return, really

    [ { "description"   : "game with no strikes or spares"
      , "property"      : "score"
      , "previous_rolls": [3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6, 3, 6]
      , "expected"      : 90
      }   
    , { "description"   : "can't score incomplete game"
      , "property"      : "score"
      , "previous_rolls": [0, 0]
      , "expected"      : {"error": "something"}
      }   
    , { "description"   : "can't roll more pins than there are on the lane"
      , "property"      : "roll"
      , "previous_rolls": [5] 
      , "roll"          : 6 
      , "expected"      : {"error": "something"}
      }   
    ]

ErikSchierboom · 2017-02-21T08:46:45Z

Actually, shouldn't the last case be:

{ "description"   : "can't roll more pins than there are on the lane"
  , "property"      : "roll"
  , "previous_rolls": [5] 
  , "roll"          : 6 
  , "expected"      : {"error": "something"}
}

With the property being tested set to "roll" instead of "score"?

petertseng · 2017-02-21T09:38:57Z

With the property being tested set to "roll" instead of "score"?

You're right. Edited to be so, so that we are on the same page

ErikSchierboom · 2017-02-21T09:46:41Z

You're right. Edited to be so, so that we are on the same page

I think your three test-cases example looks perfect now! Very clean and understandable. Great!

rbasso · 2017-02-21T10:22:39Z

First of all, let me say that I agree with the idea of returning which of the rolls in the lists in invalid. It is an useful information for the user.

Sorry for getting in the middle of this PR, I was just thinking out too loud. Now, let me explain what I meant.

I think that's because there are two methods being tested here:

Recording a roll.

Getting the score.

This may be where we diverge. I think the exercise is just about scoring a game, so we need to test score, and that is also the only test group in the canonical-data.json.

To fulfill the new requirements of returning which roll is the invalid, this score may return:

A score, for a valid and complete game.
An exceptional value for an incomplete game.
Another exceptional value for an invalid list of rolls, with the information of which roll is the invalid one.

A valid score should be represented normally in expected:

"expected": 42

An exceptional condition would, by convention, be represented as an error:

"expected": { "error": "Incomplete game" }

or

"expected": { "error": "Invalid roll 6 - Cannot score more than 10" }

I was just thinking that, because error was defined as a string in #551, there is not way for us to include more information about the error in a structure way.

Anyway, it seems that this PR has taken a turn, and now the proposal includes also testing an intermediary function. In this case, I think that my original arguments doesn't apply anymore.

ErikSchierboom · 2017-02-21T20:10:38Z

@rbasso I'd also be content with your solution, but I do really like to "fail-fast" in my code, and failing on an invalid roll instead of only failing when calculating a score seemed reasonable to me.

petertseng · 2017-02-21T20:28:05Z

First, I will say that I will propose a new file to match the new schema, which should make clear when the errors are happening.

But not now. Maybe in 12 hours.

Second, I will say that the Haskell track only has a score function returning an Either, with the Left being a type that can either be an invalid roll (specifying precisely which roll was invalid) or an incomplete game. The discussion can be seen at exercism/haskell#440 and the result at https://github.com/exercism/xhaskell/blob/master/exercises/bowling/test/Tests.hs

Now I see what we mean when we say we want a richer error than just "error": <string>": it would be great to have structured data specifying which roll was invalid in the error object. Exception-based languages should be able to make this happen easily too; if this is seen as desirable we should revise the error object.

rbasso · 2017-02-22T04:37:52Z

About when to fail

@rbasso I'd also be content with your solution, but I do really like to "fail-fast" in my code, and failing on an invalid roll instead of only failing when calculating a score seemed reasonable to me.

Think in what I wrote more like a question than a suggestion. By the way, I also prefer to fail-fast most of the times, @ErikSchierboom.

I was just pondering that maybe we are letting details of "how to solve the problem" affect the "how to specify the problem". If the solution gives the correct results, it shouldn't matter how it does it internally.

About more structured expected types

Now I see what we mean when we say we want a richer error than just "error": ": it would be great to have structured data specifying which roll was invalid in the error object. Exception-based languages should be able to make this happen easily too; if this is seen as desirable we should revise the error object.

I just started thinking about the subject after we added the restrictions from #551 in #602, @petertseng, but maybe the problem can be generalized a little. Maybe error is just a special case of the more general problem of returning different things depending on the input data.

Let's say we have a function called feedShark, which can return one of the following (a sum type), depending on the input food:

Case 1 - a boolean indicating if it has eaten the food.
Case 2 - an exceptional value, indicating that the object is something too big to even think about feeding it.
Case 3 - another exceptional value, indicating that the feeder has lost a body part in the proccess, also indicating which part was eaten.

We have a lot of different ways to encode that in JSON:

[ { "description": "Try to feed electronics"
  , "property"   : "feedShark"
  , "food"       : "laptop"
  , "expected"   : false
  }
, { "description": "Try to feed a big animal"
  , "property"   : "feedShark"
  , "food"       : "elephant"
  , "expected"   : "Impossible"
  }
, { "description": "Try to feed vegetables"
  , "property"   : "feedShark"
  , "food"       : "lettuce"
  , "expected"   : { "feederEaten": "right arm" }
  }
]

We could also be more descriptive about the cases, like this:

[ { "description": "Try to feed electronics"
  , "property"   : "feedShark"
  , "food"       : "laptop"
  , "expected"   : { "eaten": false }
  }
, { "description": "Try to feed a big animal"
  , "property"   : "feedShark"
  , "food"       : "elephant"
  , "expected"   : { "impossible": {} }
  }
, { "description": "Try to feed vegetables"
  , "property"   : "feedShark"
  , "food"       : "lettuce"
  , "expected"   : { "feederEaten": "right arm" }
  }
]

We could even flatten everything, maybe for readability:

[ { "description": "Try to feed electronics"
  , "property"   : "feedShark"
  , "food"       : "laptop"
  , "eaten"      : false
  }
, { "description": "Try to feed a big animal"
  , "property"   : "feedShark"
  , "food"       : "elephant"
  , "impossible" : {}
  }
, { "description": "Try to feed vegetables"
  , "property"   : "feedShark"
  , "food"       : "lettuce"
  , "feederEaten": "right arm"
  }
]

Edit: Fixed the example so that they make sense!

`git diff -w` on this commit should be mostly clean. In preparation for new schema.

This makes clearer that the rolls are assumed to happen prior to the test at hand.

The transformation is simple: Any case with a "roll_should_fail" turns into a "property": "roll" case with an appropriate error. All other cases turn into a "property": "score" case.

petertseng · 2017-02-22T05:58:00Z

Same old place.

https://github.com/petertseng/x-common/compare/verify...petertseng:verify-roll-should-fail?expand=1

petertseng · 2017-02-22T05:59:19Z

The unindent is inevitable to match the new schema. But that's why it's in its own commit, so that it is easily seen that that commit only unindents (and removes one level of nesting by removing "score": key.

ErikSchierboom · 2017-02-22T07:27:03Z

IMHO, the current version is great. What do you think @rbasso? Perhaps we should move the discussion about return values to a separate issue?

rbasso · 2017-02-22T07:50:11Z

IMHO, the current version is great. What do you think @rbasso?

I'm usually against testing for intermediary functions, but I never solved this exercise and I trust that you have your reasons. 👍

Perhaps we should move the discussion about return values to a separate issue?

It was just an idea...I guess I'm spending too much time with JSON. 😄

ErikSchierboom · 2017-02-22T08:09:18Z

@rbasso Well, the problem description requirements actually explicitly mention two operations to be implemented:

roll(pins : int) is called each time the player rolls a ball. The argument is the number of pins knocked down.

score() : int is called only at the very end of the game. It returns the total score for that game.

:)

rbasso · 2017-02-22T10:34:30Z

Well, the problem description requirements actually explicitly mention two operations to be implemented

You are indisputably right about that. 😄

petertseng · 2017-02-23T05:06:07Z

Sorry, no squash since I think the individual changes are easier to understand than a single diff that purports to change the entire file (it really doesn't)

I was a bit selfish and only served my specific goals for this PR: The goal of expressing in some way that a roll should fail fast (as soon as it is known to be erroneous).

There are other goals that are still worthy, even though I left them aside for this PR. I kept the scope small for this PR, but these are worth their own discussion:

More structured data in `error`

This allows tracks to easily check whether the right kind of error is given. This seems easy enough to implement for a few tracks: Those with sum types can check that the constructor, and those with exceptions can check the class of the exception.

From what I know, the Swift track is very diligent about doing this. I don't have time to show y'all all the examples, but:

Other tracks don't do that as often. I explained in #401 (comment) that e.g. Rust merely checks Result#is_err and Haskell merely checks Data.Either.isLeft (with bowling being a unique case where the error type is checked!)

If more tracks are interested in doing this, it is certainly possible. It may be good to see whether there is any fixed schema that will relieve us from having to write exercise-specific code for each exercise's errors.

Bowling overspecifying

The https://github.com/exercism/x-common/blob/master/exercises/bowling/metadata.yml tells us this exercise hails from http://butunclebob.com/ArticleS.UncleBob.TheBowlingGameKata . Now, note that we have already morphed it from its original form since we require checking for errors whereas the original did not.

We may consider further revisions, such as not specifying the internals and only asking "the score resulting from these rolls, or error". For maximum effect it is probably good if the error describes which roll is erroneous (as the Haskell tests do).

I don't oppose that change, but I do believe it is best left until we have a better error.

ErikSchierboom · 2017-02-23T07:21:41Z

🎉

petertseng changed the title ~~bowling: wishlist: be more explicit about when errors should happen~~ bowling: be more explicit about when errors should happen Feb 12, 2017

petertseng mentioned this pull request Feb 13, 2017

canonical-data.json standardisation discussion (was: Malformed data?) #336

Closed

petertseng commented Feb 21, 2017

View reviewed changes

petertseng added 2 commits February 20, 2017 19:05

bowling: Use error object defined in #401

35df72b

petertseng added 4 commits February 21, 2017 21:34

bowling: unindent cases

5fc4608

`git diff -w` on this commit should be mostly clean. In preparation for new schema.

bowling: rename rolls to previous_rolls

1122faf

This makes clearer that the rolls are assumed to happen prior to the test at hand.

bowling: Add score and roll properties

8ddbd7f

The transformation is simple: Any case with a "roll_should_fail" turns into a "property": "roll" case with an appropriate error. All other cases turn into a "property": "score" case.

bowling: Add exercise and version; rename # to comments

a346497

petertseng merged commit 62e8a1a into exercism:master Feb 23, 2017

petertseng deleted the bowling branch February 23, 2017 05:07

petertseng mentioned this pull request May 9, 2017

bowling 1.0.0.2: drop unneeded rolls, edit a description exercism/haskell#531

Merged

britto mentioned this pull request Jun 18, 2017

[Bowling] Update implementation and tests exercism/elixir#347

Merged

ErikSchierboom mentioned this pull request Jul 13, 2021

grade-school: fix student cannot be in multiple grades logic #1819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bowling: be more explicit about when errors should happen #536

bowling: be more explicit about when errors should happen #536

petertseng commented Feb 6, 2017 •

edited

Loading

ErikSchierboom commented Feb 7, 2017

petertseng commented Feb 12, 2017

petertseng Feb 21, 2017

petertseng commented Feb 21, 2017

rbasso commented Feb 21, 2017 •

edited

Loading

ErikSchierboom commented Feb 21, 2017

petertseng commented Feb 21, 2017 •

edited

Loading

ErikSchierboom commented Feb 21, 2017

petertseng commented Feb 21, 2017

ErikSchierboom commented Feb 21, 2017

rbasso commented Feb 21, 2017

ErikSchierboom commented Feb 21, 2017

petertseng commented Feb 21, 2017 •

edited

Loading

rbasso commented Feb 22, 2017 •

edited

Loading

petertseng commented Feb 22, 2017

petertseng commented Feb 22, 2017 •

edited

Loading

ErikSchierboom commented Feb 22, 2017

rbasso commented Feb 22, 2017

ErikSchierboom commented Feb 22, 2017

rbasso commented Feb 22, 2017

petertseng commented Feb 23, 2017 •

edited

Loading

ErikSchierboom commented Feb 23, 2017

bowling: be more explicit about when errors should happen #536

bowling: be more explicit about when errors should happen #536

Conversation

petertseng commented Feb 6, 2017 • edited Loading

ErikSchierboom commented Feb 7, 2017

petertseng commented Feb 12, 2017

petertseng Feb 21, 2017

Choose a reason for hiding this comment

petertseng commented Feb 21, 2017

rbasso commented Feb 21, 2017 • edited Loading

ErikSchierboom commented Feb 21, 2017

petertseng commented Feb 21, 2017 • edited Loading

ErikSchierboom commented Feb 21, 2017

petertseng commented Feb 21, 2017

ErikSchierboom commented Feb 21, 2017

rbasso commented Feb 21, 2017

ErikSchierboom commented Feb 21, 2017

petertseng commented Feb 21, 2017 • edited Loading

rbasso commented Feb 22, 2017 • edited Loading

About when to fail

About more structured expected types

petertseng commented Feb 22, 2017

petertseng commented Feb 22, 2017 • edited Loading

ErikSchierboom commented Feb 22, 2017

rbasso commented Feb 22, 2017

ErikSchierboom commented Feb 22, 2017

rbasso commented Feb 22, 2017

petertseng commented Feb 23, 2017 • edited Loading

More structured data in error

Bowling overspecifying

ErikSchierboom commented Feb 23, 2017

petertseng commented Feb 6, 2017 •

edited

Loading

rbasso commented Feb 21, 2017 •

edited

Loading

petertseng commented Feb 21, 2017 •

edited

Loading

petertseng commented Feb 21, 2017 •

edited

Loading

rbasso commented Feb 22, 2017 •

edited

Loading

petertseng commented Feb 22, 2017 •

edited

Loading

petertseng commented Feb 23, 2017 •

edited

Loading

More structured data in `error`