isbn-verifier: canonical-data.json (order and descriptions)

[sjwarner-bp]

Hi all,

I have just gone about implementing isbn-verifier as part of the Java stream, and while doing so I noticed that the descriptions in canonical-data.json seem to be inconsistent (both in verbosity and syntax), plus I was not sure if there might be a better order for the tests.

Of course, it could have just been my own lack of understanding that made it difficult, but I did find implementing them confusing.

Does anyone here have any opinions on it? I'd like to hear :slightly_smiling_face:

Update: https://github.com/exercism/problem-specifications/blob/master/exercises/isbn-verifier/canonical-data.json

[Insti]

Providing a link would be helpful.

[sjwarner-bp]

Oops! It can be found here :slightly_smiling_face:

https://github.com/exercism/problem-specifications/blob/master/exercises/isbn-verifier/canonical-data.json

[Insti]

The exercise is new so the order and descriptions could probably use some tweaking, do you have any suggestions?

[sjwarner-bp]

For instance,

{
            "description": "invalid isbn check digit",
            "property": "isValid",
            "input": "3-598-21508-9",
            "expected": false
        }

Is invalid, but for what reason? The too long isbn test is also invalid, but for a different 'rule'.

The grouping of tests seems about right. Some tests explain why they are invalid and some do not. Also, there isn't a massive range testing 'invalid' characters either, though this isn't so much of an issue.

[Insti]

Is invalid, but for what reason

Because the check digit is not correct?

[Insti]

The too long isbn test is also invalid, but for another reason.

Is this because the number is too long?

[sjwarner-bp]

Because the check digit is not correct?

This is correct.

Is this because the number is too long?

This is also correct.

When I went through implementing these tests in Java, I worried that the canonical data lacked clarity, and the descriptions might be cloudy (especially to newer users). Other canonical data often seems to be more consistent and clearer. If you think otherwise, then I won't persist. I just thought it was worth raising for another opinion.

[Insti]

Please persist, I do think the descriptions (and ordering) could be improved.

The trick is coming up with a description that is concise and clear. Many languages use the description as a test name, so they cannot be too long.

Don't worry excessively if others will be able to understand it, if you worked it out, they probably will be able to also, and if they cannot, they will hopefully ask a question which will prompt a suitable clarification. This is not intended as an early exercise, so by the time the student gets to this problem they will have already solved many others.

[Insti]

Also, the canoncial-data is not the specification, the student should be reading the problem description too.

[sjwarner-bp]

the canoncial-data is not the specification, the student should be reading the problem description too

This is also an interesting point. Going through the README.md for isbn-verifier, it might also benefit from some extra attention. My understanding of the problem domain isn't great, and while it clarified it a bit, several questions remained unanswered.

From a very basic point of view, it never says that the ISBN can only be 10 digits long. This is just one example. Maybe the issue is README.md needs some more detail?

[Insti]

Maybe the issue is README.md needs some more detail?

This is probably also true. What were the questions that you had?

[sjwarner-bp]

Well an initial thing, is that there isn't a concise definition of what an ISBN-10 can be formally defined as. I'd also value knowing exactly what is 'valid' and what isn't before reading about ISBN-10 in detail.

A line to the effect of:

"The ISBN-10 format is 9 digits plus one check character (either a digit or an X only). These may be communicated with or without hyphens, and can be checked for their validity by ".

It is nice that the description.md gives some background around the problem, but maybe a simple definition of what the system should do before getting in to exactly what the digits of an ISBN mean would be good, in my opinion.

[sjwarner-bp]

The description.md for Luhn is extremely good for this in my opinion.

Given a number determine whether or not it is valid per the Luhn formula.

The Luhn algorithm is a simple checksum formula used to validate a variety of identification numbers, such as credit card numbers and Canadian Social Insurance Numbers.

The task is to check 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 are disallowed.

This is direct, right to the point, and then goes in to extra detail for those interested.

[Insti]

"The ISBN-10 format is 9 digits plus one check character (either a digit or an X only). These may be communicated with or without hyphens, and can be checked for their validity by "

Something like that would make a good addition to the description.md 👍 Will you make a pull request to add it?

[ErikSchierboom]

Great points @sjwarner-bp!

[sjwarner-bp]

Sure thing - I'll get on it soon :+1: