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

Note on luhn description for single digits #492

Merged
merged 1 commit into from
Jan 15, 2017

Conversation

stevejb71
Copy link
Contributor

Add notes on edge cases for Luhn in the description, and add a 0 length string test.

@stevejb71 stevejb71 mentioned this pull request Jan 11, 2017

As an example, here is a valid (but fictitious) Canadian Social Insurance
As an example of a valid string, here is fictitious Canadian Social Insurance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "is a" rather than just "is"

@@ -7,8 +7,11 @@ The task is to write a function that checks if a given string is valid.

Validating a Number
------
Strings of length 1 or less are not valid. Spaces are allowed in the input,
but they should be stripped before checking. All other non-digit characters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sentence makes me think of other cases: how about a string of only spaces?

However, it's not true that I would refuse to see this merged unless such a case was added. What do people think of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the space stripping is already tested, so that a spaces only test would be superfluous.

"description": "single digit strings can not be valid",
"description": "zero length strings are not valid",
"input": "",
"expected": false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an implementation that would return the wrong result for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, the single character test cases should cover this. I'm happy to remove this commit unless anyone else can think of an example...

A 0 or 1 length string is not valid, and nor is a string
with non-space characters in.
@stevejb71 stevejb71 force-pushed the NoteOnLuhnDescriptionForSingleDigits branch 2 times, most recently from fbb71db to 2b9264e Compare January 12, 2017 14:18
@stevejb71
Copy link
Contributor Author

Removed the commit containing an extra test for 0 length string.

@stevejb71 stevejb71 merged commit 20b8677 into master Jan 15, 2017
@stevejb71 stevejb71 deleted the NoteOnLuhnDescriptionForSingleDigits branch January 15, 2017 01:24
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

Successfully merging this pull request may close these issues.

4 participants