[WIP] isbn-verifier: implement exercise

[eulercb]

A detailed description of this exercise can be found here.

TODO:

• ISBN-10 verifier
• ISBN-10 generator from a starting ISBN (bonus task)
• ISBN-13 generator (bonus task)
• Suggested changes

[jamesmcm]

Just some notes, overall it looks good.

Perhaps the extra tasks you suggest should be split out in to separate exercises?

[jamesmcm]

This is quite a lot of code for a small task, I'd recommend something like:

import string

def verify_isbn(isbn):
    handleX = lambda x: 10 if x == 'X' else x
    nums = [int(handleX(x)) for x in isbn if x in string.digits+'X']
    return(sum([x*(10-ind) for ind, x in enumerate(nums)]) % 11 == 0)

[jamesmcm]

Here you can use AssertTrue and AssertFalse instead of AssertIs.

I.e.:

    def test_verify_isbn_example_with_X_spaces(self):
        self.assertTrue(isbn_verifier.verify_isbn('955 20 3051 X'))

[ilya-khadykin]

assertFalse won't fail the test if the tested method returns None in comparison with assertIs

Consider this:

import unittest

tc = unittest.TestCase()

tc.assertFalse(None) # passes
tc.assertIs(None, True) # fails as expected

Do you see the difference, @jamesmcm?

So, I think it's better to use assertIs even if readability suffers a bit

[jamesmcm]

That's a good point, seems like strange behaviour from the unittest module.

[ilya-khadykin]

Yeah, especially considering that bool(None) returns False meaning that None is Falsy

[N-Parsons]

@m-a-ge, your example code is asserting that None is Falsey, so passing is the expected behaviour - bool(None) is False, so assertFalse(None) should pass (and does).

import unittest

tc = unittest.TestCase()

tc.assertFalse(None)  # Passes, since None is Falsey
tc.assertTrue(None)   # Fails because None is not Truthy

tc.assertIs(None, False)  # Fails because None is not False
tc.assertIs(None, True)   # Fails because None is not True

In theory, there's nothing wrong with using assertTrue and assertFalse in exercise tests, but the presence of the latter will cause some tests to pass when no code is implemented, and so these could confuse some people. Using assertIs creates another slight issue, in that it forces learners to return booleans, even though they may expect to be able to use Truthy and Falsey statements instead. I'd consider assertIs the lesser of two evils.

[ilya-khadykin]

@N-Parsons thanks for clarification

[jamesmcm]

I would consider this a much, much easier difficulty. Perhaps put it after the RNA Transcription exercise?

[N-Parsons]

@jamesmcm, the implementation of the two additional tests is fairly simple, so I don't think they should go into a separate exercise.

ISBN-10 to ISBN-13: Prepend "978" and calculate the new checksum (by the ISBN-13 method).
ISBN-10 generator: Calculate the checksum, if the ISBN is valid, return it, otherwise correct the last digit and return it.

These tests are clearly quite easy extensions if the checksum calculation is implemented as a separate function to the validation.

[N-Parsons]

Looks good overall, but it could do with some additional information in the README (and .meta/hints.md so that it doesn't get lost in future). You also seem to have some extra changes to your config.json that will need to be removed before merging.

I also commented on the choice of function names, but I'd like to hear your thoughts on this.

[N-Parsons]

Looks like you've accidentally included some changes for a different exercise.

[N-Parsons]

This should be at heading level 2 (## Submitting Exercises).

[N-Parsons]

generate_isbn13_from_isbn10 might be a better function name?

[N-Parsons]

generate_isbn might be a better function name? I think function names should be more descriptive of what they do than what they are.

[N-Parsons]

Could you add something here (and to .meta/hints.md) to explain that we are also expecting implementations of an ISBN10 generator and an ISBN13 generator? It will also need to explain how the conversion from ISBN10 to ISBN13 works, and how to do the ISBN13 checksum calculation.

[cmccandless]

@eulercamposbarros Canonical-data has been updated in /exercism/problem-specifications/#993

[stale]

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[pheanex]

Would it be okay for everyone If I continue from where @jamesmcm left off?
I'd really like to see this exercise online.

[cmccandless]

@pheanex Actually the PR was started by @eulercamposbarros. In any case, as this has been closed due to being abandoned, the original author has effectively forfeited his/her claim on the work, so feel free to pick it up!