Publication ranges #5
Conversation
Squashed commits: [68dfa1e] Add publication ranges, for calculating all ISBN13s for a registrant's prefix
|
Thanks @slimdave ! I haven't had a chance to look this over yet, but one thing that immediately jumps out is that it would be nice if |
|
That's a good idea. Done. |
| @@ -45,6 +45,16 @@ def isbn13 | |||
| '978' + isbn[0..-2] + isbn_13_checksum | |||
| end | |||
|
|
|||
| def isbn13_checksum_corrected | |||
There was a problem hiding this comment.
It seems like most of the methods use isbn_13 rather than isbn13. My preference would be to standardize on isbn_13 (and isbn_10), and to open a separate PR to rename the anomalous isbn13 method.
There was a problem hiding this comment.
A better name for this might also be something like isbn_13_with_corrected_checksum that makes clear that an ISBN is being returned, and not a checksum.
| end | ||
|
|
||
| def publication_numbers | ||
| (0..(number_of_publications-1)) |
There was a problem hiding this comment.
Would be best to add spaces around the - sign like you do in other places in this class.
| end | ||
|
|
||
| def to_prefix | ||
| (prefix.to_i+1) * multiplier - 1 |
There was a problem hiding this comment.
Spaces around operator here too.
| end | ||
|
|
||
| def from | ||
| Lisbn.new((from_prefix*10).to_s).isbn13_checksum_corrected |
There was a problem hiding this comment.
Spaces around operator here too.
| end | ||
|
|
||
| def to | ||
| Lisbn.new((to_prefix*10).to_s).isbn13_checksum_corrected |
There was a problem hiding this comment.
Spaces around operator here too.
| private | ||
| attr_reader :seed_isbn13 | ||
|
|
||
| def multiplier |
There was a problem hiding this comment.
This method seems to have the same return value as the number_of_publications method. Should we pick one?
| @seed_isbn13 = Lisbn.new(seed_isbn13) | ||
| end | ||
|
|
||
| def parts |
There was a problem hiding this comment.
The naming of this method is confusing. Maybe registration_parts?
| @@ -0,0 +1,57 @@ | |||
| class Lisbn < String | |||
| class PublicationRange | |||
There was a problem hiding this comment.
This mechanism is a bit hard to understand, but I think from_prefix is basically:
seed_isbn13.parts[0..2].join + "0" * (12 - prefix.size) + checksum
and to_prefix is:
seed_isbn13.parts[0..2].join + "9" * (12 - prefix.size) + checksum
Is it easier to implement and think about by manipulating the parts (as a string), rather than doing the integer multiplication approach?
|
@slimdave I made a few comments above. I also added you as a collaborator on the repository, thanks for your contributions! Feel free to merge in minor changes like updates to the ranges file as you see fit. Probably a good idea to continue to request review on larger updates such as this. I'll try to be better about reviewing them promptly. Thanks again! |
This adds the ability to use an ISBN13 to generate a publication range object, which can then generate the complete list of valid ISBN13s for that registrant element.
A few other methods are included, mostly for display purposes.