Add isbn-verifier exercise #924

cschlosser · 2017-10-04T19:35:11Z

This was an starter exercise in my CS class.

The exercise is really simple, depending on how much your language supports converting types and string manipulation but it can be blown up into a full grown exercise with the bonus tasks.

This teaches you some simple string manipulation combined with basic numerical calculations.
You also get to use the three datatypes of number, string and bool.

NobbZ · 2017-10-04T19:51:08Z

There should be at least a rough description of how a ISBN-10 is structured and how the checksum is calculated.

What is an ISBN comprised of and when is an ISBN valid.

Insti · 2017-10-05T07:43:11Z

exercises/isbn-verifier/canonical-data.json

+            "input": "3-598-21507-XA",
+            "expected": false
+        }
+    ]


Test suggestion: Valid ISBN for which the check digit should be 0 but is X to catch solutions that convert X to 0

Great idea, added!

Insti · 2017-10-05T07:46:01Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "valid character in isbn",


This description is confusing.

Maybe: "X is only valid as a checkdigit."

Also make sure that this ISBN would be valid if 10 was used in place of X in the calculation.

Good idea, thanks!

Insti · 2017-10-05T07:46:35Z

exercises/isbn-verifier/canonical-data.json

+            "expected": true
+        },
+        {
+            "description": "isbn without separating dashes",


This description is the same as the last, what is this test testing differently?

The X is the checkdigit

Insti · 2017-10-05T07:46:56Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "isbn without check digit",


duplicate description

Insti · 2017-10-05T07:47:09Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "too long isbn",


duplicate description

Insti · 2017-10-05T07:48:36Z

Good exercise. 👍
Needs a few tweaks but is heading in the right direction.

cschlosser

Thank you for your feedback, any further review is quite welcome!

cschlosser · 2017-10-05T13:31:16Z

exercises/isbn-verifier/canonical-data.json

+            "expected": true
+        },
+        {
+            "description": "isbn without separating dashes",


The X is the checkdigit

cschlosser · 2017-10-05T13:31:28Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "valid character in isbn",


Good idea, thanks!

cschlosser · 2017-10-05T13:32:47Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "isbn without check digit",


cschlosser · 2017-10-05T13:32:54Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "too long isbn",


cschlosser · 2017-10-05T13:36:19Z

exercises/isbn-verifier/canonical-data.json

+            "input": "3-598-21507-XA",
+            "expected": false
+        }
+    ]


Great idea, added!

Insti · 2017-10-05T20:16:16Z

Thanks for those changes.
I'm sure it will evolve as it gets implemented and people start solving the exercise, but this seems like a reasonable starting point.

rpottsoh · 2017-10-05T23:45:51Z

exercises/isbn-verifier/description.md

+
+## Bonus tasks
+
+* Generate an valid ISBN-13 from the input ISBN-10 (and maybe verify it again with an derived verifier)


Change all an's to a.

rpottsoh · 2017-10-06T00:56:26Z

exercises/isbn-verifier/canonical-data.json

+            "expected": false
+        },
+        {
+            "description": "check digit should be 0 but is X to prevent solutions to use X as 0",


How about?:

"description": "check digit of X should not be used for 0",

This test is essentially the same as:

"description": "invalid isbn check digit",

Thanks for the improved description.

Yes indeed but X is in a bit of a special position for ISBN so I think it doesn't hurt to add some extra tests for this.

rpottsoh · 2017-10-09T23:27:01Z

exercises/isbn-verifier/description.md

@@ -15,7 +15,7 @@ The following number block is to identify the publisher. Since this is a three d
 The last digit in the ISBN is the check digit which is used to detect read errors.

 The first 9 digits in the ISBN have to be between 0 and 9.
-The check digit can additionally be an 'X' to allow 10 to be a valid check digit as well.
+The check digit can additionally be a 'X' to allow 10 to be a valid check digit as well.


@christophschlosser my apologies. I was not clear on when an should be a. In this instance an is OK. An is used when the next word starts with a vowel or starts with a vowel sound. In this instance 'X' starts with a vowel sound, e in this instance.

No problem @rpottsoh . TIL. I uploaded a new commit where this is fixed.

rpottsoh · 2017-10-09T23:27:57Z

exercises/isbn-verifier/canonical-data.json

@@ -79,7 +79,7 @@
            "expected": false
        },
        {
-            "description": "check digit should be 0 but is X to prevent solutions to use X as 0",
+            "description": "check digit of X should not be used for 0",


👍 Thanks!

rpottsoh · 2017-10-09T23:28:20Z

exercises/isbn-verifier/description.md

@@ -30,6 +30,6 @@ It's getting even trickier since the check-digit of an ISBN-10 can be 'X'.

 ## Bonus tasks

-* Generate an valid ISBN-13 from the input ISBN-10 (and maybe verify it again with an derived verifier)
+* Generate a valid ISBN-13 from the input ISBN-10 (and maybe verify it again with a derived verifier)


👍 Thanks!

rpottsoh

Thanks for making these changes and more importantly, thanks for providing a new exercise for Exercism! 👍

petertseng · 2017-11-01T03:10:25Z

exercises/isbn-verifier/canonical-data.json

+        {
+            "description": "X is only valid as a check digit",
+            "property": "isbn",
+            "input": "3-598-2X507-0",


An implementation that unconditionally treats X as 10 will still reject this ISBN:

3 * 10 + 5 * 9 + 9 * 8 + 8 * 7 + 2 * 6 + 10 * 5 + 5 * 4 + 0 * 3 + 7 * 2 + 0 * 1 = 299, and 299 is not divisible by 11.

If the intention of this test is to ensure that an implementation only treats X as 10 in the check digit position, then a case such as "3-598-2X507-9" is needed.

petertseng · 2017-11-01T03:16:24Z

exercises/isbn-verifier/canonical-data.json

+        {
+            "description": "too long isbn",
+            "property": "isbn",
+            "input": "3-598-21507-XA",


An implementation that would potentially attempt to checksum ISBNs that are too long would still reject this input because it would see that the "A" is invalid.

If the intention of this case is to ensure that an implementation does in fact reject ISBNs that are too long, at the very least a valid character should be used instead of A.

If one wanted to be thorough, one might have to include three different cases here, respectively checking for implementations that try to checksum:

all 11 resulting digits (with a coefficient of 11 on the leftmost one, naturally)

the 10 rightmost digits

the 10 leftmost digits

by providing a too-long ISBN that has a valid check digit under each respective scheme.

isbn-verifier 1.1.0 "X is only valid as a check-digit" #924 (comment) An implementation that unconditionally treats X as 10 will still reject the ISBN 3-598-2X507-0 3 * 10 + 5 * 9 + 9 * 8 + 8 * 7 + 2 * 6 + 10 * 5 + 5 * 4 + 0 * 3 + 7 * 2 + 0 * 1 = 299, and 299 is not divisible by 11. If the intention of this test is to ensure that an implementation only treats X as 10 in the check digit position, then a case such as 3-598-2X507-9 is needed. The following may be run in a terminal to ensure that this ISBN does sum to a multiple of 11 under such a mistaken implementation: ruby -e 'p "3-598-2X507-9".delete(?-).chars.reverse.map.with_index { |c, i| (c == ?X ? 10 : Integer(c)) * (i + 1) }.sum % 11' "too long isbn" #924 (comment) An implementation that would potentially attempt to checksum ISBNs that are too long would still reject this input because it would see that the "A" is invalid. If the intention of this case is to ensure that an implementation does in fact reject ISBNs that are too long, at the very least a valid character should be used instead of A. If one wanted to be thorough, one might have to include three different cases here, respectively checking for implementations that try to checksum: * all 11 resulting digits (with a coefficient of 11 on the leftmost one, naturally) * the 10 rightmost digits * the 10 leftmost digits by providing a too-long ISBN that has a valid check digit under each respective scheme. This is not done in this PR because it has not yet become apparent through submissions that such three cases are necessary (submissions that unconditionally attempt to checksum using any of the three schemes listed).

Add isbn-verifier exercise

550f10b

ilya-khadykin added the new exercise idea label Oct 4, 2017

Describe ISBN

877207b

What is an ISBN comprised of and when is an ISBN valid.

Insti reviewed Oct 5, 2017

View reviewed changes

cschlosser commented Oct 5, 2017

View reviewed changes

Improve test descriptions and add new test

09a2e97

rpottsoh requested changes Oct 5, 2017

View reviewed changes

rpottsoh reviewed Oct 6, 2017

View reviewed changes

Update test description and fix wording

f5587cd

rpottsoh requested changes Oct 9, 2017

View reviewed changes

rpottsoh reviewed Oct 9, 2017

View reviewed changes

Fix wording

a4938d4

rpottsoh approved these changes Oct 10, 2017

View reviewed changes

rpottsoh merged commit 0493308 into exercism:master Oct 10, 2017

petertseng reviewed Nov 1, 2017

View reviewed changes

petertseng mentioned this pull request Nov 1, 2017

isbn-verifier: Test only the fact that each case claims to test #993

Merged


		## Bonus tasks

		* Generate an valid ISBN-13 from the input ISBN-10 (and maybe verify it again with an derived verifier)

Uh oh!

Add isbn-verifier exercise #924

Add isbn-verifier exercise #924

Uh oh!

Conversation

cschlosser commented Oct 4, 2017

Uh oh!

NobbZ commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Oct 5, 2017

Uh oh!

cschlosser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Insti commented Oct 5, 2017

Uh oh!

rpottsoh Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpottsoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpottsoh Oct 5, 2017 •

edited

Loading