luhn: Update Luhn tests and description #474
Conversation
| Double the second digits, starting from the right | ||
|
|
||
| ``` | ||
| 086 858 276 |
There was a problem hiding this comment.
the 7 in the final position has changed back to a 6 here.
| ``` | ||
|
|
||
| Given an account number your code should be able to return a check digit | ||
| that can be appended to the account to generate a valid Luhn |
There was a problem hiding this comment.
a valid Luhn
You will have to excuse a question from someone unfamiliar with the domain, but is it valid to use here "Luhn" as a shorthand for "number that is valid according to Luhn algorithm"?
There was a problem hiding this comment.
Yeah, that's a bad sentence. I was hoping people wouldn't notice.
|
I like what you're doing here, and appreciate the detailed description in the OP. ❤️ I'm slightly against including the Some suggestions:
|
I'm not sure how this would look. Include the tests but mark them as optional? Or just say in the readme, "Maybe you want to write this function to generate a check digit."?
Normally I would start off with tests of small inputs, and the old test suite had some tests that did this. But with this exercise the size of the input doesn't matter much. Small or long, the algorithm is exactly the same. So tests of short strings weren't any easier for students to implement.
I'm not sure what a degenerate case would be here. There are strings that can pass validation even if certain numbers are transposed, but I didn't see much value in testing that here. I guess we could have a test of strings that contain non-digits.
1 digit inputs should always be false. Makes sense to test that. |
Insti
changed the title
|
I think I've addressed the concerns so far. |
| The [Luhn algorithm](https://en.wikipedia.org/wiki/Luhn_algorithm) is | ||
| a simple checksum formula used to validate a variety of identification | ||
| numbers, such as credit card numbers and Canadian Social Insurance | ||
| Numbers. |
There was a problem hiding this comment.
It would be nice to add a sentence or two describing what we're supposed to be doing.
"The task is to write a function to check if a given number is valid."
I'm addressing a few different things here: 1. The tests exposed functionality that is not needed The `addends` and `check_digit` methods aren't needed by clients of this code, and I didn't see that students got any benefit from explicitly implementing them. The presence of these tests pushed students towards a particular implementation, which we [try to avoid](https://github.com/exercism/x-common/blob/master/CONTRIBUTING.md#improving-consistency-by-extracting-shared-test-data) > The test cases should not require people to follow one very specific way to solve the problem, e.g. the tests should avoid checking for helper functions that might not be present in other implementations. To fix this I've removed tests of `addends` and `check_digit`. 2. The tests were duplicative. There's not a way (that I could see, at least) of implementing Luhn in tiny steps. It's either working or its not. Once you have a couple valid tests and a couple invalid tests your are pretty much covered. (note: I guess you could test the cases where Luhn can be wrong, but that seemed outside the scope of this exercise.) Again, from the guide > All tests should be essential to the problem. Ensure people can get the first test passing easily. Then each following test should ideally add a little functionality. People should be able to solve the problem a little at a time. Avoid test cases that don't introduce any new aspect and would already pass anyway. To fix this I've removed most duplicative tests that would pass as soon as the student got a working Luhn implementation. 3. The examples and the description didn't match Luhn can be hard to grasp. At least it can be for me. What helps me is to have explicit, real-world examples. To help with this I've updated the description.md file to have examples of credit cards and Canadian SINs, two places where Luhn is used in the real world. To re-enforce the descriptions, I've used the same examples in the tests. So students can see step-by-step breakdowns of how to get their tests passing. Note: I'm still on the fence about the `generate_check_digit` method. It seems to be to be outside the scope of this exercise, but I also think implementing it is interesting and useful. I could go either way on keeping it or removing it.
No one voiced a strong argument for keeping the Check Digit tests, so I'm removing the test and their section in the description. As recommended, I'm adding 3 tests - invalid string containing a non-digit - Single digit 0 (which can return 0 for `modulo 10`) is not valide - Non-zero single digit is invalid
IanWhitney
force-pushed
the
update_luhn_tests
branch
from
b7b205f to
60c733c
Compare
|
It concerns me that the entire test suite can be passed by: |
|
We could add more positive cases...and the students could pass them with a switch statement.
To me this seems like a problem solved by the nitpickers.
… On Jan 2, 2017, at 2:11 AM, Geoff Hubbard ***@***.***> wrote:
It concerns me that the entire test suite can be passed by:
return input == "046 454 286"
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Any more updates here? If not, I'll merge on Sunday, January 8. |
|
(way late to the party...) ❤️ this! Thanks for tackling this, @IanWhitney. It was a less-than-delightful experience to go through this exercise as it was. 🙇 also... @Insti said:
@IanWhitney replied
Oh yeah, we've got tons of exercises where there are sufficiently few test cases that they can be "defeated" by pure conditional logic (aka "evil coder"). I'm assuming we're working with practitioners who understand you get what you put in. |
I'm addressing a few different things here:
The
addendsandcheck_digitmethods aren't needed by clients of thiscode, and I didn't see that students got any benefit from explicitly
implementing them. The presence of these tests pushed students towards a
particular implementation, which we try to
avoid
To fix this I've removed tests of
addendsandcheck_digit.There's not a way (that I could see, at least) of implementing Luhn in
tiny steps. It's either working or its not. Once you have a couple valid
tests and a couple invalid tests your are pretty much covered. (note: I
guess you could test the cases where Luhn can be wrong, but that seemed
outside the scope of this exercise.)
Again, from the guide
To fix this I've removed most duplicative tests that would pass as soon
as the student got a working Luhn implementation.
Luhn can be hard to grasp. At least it can be for me. What helps me is
to have explicit, real-world examples. To help with this I've updated
the description.md file to have examples of credit cards and Canadian
SINs, two places where Luhn is used in the real world.
To re-enforce the descriptions, I've used the same examples in the
tests. So students can see step-by-step breakdowns of how to get their
tests passing.
Note:
I'm still on the fence about the
generate_check_digitmethod. It seemsto be to be outside the scope of this exercise, but I also think
implementing it is interesting and useful. I could go either way on
keeping it or removing it.