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

luhn: It's about checksums. #523

Open
Insti opened this issue Jan 30, 2017 · 9 comments
Open

luhn: It's about checksums. #523

Insti opened this issue Jan 30, 2017 · 9 comments

Comments

@Insti
Copy link
Contributor

Insti commented Jan 30, 2017

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
Copy link
Contributor

gibfahn commented Jan 30, 2017

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
Copy link
Contributor

rootulp commented Feb 3, 2017

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
Copy link
Contributor

stkent commented Feb 15, 2017

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
Copy link
Contributor

gibfahn commented Feb 15, 2017

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
Copy link
Contributor

rbasso commented Feb 15, 2017

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 solution doesn't check the input, when given invalid input, it will either:
    • return true and/or false to some invalid inputs.
    • generate an error or halt, so it is a partial function.
  • If it checks the input, it can, reasonably:
    • return false on every invalid input, keeping the current behavior.
    • generate an error, so it is a partial function.

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

  • check the input, to generate an useful error message, making a partial function.
  • check the input and change the output type to a Maybe Bool (optional boolean value), which seems more idiomatic in Haskell.

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
Copy link
Contributor

gibfahn commented Feb 15, 2017

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
Copy link

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.

emcoding pushed a commit that referenced this issue Nov 19, 2018
Run non-exercise tests in CI
Disable code coverage reporting when we're running in CI
@bitfield
Copy link
Contributor

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
Copy link
Member

coriolinus commented Nov 28, 2018 via email

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

No branches or pull requests

8 participants