-
-
Notifications
You must be signed in to change notification settings - Fork 546
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: Update test of non-digit #496
Conversation
While implementing these tests for Rust I found that the final test was not well written (my fault!) exercism/rust#247 I added the alphabetical character to an already-invalid string. So the test doesn't show anything useful. Instead the test should show that a known-valid string is made invalid by the inclusion of a non-digit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test doesn't show anything useful.
well to be fair, it would show that function under test doesn't crash or do something absurd (true, false, file not found).
But yes, I support turning true -> false rather than false -> false.
I am sad to say that this case still leaves it possible for an implementation to not specifically deal with the "a" and yet reject "046a 454 286". A Ruby person might write...
valid = input.size >= 2 && input.reverse.each_char.reduce([0, false]) { |(acc, double), d|
next [acc, double] if d == ' '
d = d.to_i
[acc + (double && d != 9 ? (d * 2 % 9) : d), !double]
}.first % 10 == 0
... because 'a'.to_i is 0, and the 0 causes all the doubled digits to be in the wrong places, causing the string to be correctly rejected but for the wrong reason.
But I don't know if that's worth a whole 'nother case just to deal with the particulars of a single language (for example, the above Ruby code incorrectly accepts 046aa 454 286
, because now the two zeros end up having no effect on which digits are doubled)
Edit: Nobody caught the bug in the above code! Now I fixed it.
As mentioned earlier, all of this can be passed with a single equality check. #474 (comment) But I think the current tests work well to cover the problem. |
The difference is that the equality check would never pass code review since it is clearly not even trying to solve the general problem, just the specific cases. A student submitting the equality check will probably understand that the just-submitted code only handles these specific cases. The student will correctly understand that the code won't work for any other cases. In contrast, the above wrong code looks to at least be making a reasonable attempt. A student submitting that code might very reasonably (but incorrectly) think that the just-submitted code works for all cases. The student would be wrong, but there is no test case to show the wrongness. Far more dangerous to allow reasonable-looking code that is still wrong, who knows what things could be hidden there... there are entire contests dedicated to underhanded code. What I've said is only relevant to this PR if we believe a student might submit code like the above. |
Oh, erm, would the student actually think that? Surely they would know something is up, given that they see that their own code doesn't handle that |
@@ -27,7 +27,7 @@ | |||
}, | |||
{ | |||
"description": "strings that contain non-digits are not valid", | |||
"input": "827a 1232 7352 0569", | |||
"input": "046a 454 286", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"strings that contain non-digits are not valid even if the digits would be"
Raw Brainstorming Version of 24 Pull Requests Ruby
While implementing these tests for Rust I found that the final test was
not well written (my fault!)
exercism/rust#247
I added the alphabetical character to an already-invalid string. So the
test doesn't show anything useful.
Instead the test should show that a known-valid string is made invalid
by the inclusion of a non-digit