ISBN Verifier about julia HOT 12 CLOSED

from julia.

Hello!

Why do you particularly want to avoid calling replace in two places?

If you do want to call it just the once you can do so without a try/catch. I think what's tripping you up is that you can't reuse ISBN(s) in isvalid(ISBN, s) without a try/catch and yet you don't want to duplicate the code in the ISBN function. But what if you had a third function?

Does that help?

Regarding a hint in the description, I don't think we are pushing students to use replace only once or avoid try/catch, so I'm not sure we need one?

Anyone wanting to improve their solution can always request mentoring :)

Looking at the description, I think our main issue is that we don't specify the problem very completely (you have to read the tests for that).

We also suggest a truly horrendous bonus task: "Implement show(io::IO, isbn::ISBN) to print the ISBN in the standard format with dashes." Where to place the dashes depends on pages and pages of data about different worldwide publishers, etc cos each part of the ISBN is variable length.

We should probably remove that, tho I think that bit is only visible in the instructions.md that doesn't get used? I forget how that works.

from julia.

Hi thanks for the answer! I had not thought about that indeed, I really wanted to use those two together :)

However, just so I don't miss the point again with a third function I would need to use that function twice or can I avoid that?

The strange bonus track I have not noticed honestly so it should be hidden.

For sure I had to read the tests while doing the exercises otherwise the DomainError would have never crossed my mind.

from julia.

ISBN(s) and isvalid(ISBN, s) will each need to call your third function at least once each. Further details (not too spoilery) below.

from julia.

Thanks so much! This is the solution I didn't know existed :)

from julia.

Here's my solution just written and a comment:

# It doesn't make any sense to me for an ISBN to be a String - basically none of the String API makes sense for an ISBN.
# But the tests require this, so whatever.
struct ISBN{T} <: AbstractString
    s::T
end

Base.string(s::ISBN) = s.s

function Base.tryparse(::Type{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
            # Invalid character
            return nothing
        end
    end
    if coeff != 0
        # String did not contain 10 "digits"
        return nothing
    elseif acc % 11 != 0
        # Check digit invalid
        return nothing
    else
        # Canonicalise the string in the simplest way:
        s = replace(s, '-' => "")
        return ISBN{typeof(s)}(s)
    end
end

function ISBN(s)
    x = tryparse(ISBN, s)
    if isnothing(x)
        throw(DomainError(s, "Invalid ISBN (wrong length, wrong checksum or invalid characters)"))
    else
        x
    end
end

Base.isvalid(::Type{ISBN}, s) = tryparse(ISBN, s) != nothing

macro isbn_str(s)
    ISBN(s)
end

And if you are iterating over the characters in the string you don't need to use replace in isvalid at all, so here's another way you can write it:

# It doesn't make any sense to me for an ISBN to be a String - basically none of the String API makes sense for an ISBN.
# But the tests require this, so whatever.
struct ISBN{T} <: AbstractString
    s::T

    function ISBN(s)
        if !isvalid(ISBN, s)
            throw(DomainError(s, "Invalid ISBN (wrong length, wrong checksum or invalid characters)"))
        else
            # Canonicalise the string in the simplest way:
            s = replace(s, '-' => "")
            new{typeof(s)}(s)
        end
    end
end

Base.string(s::ISBN) = s.s

function Base.isvalid(::Type{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
            # Invalid character
            return false
        end
    end
    # s must contain 10 "digits" and check digit must be valid.
    return coeff == 0 && acc % 11 == 0
end

macro isbn_str(s)
    ISBN(s)
end

from julia.

# It doesn't make any sense to me for an ISBN to be a String - basically none of the String API makes sense for an ISBN.
# But the tests require this, so whatever.

isvalid is only defined on strings by default, which is why I think I added that back then. But I agree there's no real need for it, feel free to remove that test.

from julia.

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

That sounds reasonable. Do you want to submit a PR to change it?

from julia.

If you want and there is no rush I can try to make the PR, I think I might have understood the point. Otherwise I will solve the exercise again after you change it!

from julia.

I made a PR at #504. Can't remember if @gpucce would have been notified about that.

from julia.

I think yes because I knew you had done it :) @cmcaine should I close the issue?

from julia.

I'll do it. Thanks for engaging!

from julia.