ISBN Verifier - Invalid code passes test cases

Exercism Building Exercism
,
1

This incorrect code passes all current test cases in the ISBN Verifier.

package isbnverifier

func IsValidISBN(isbn string) bool {
	sum := 0
	digitCount := 0
	for i := len(isbn) - 1; i >= 0; i-- {
		var num int
		ch := isbn[i]
		if ch == '-' {
			continue
		}
		if ch == 'X' && i == len(isbn)-1 {
			digitCount++
			num = 10 * digitCount
		} else if ch > '0' || ch < '9' {
			digitCount++
			num = int(ch-'0') * digitCount
		} else {
			return false
		}
		if digitCount > 10 {
			return false
		}
		sum += num
	}
	if digitCount != 10 {
		return false
	}
	return sum%11 == 0
}

The issue is with the line else if ch > '0' || ch < '9'. I meant that line to check if the character is a number, but I wrote that condition wrong. However, the code still passes the test cases it should fail on because the digitCount exceeds 10.

Adding this test case to the test suite will cause the above code to fail as it should.

{
	description: "isbn with invalid check digit",
	isbn:        "123456789E",
	expected:    false,
}
2

That’s a tricky hole in the test coverage to describe. The proposed invalid check digit does not explain it - we already have 2 cases with invalid check digits AND invalid characters. What’s the distinguishing property from those 2 cases: 3-598-21507-A and 4-598-21507-B? Is it something like invalid check digit is detected, not converted like an ASCII digit?

3

By the way: This is not Go specific, the gap is in the problem specs.

4

3-598-21507-A and 4-598-21507-B return false on my code because whatever sum is calculated isn’t divisible by 11, whereas the sum that is calculated for 123456789E happens to be divisible by 11. While I can’t see any inherent distinguishing quality to 123456789E, I think it’s perfectly reasonable to expect a typo like this to show up in a real world scenario and it is perfectly reasonable to expect a programmer to make a similar logical error.

5

@Bharath314 What test case description would you suggest based on the identified facts “happens to be divisible by 11” and “a similar logical error”? It should be convincing for at least 3 maintainers, so they agree to add this test case :slight_smile: