ISBN biznis (JuliaLang#504)

* Change problem definition

As I wrote in exercism/julia#503

Thinking about it some more, I think that's because `isvalid` isn't really
the right function for us. `isvalid` checks if a string or character
contains only valid UTF-8 codepoints. I think it only makes sense for types
where the representation of the type (the bits in memory) can encode
something invalid in normal use. This is the case for strings because Julia
does not validate that strings or characters are correctly encoded when
they are created, it just treats them as opaque bytes.

For most Julia types, however, you cannot create an invalid value of the
type, except through silly use of pointers.

I think in real code the advice would be to parse the string into an ISBN
type, and if that is not possible (e.g. because the string does not encode
a valid ISBN), the ISBN constructor (or `parse(ISBN, s)`, whichever makes
more sense) should throw an error. If a non-throwing option is required,
`tryparse(ISBN, s)` is the right function to define.

Personally, I would define only a constructor and have it throw errors.

* Improve docstring

* Use the vararg form of print

It's slightly faster and maybe better practice.

* Remove unused isbn-verifier/.meta/description.md

* isbn: update tests and instructions

Simplifies the public API of `ISBN` to something that's arguably more
realistic and includes enough detail in the description to solve the
exercise without reading the tests.

* Define an explicit `==` method in isbn-verifier example

* write a design.md

* better test_nothrows

* Clarify where to put the bonus tests

Co-authored-by: Sascha Mann <[email protected]>

Co-authored-by: Sascha Mann <[email protected]>

exercises/practice/isbn-verifier/.docs/instructions.md

@@ -24,11 +24,17 @@ Since the result is 0, this proves that our ISBN is valid.
 
 ## Task
 
-Given a string the program should check if the provided string is a valid ISBN-10.
-Putting this into place requires some thinking about preprocessing/parsing of the string prior to calculating the check digit for the ISBN.
+Define a new `ISBN` type and a constructor for it that accepts a string.
+The constructor should throw a `DomainError` if the provided string does not look like a valid ISBN-10.
 
-The program should be able to verify ISBN-10 both with and without separating dashes.
+The constructor should accept strings with and without separating dashes.
 
+`ISBN` values should compare as equal if the ISBN-10s are the same and not otherwise, e.g.
+
+```jl
+ISBN("1-234-56789-X") == ISBN("123456789X") == ISBN("1234-56789X")
+ISBN("1-234-56789-X") != ISBN("3-598-21508-8")
+```
 
 ## Caveats
 
@@ -37,6 +43,12 @@ Now, it's even trickier since the check digit of an ISBN-10 may be 'X' (represen
 
 ## Bonus tasks
 
-* Generate a valid ISBN-13 from the input ISBN-10 (and maybe verify it again with a derived verifier).
+If you attempt these tasks please write your own tests for them in your solution file!
+
+* Define a string macro so that `isbn"3-598-21507-X" == ISBN("3-598-21507-X")`.
+
+* Let the constructor accept ISBN-13s as well. The same ISBN should compare equal regardless of format: `ISBN("1-234-56789-X") == ISBN("978-1-234-56789-7")`.
+
+* Define a function or iterator to generate a valid ISBN, maybe even from a given starting ISBN.
 
-* Generate valid ISBN, maybe even from a given starting ISBN.
+* Can you store the ISBN as an integer rather than a string and would that ever be a good idea? If you did, could you still print the ISBN (including check-digit) as a string?

exercises/practice/isbn-verifier/.meta/design.md

@@ -0,0 +1,9 @@
+# Design
+
+This exercise differs from the upstream problem-specifications one because we're trying to teach "[parse don't validate](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/)".
+
+Implementations of tests for the bonus tasks are deliberately excluded and instead we encourage the student to write their own. If you are thinking of changing this, maybe review the newer solutions and see if students did actually start defining their own tests (we started suggesting writing your own tests in May 2022).
+
+I (@cmcaine) wasn't sure if the better interface was `ISBN(s)` or `parse(ISBN, s)`. I went with the former to keep things simple, but there's a clear argument in the standard library for defining both (e.g. `Dates.Date` and `Base.UUID`).
+
+It might have been better to require `ISBN(s)` to throw `ArgumentError`, as e.g. `Dates(2022, 13, 1)` or `parse(Int, 'a')` do, but earlier versions of this exercise use `DomainError`, so whatever. Mostly this is Julia's fault for having two exception types with similar uses.

exercises/practice/isbn-verifier/.meta/example.jl

@@ -1,28 +1,44 @@
-macro isbn_str(s)
-    ISBN(s)
-end
-
-struct ISBN <: AbstractString
-    s::AbstractString
-
-    ISBN(s) = verify(s) ? new(replace(s, "-" => "")) : throw(DomainError(s, "invalid ISBN string"))
+import Base: ==, show
+
+struct ISBN{T<:AbstractString}
+    s::T
+
+    """
+        ISBN(s)
+
+    An International Standard Book Number. Throws `DomainError` if `s` does not look like a valid 10-digit ISBN.
+    """
+    function ISBN(s)
+        acc = 0
+        coeff = 10
+        for c in s
+            if c == '-'
+                continue
+            elseif isdigit(c)
+                # c - '0' is a much faster version of parse(Int, c) when we know c is in '0':'9'.
+                acc += (c - '0') * coeff
+                coeff -= 1
+            # Only the check "digit" is allowed to be 'X' (10)
+            elseif c == 'X' && coeff == 1
+                acc += 10
+                coeff -= 1
+            else
+                throw(DomainError(s, "'$c' is not allowable at this location"))
+            end
+        end
+        coeff == 0 || throw(DomainError(s, "ISBNs must be 10 digits long"))
+        acc % 11 == 0 || throw(DomainError(s, "ISBN checksum failed"))
+        # Canonicalise in the simplest way
+        s′ = replace(s, '-' => "")
+        new{typeof(s′)}(s′)
+    end
 end
 
-Base.string(s::ISBN) = s.s
-
-function verify(s::AbstractString)
-    s = replace(s, "-" => "")
-    chars = split(s, "")
+value(x::ISBN) = x.s
 
-    length(chars) == 10 || return false
+==(a::ISBN, b::ISBN) = value(a) == value(b)
 
-    if last(chars) == "X"
-        chars[end] = "10"
-    end
-
-    all(c -> all(isdigit, c), chars) || return false
-
-    sum(parse(Int, c) * i for (c, i) in zip(chars, 10:-1:1)) % 11 == 0
-end
+macro isbn_str(s) ISBN(s) end
 
-Base.isvalid(::Type{ISBN}, s::AbstractString) = verify(s)
+# Show ISBNs as they might appear in your source code if you used the macro.
+show(io::IO, isbn::ISBN) = print(io, "isbn\"", value(isbn), '"')

exercises/practice/isbn-verifier/runtests.jl

@@ -2,64 +2,31 @@ using Test
 
 include("isbn-verifier.jl")
 
-@test ISBN <: AbstractString
+"""
+    @test_nothrow expr
 
-@testset "valid ISBN numbers" begin
-    # ISBN number
-    @test isvalid(ISBN, "3-598-21508-8")
-    # ISBN number with a check digit of 10
-    @test isvalid(ISBN, "3-598-21507-X")
-    # ISBN without separating dashes
-    @test isvalid(ISBN, "3598215088")
-    # ISBN without separating dashes and X as check digit
-    @test isvalid(ISBN, "359821507X")
+Test that the expression `expr` does not throw an exception.
+"""
+# Julia doesn't include this because you would usually test some property of
+# your object (as we do later) rather than checking if an exception was thrown.
+# We define this anyway so that you can focus on the validation logic before
+# working out how to make the structs compare equal.
+macro test_nothrow(expr)
+    :(@test ( $expr; true ))
 end
 
-@testset "invalid ISBN numbers" begin
-    # invalid ISBN check digit
-    @test !isvalid(ISBN, "3-598-21508-9")
-    # check digit is a character other than X
-    @test !isvalid(ISBN, "3-598-21507-A")
-    # invalid character in ISBN
-    @test !isvalid(ISBN, "3-598-2K507-0")
-    # X is only valid as a check isdigit
-    @test !isvalid(ISBN, "3-598-2X507-9")
-    # ISBN without check digit and dashes
-    @test !isvalid(ISBN, "359821507")
-    # too long ISBN and no dashes
-    @test !isvalid(ISBN, "3598215078X")
-    # ISBN without check digit
-    @test !isvalid(ISBN, "3-598-21507")
-    # too long ISBN
-    @test !isvalid(ISBN, "3-598-21507-XX")
-    # check digit of X should not be used for 0
-    @test !isvalid(ISBN, "3-598-21515-X")
-    # empty ISBN
-    @test !isvalid(ISBN, "")
-    # invalid character in ISBN
-    @test !isvalid(ISBN, "3-598-P1581-X")
-    # too short ISBN
-    @test !isvalid(ISBN, "00")
-    # input is 9 characters
-    @test !isvalid(ISBN, "134456729")
-    # invalid characters are not ignored
-    @test !isvalid(ISBN, "3132P34035")
-    # input is too long but contains a valid ISBN
-    @test !isvalid(ISBN, "98245726788")
-end
-
-@testset "constructing valid ISBN numbers" begin
+@testset "valid ISBNs don't throw" begin
     # ISBN number
-    @test string(isbn"3-598-21508-8") == "3598215088"
+    @test_nothrow ISBN("3-598-21508-8")
     # ISBN number with a check digit of 10
-    @test string(isbn"3-598-21507-X") == "359821507X"
+    @test_nothrow ISBN("3-598-21507-X")
     # ISBN without separating dashes
-    @test string(isbn"3598215088") == "3598215088"
+    @test_nothrow ISBN("3598215088")
     # ISBN without separating dashes and X as check digit
-    @test string(isbn"359821507X") == "359821507X"
+    @test_nothrow ISBN("359821507X")
 end
 
-@testset "constructing invalid ISBN numbers" begin
+@testset "invalid ISBNs throw DomainError" begin
     # invalid ISBN check digit
     @test_throws DomainError ISBN("3-598-21508-9")
     # check digit is a character other than X
@@ -91,3 +58,7 @@ end
     # input is too long but contains a valid ISBN
     @test_throws DomainError ISBN("98245726788")
 end
+
+@testset "ISBNs compare equal when they're the same number" begin
+    @test ISBN("3-598-21508-8") == ISBN("3598215088") != ISBN("3-598-21507-X")
+end