Skip to content

fix: ISBN-10 Generate outer mod-11 and restrict X to check-digit position - #35

Open
binggao1230 wants to merge 1 commit into
osamingo:masterfrom
binggao1230:fix/isbn10-outer-mod11-and-x-position
Open

fix: ISBN-10 Generate outer mod-11 and restrict X to check-digit position#35
binggao1230 wants to merge 1 commit into
osamingo:masterfrom
binggao1230:fix/isbn10-outer-mod11-and-x-position

Conversation

@binggao1230

Copy link
Copy Markdown

Summary

This PR fixes two bugs in the ISBN-10 implementation (isbn.go) and adds regression tests. Both bugs are latent — the existing test suite passes without covering them.

Internal-consistency smoking gun (no external oracle needed)

The library disagrees with itself on the sum % 11 == 0 boundary:

checkdigit.NewISBN10().Verify("0000000000")  // => true  (correct check digit is 0)
checkdigit.NewISBN10().Generate("000000000") // => 11    (out of range [0,10])

Verify confirms the correct check digit for seed "000000000" is 0, but Generate returns 11 — a value outside the valid check-digit range [0, 10] and not representable as a single character. Generate and Verify contradict each other within the same library, and the Generate → Verify round-trip cannot even be formed for this seed.

Bug 1 — Generate missing the outer % 11 (ISBN-10 check-digit formula)

Per ISO 2108, the ISBN-10 check digit is:

check_digit = (11 - (weighted_sum mod 11)) mod 11      // range 0-10, 10 => 'X'

isbn.go computed 11 - sum%11 without the outer mod 11. When the weighted sum is divisible by 11, this yields 11 instead of 0:

Seed (9 digits) Weighted sum sum%11 Before (buggy) After (fixed)
"000000000" 0 0 11 0
"055555555" 220 0 11 0

The fix applies the outer % 11:

// before
return 11 - sum%11, nil
// after
return (11 - sum%11) % 11, nil

Only the sum%11==0 boundary changes (11 → 0); every other check digit (1–10) is unchanged. Regression guards: Generate("000000001")=9, Generate("000000006")=10 (the X case), Generate("123456789")=10 all remain correct.

Bug 2 — Verify accepts X in any position

Verify mapped n == 'X'digit = 10 for any position. Per ISO 2108, X (value 10) is valid only as the check digit (the final character). Codes with X in a non-final position were falsely accepted when the weighted sum happened to be divisible by 11:

Input X position Before (buggy) After (fixed)
"X026515627" 0 (first) true false
"0X00000009" 1 (second) true false

The fix rejects X in any non-final position. Valid ISBNs with X in the last position remain accepted (e.g. Verify("000000006X")=true).

Fix

isbn.go (minimal, style-matching):

  1. Generate: return 11 - sum%11, nilreturn (11 - sum%11) % 11, nil (+ ISO 2108 comment).
  2. Verify: track the loop index and reject X in any non-final position (+ spec comment: X is valid only as the check digit).

The weighted-sum verification logic is otherwise untouched.

Tests

Adds isbn_edge_test.go (follows the existing isbn_test.go table-driven style):

  • Generate sum%11==0 cases (000000000→0, 055555555→0) + range check.
  • Generate non-zero regression guards (000000001→9, 000000006→10, 123456789→10).
  • Verify X-in-wrong-position false-accepts (X026515627→false, 0X00000009→false).
  • Verify valid-X-last + controls (000000006X→true, 0000000000→true, 0000000001→false).
  • Generate → Verify round-trip property over seeds including the sum%11==0 boundary.

Verification

  • go test ./...PASS (existing + new tests).
  • go vet ./... — clean.
  • gofmt -l . — clean.
  • Pre-fix: the RED cases above reproduced on pristine HEAD (5504ad35). Post-fix: all RED cases resolved, all controls unchanged, round-trip holds.

…tion

Two bugs in the ISBN-10 implementation (isbn.go):

1. Generate: `return 11 - sum%11, nil` missed the outer `% 11` required by
   the ISO 2108 check-digit formula `(11 - (sum mod 11)) mod 11`. When the
   weighted sum is divisible by 11, Generate returned 11 instead of 0 — an
   out-of-range value (valid range is 0-10) that breaks the Generator
   contract and contradicts the library's own Verify, which accepts
   Verify("0000000000")=true (correct check digit 0). Fixed by applying the
   outer `% 11`: `(11 - sum%11) % 11`. Only the sum%11==0 boundary changes
   (11 -> 0); all other check digits 1-10 are unchanged.

2. Verify: the `case n == 'X': digit = 10` branch accepted 'X' (value 10)
   in any position. Per ISO 2108, 'X' is valid only as the check digit (the
   final character). Codes such as "X026515627" (X at position 0) and
   "0X00000009" (X at position 1) were falsely accepted. Fixed by rejecting
   'X' in any non-final position. Valid ISBNs with 'X' in the last position
   (e.g. "000000006X") remain accepted.

Adds isbn_edge_test.go with regression tests for both bugs (Generate
sum%11==0 cases, non-zero regression guards, Verify X-in-wrong-position
false-accepts, valid-X-last controls, and a Generate->Verify round-trip
property over seeds including the sum%11==0 boundary).

`go test ./...`, `go vet ./...`, and `gofmt -l .` all pass clean.
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.

1 participant