6
\$\begingroup\$

I'm just looking for a cleaner way. Obviously I could store a variable for some of the int parts. Personally, I think the one-liner looks better and everyone's definition of what constitutes clean readable code differs.

def gen_isbn(isbn):

   for i, val in enumerate(isbn):
       if len(isbn) == 9:
           yield (i + 1) * int(val)
       else:
           yield int(val) * 3 if (i + 1) % 2 == 0 else int(val) * 1

def isbn10(isbn):

    mod = sum(gen_isbn(isbn[:-1])) % 11
    return mod == int(isbn[-1]) if isbn[-1] != 'X' else mod == 10

def isbn13(isbn):

    mod = sum(gen_isbn(isbn[:-1])) % 10
    return 10 - mod == int(isbn[-1]) if mod != 10 else int(isbn[-1]) == 0

I'm really just wondering if there's a regex or cleaner one liners etc since this is already pretty short and works. It should be for both ISBN 10 and 13.

Preferably, I would prefer pure Python. These are grabbed from web-scraping and hence I have enough external libraries floating around than throwing another one in just for ISBN validation.

\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

Unnecessary check / bug

mod can never be \$10\$ in isbn13 (it will always be smaller) because it is defined as x `mod` 10 and: $$ x\ `mod` \ n< \ n$$

So you can just write:

return 10 - mod == int(isbn[-1])

Remove repetition

I like the conditional expression, it is terse and clear, and it can reduce duplication by a lot, but in the below int(val) is repeated twice:

int(val) * 3 if (i + 1) % 2 == 0 else int(val) * 1

This is easy to fix, just make the ternary comprehend just the multiplier:

int(val) * (3 if (i + 1) % 2 == 0 else 1)

or, using a temporary var:

multiplier = 3 if (i + 1) % 2 == 0 else 1
int(val) * multiplier

This makes it extra-clear that int(val) remains the same and only the number it is multiplied with changes.

Base eleven

A special case for X is not needed if you think about it as base eleven, where X is the eleventh digit:

def from_base_eleven(digit):
    return 10 if digit == 'X' else int(digit)

And then:

return mod == from_base_eleven(isbn[-1])

It makes the logic you're following easier to follow.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ @Slayer it is common practice here at CodeReview to wait a day or two before accepting to give other people a chance to answer. \$\endgroup\$ Commented Feb 7, 2016 at 21:30
1
\$\begingroup\$

I don't know that's it's clearer, and in fact, I would likely argue that your version is better in that regard, but here is a rather long-ish one-liner for the ISBN13 evaluation:

def isbn13a(isbn):
    return 0 == (sum([int(i) for i in isbn[::2]])+3*sum([int(i) for i in isbn[1::2]]))%10

We could write it more clearly like this:

def isbn13(isbn):
    even = sum([int(i) for i in isbn[::2]])
    odd = 3*sum([int(i) for i in isbn[1::2]])
    return 0 == (odd + even) % 10

However it would be better to also add a check for the proper length.

Since the sum including the checksum should be zero, we don't need to handle the last digit any differently than any other digit. Instead we simply fold it into the sum and then check for zero remainder mod 10.

\$\endgroup\$

You must log in to answer this question.