Skip to content

Add an ISBN Verifier test case for short ISBN#1251

Merged
rpottsoh merged 1 commit intoexercism:masterfrom
jgomo3:master
Jun 18, 2018
Merged

Add an ISBN Verifier test case for short ISBN#1251
rpottsoh merged 1 commit intoexercism:masterfrom
jgomo3:master

Conversation

@jgomo3
Copy link
Copy Markdown
Contributor

@jgomo3 jgomo3 commented Jun 17, 2018

If your code verify the isbn length to be only less than or equal to 10, it will accept some ISBN with less than 10 digits which still satisfy other rules, being "00" a case which could be considered ISBN by some "almost correct" solutions missing this consideration.

I.e: http://exercism.io/submissions/f2fac1e79bf441e08afecd196ec0a442

If your code verify the isbn length to be only less than or equal to 10, it will accept some ISBN with less than 10 digits which still satisfy other rules, being "00" a case which could be considered ISBN by some "almost correct" solutions missing this consideration.

I.e: http://exercism.io/submissions/f2fac1e79bf441e08afecd196ec0a442
Copy link
Copy Markdown
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

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

Interesting catch. Thanks posting this improvement.

@petertseng
Copy link
Copy Markdown
Member

petertseng commented Jun 18, 2018

Thank you. Please mark that this closes #1223 using a word in https://help.github.com/articles/closing-issues-using-keywords/ . (unless someone would like to convince me that #1223 should be done with a case that has nonzero digits)

@rpottsoh
Copy link
Copy Markdown
Member

I like it. I agree that this PR should put #1223 to bed.

@rpottsoh
Copy link
Copy Markdown
Member

closes #1223

@rpottsoh rpottsoh merged commit 5b8998d into exercism:master Jun 18, 2018
rpottsoh added a commit to rpottsoh/exercism-problem-specifications that referenced this pull request Jun 18, 2018
I was too hasty when I merged exercism#1251.  I missed that version had not been updated.
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.

3 participants