exercism / problem-specifications

Shared metadata for exercism exercises.
MIT License
327 stars 544 forks source link

luhn: It's about checksums. #523

Open Insti opened 7 years ago

Insti commented 7 years ago

The main purpose of this problem is doing the checksum calculation so checking that the input string is valid should not be part of the requirements.

Test strings in the canonical-data.json file should all be well formed and only contain numbers. (Although I could be convinced that adding spaces for readability is a good idea.)

gibfahn commented 7 years ago

I find it quite helpful to have to parse and filter input, it makes the exercise seem more like actually writing a function that might actually be used rather than just implementing an algorithm. (At least for me) the idea is to practice writing realistic code in different languages to get a feel for them, so I appreciate extra touches such as handling dodgy input.

rootulp commented 7 years ago

I think there are enough practice problems that require parsing and filtering input. I agree with @Insti that the focus should be on calculating the checksum.

stkent commented 7 years ago

There's a definite tension here: parsing is well-covered by Exercism, but skipping it in any individual exercise is certainly likely to reduce the "real-world applicability." I'd be in favor of trimming down to assume valid input, and continuing to test validity (vs. returning the actual checksum value; not sure if that was part of the original suggestion here or not, so figured I'd clarify).

gibfahn commented 7 years ago

Having been through the exercises for Rust, and then spending some time making the luhn tests fit the description in the README, I found this specific test had quite an interesting filtering section. The solution should only allow digits or (the space character), not any other whitespace, but should also require you to strip out the whitespace before applying the algorithm.

The actual algorithm implementation by comparison was quite mundane.

I haven't written a lot of exercises, so I don't have much perspective on that, just giving my feeling as someone going through all the tests to practice a language.

and continuing to test validity (vs. returning the actual checksum value; not sure if that was part of the original suggestion here or not, so figured I'd clarify).

I don't think anyone suggested removing the result % 10 == 0 part of the exercise.

rbasso commented 7 years ago

In the current form, this exercises forces us to return false on invalid strings (which I don't like too much), so we have to check the input. Changing it, so that all the input strings are clean, would create this scenario:

If the exercise is changed as proposed, given the above options, I would either:

Either way, I would end up checking any input string. It is hard to avoid checking the input data when the input format allows invalid data...

This one is hard, @Insti ...I'm divided...

I would say that I support the change 👍 , not because it saves the work of checking the input, but because is would remove the tests that force signaling invalid input as a false.

gibfahn commented 7 years ago

check the input and change the output type to return a Maybe Bool (optional boolean value), which seems more idiomatic in Haskell.

Makes sense to me (it'd be Result<bool, _> in Rust). I agree you'd want to return an error for invalid input, not just false.

Emerentius commented 6 years ago

Could we clarify how non-space whitespace is handled? The description says they are disallowed but none of the tests check for that. I see many solutions that use the convenient is_whitespace functions which filter out more than spaces.

bitfield commented 5 years ago

There's a little footnote here. Because the algorithm involves doubling every other digit, you have to keep track of how many digits you've seen, not just where you are in the string. (Or sanitise the string beforehand.)

Test cases containing whitespace are important for catching the mistake where you double the digit either side of a space, for example.

coriolinus commented 5 years ago

On the other hand, there was a decision a while back, the reference to which I am currently too lazy to dig up, that most exercises should not test input cleaning; students should be able to assume that their inputs will be generally valid for the problems given. Input cleaning should be required only when it's directly relevant to the point of the exercise; otherwise, it would be required for every single exercise.

There are exercises for which exceptions should be made, but in my opinion Luhn isn't one of them.

On Wed, Nov 28, 2018 at 7:56 PM John Arundel notifications@github.com wrote:

There's a little footnote here. Because the algorithm involves doubling every other digit, you have to keep track of how many digits you've seen, not just where you are in the string. (Or sanitise the string beforehand.)

Test cases containing whitespace are important for catching the mistake where you double the digit either side of a space, for example.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/exercism/problem-specifications/issues/523#issuecomment-442563276, or mute the thread https://github.com/notifications/unsubscribe-auth/AHdeTsPCDZ0cYtkrXOv745YNDbq2ZItPks5uztxrgaJpZM4LxMND .