Skip to content
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

bowling: should we be precise about which roll, specifically, is erroneous? #440

Closed
petertseng opened this issue Nov 22, 2016 · 16 comments
Closed
Labels

Comments

@petertseng
Copy link
Member

petertseng commented Nov 22, 2016

Take, for example, the sequence of rolls [5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

In this, we would expect the 6 is the erroneous roll. By this, we mean that if game = roll 6 (roll 5 bowlingStart), no matter what you do to game you cannot get a score out of it.

The tests don't test that precisely - they currently cannot, since roll returns a Bowling, and there is no way to determine the above-described property of a Bowling without trying to roll it to completion.

So my question is: Would we like to be more precise about which roll is erroneous?

(I have argued the same in the Rust implementation)

The gist is that I like to fail fast and close to the source of the error, so that it is easy to understand what caused the error.

If we find this desirable, some proposals to do it:

  • Add isValid :: Bowling -> Bool. I find this a little unclean.
  • Change roll's type to roll :: Bowling -> Int -> Either ___ Bowling (the ___ can be any user-defined error they like; the tests only need to test isLeft on it)

And a final question:

Should this be brought to x-common? Note that there will be a slight challenge in how to represent the "this specific roll should fail" in JSON (my current leading thought is keeping the rolls array but then adding a roll_should_fail entry)

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

Disclaimer: I didn't solve this exercises yet in any language.

I don't like to much the idea of having an isValid function, but you proposal to change the roll type seems sound, @petertseng.

If you want to be flexible in the type of error, maybe it can be generalized to any MonadError m e, but I never tried to write a test suite like this one before.

I would start with the rigid type Int -> Either String Bowling, because we'll need to supply a signature anyway, but making it more general is always nice. 😄

The only problem that I see in this approach is that we may be restricting too much the way to calculate the final score. Considering that we don't score partial games, is it hard to justify an interface that forces the student to partially evaluate it.

I'm divided in this one. The way it is now, it is impossible to know where it fails; changing the interface we may loose diversity.

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

My bad. The tests already expect roll to be a function to fold rolls, so my argument makes no sense at all. 😁.

Considering that, I support your proposal, @petertseng.

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

I imagine that you are also changing score, right?

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

I had a hard time trying to discover why I had the feeling that something was strange in this exercise!

Now that I had some more time to think about it, I'll try to make a proper comment. 😄

About x-common

In x-comon there is something about a method roll that "should accept a single integer" and another one called score, that "should return the game's final score". Clearly, it was written with OOP in mind, and that can be confirmed just by looking at the title of exercism/problem-specifications#73.

Because of that, instead of not testing for intermediary functions, this object-oriented specification seems to promote testing just for intermediary functions. A more direct specification would probably only ask for a score function, to be applied directly to rolls.

So, considering what was discussed in exercism/discussions#41 and exercism/discussions#44, I think that the specification is over-prescriptive and deserves to be changed. Of course, I'm saying this from the perspective of a Haskell enthusiast.

About our current implementation

It is hard to transform object-oriented programming in functional programming, but I would risk saying that @abo64's implementation of bowling was correct and a faithful translation of what we have in x-common.

That said, it unavoidably suffers a little from over-prescriptiveness, as the source.

About the proposed changes

I consider your proposal to change the signature of roll a good improvement, @petertseng, so I support the change.

But before following that path, I would like to question if we should keep testing roll or diverge from x-common.

An alternative

What do you guys think about these signatures?

score :: [Int] -> Maybe Int
score :: [Int] -> Either String Int

We could take @petertseng's idea of ignoring the contents when it is a Left. Of course, we could also demand a somewhat more elaborated error handling:

data BowlingError = IncompleteGame
                  | InvalidRoll (Int, Int)

score :: [Int] -> Either BowlingError Int

@abo64
Copy link
Contributor

abo64 commented Nov 23, 2016

Actually the Scala version uses Either, but only a wrapper Error around a String. And the error is never checked in the tests, so I thought a simple Maybe would be enough.
Having said that: @rbasso , your BowlingError suggestion (and then of course checking the Left properly in the tests) looks most convincing to me. But we should clearly state what the two InvalidRow parameters mean (perhaps with record notation?).

@abo64
Copy link
Contributor

abo64 commented Nov 23, 2016

ok, then I will implement these changes within the next few days.

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

I guess it would be nice to wait a little until @petertseng says what he thinks about it.

@petertseng
Copy link
Member Author

Don't wait for me too long while I still have limited access (but in this case y'all got lucky)

So I think score :: [Int] -> Either BowlingError Int was the prevailing suggestion? This looks good. Can you clarify what the two InvalidRoll parameters are? The index of the invalid roll, and then the roll itself? That seems good (anything that at least indicates which roll is invalid will be good, you won't need to wait for me)

@abo64
Copy link
Contributor

abo64 commented Nov 23, 2016

sorry @petertseng I was too quick here. Something like

data BowlingError = IncompleteGame
                  | InvalidRoll { rollIndex :: Int, rollValue :: Int }

?

@petertseng
Copy link
Member Author

Seems good if that's what you were thinking. Happy to defer to @rbasso if has a different idea, but it was my best guess from seeing two ints.

@abo64
Copy link
Contributor

abo64 commented Nov 23, 2016

And it was my best guess, too. :-)

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

I'm fine with either, ignoring the Left content or demanding invalid roll's index and value.

So everybody agrees with diverging from x-common?!

@petertseng
Copy link
Member Author

So everybody agrees with diverging from x-common?!

Surprisingly, I do. I think the diversion is reasonable and is better suited for our track.

And it is not so bad - we haven't diverged in the actual test cases, only how they are tested (omitting a roll function).

Any records of past diversions? I see one in crypto-square.

But there is an interesting point - the README can now be confusing for students if we only test the score. Shall we document the diversion in the HINTS.md to lessen the confusion?

And/or: Shall we propose making the x-common README less prescriptive?

@rbasso
Copy link
Contributor

rbasso commented Nov 23, 2016

  • HINTS.md 👍
  • Data type in the stub solution 👍
  • Open issue in x-common 👍

In x-common, I guess that removing the section Requirements in description.md would be enough to avoid confusion.

@abo64
Copy link
Contributor

abo64 commented Nov 23, 2016

I remember vaguely for some exercise (was it Linked List?) a distinction in the Readme for functional and imperative languages where for the former they relaxed the requirements to some extent? But I couldn't find it anymore.

@petertseng
Copy link
Member Author

a distinction in the Readme for functional and imperative languages

All I know is that exercism/problem-specifications#193 is still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants