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 tests missing coverage for common mistake (right vs left) #500

Closed
michaelorr opened this issue Feb 8, 2017 · 7 comments
Closed

Luhn tests missing coverage for common mistake (right vs left) #500

michaelorr opened this issue Feb 8, 2017 · 7 comments

Comments

@michaelorr
Copy link
Contributor

From the README:
The first step of the Luhn algorithm is to double every second digit, starting from the right. ...

This distinction about right versus left isn't super obvious at first glance. The first example contains odd number of total digits, so every 2nd digit from the right is the same as every 2nd digit from the left.
When I got to the later examples, I thought that the README had a typo.
My implementation doubled every 2nd digit from the left (incorrect) but the test suite did not catch my mistake.

I was working on a PR to update what I thought to be a typo in the readme when I realized my mistake. I think the readme is clear enough, it was my mistake in reading it incorrectly.
I think a new test case should be added where modifying every 2nd digit from the right is a valid Luhn number as expected but modifying every 2nd digit from the left is not a valid Luhn number. Devising such a number should not be too difficult.

@michaelorr
Copy link
Contributor Author

Hopefully I'll have a little spare time this evening to add this test case.

@petertseng
Copy link
Member

Excellent explanation, thanks. We added the case "59", with an even number of digits, in exercism/problem-specifications#522. We want to add it to this track soon, depending on what happens in exercism/problem-specifications#523. The work for this track is in progress at #479 so follow along if you are interested.

@stkent
Copy link

stkent commented Feb 15, 2017

Hopefully exercism/problem-specifications#549, now merged, will also help with this!

@michaelorr
Copy link
Contributor Author

@stkent I'll take a look today. Thank you!

@michaelorr
Copy link
Contributor Author

exercism/problem-specifications#549 absolutely helps clarify the readme, thanks!
And #479 adds the test case 59 which would have caught my bug.
It looks like #479 is being held up by exercism/problem-specifications#523 and exercism/problem-specifications#547. Is there anything I could do to help move those along? 547 looks like it may be ready for merge.

@stkent
Copy link

stkent commented Feb 15, 2017

As far as I'm concerned, 547 is indeed good to merge. If/when 523 is resolved one way or the other, we'd still want the extra positive tests added in 547. If you are willing to review and/or merge 547, I'd appreciate it!

@petertseng
Copy link
Member

Closed by #479

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

3 participants