Re: [Pharo-users] Can it do this way ?

[Roelof Wobben via Pharo-users]

Hello,

I solved it but with what I find one big ugly code

isValidIsbn: aString
    | digits lastDigit acc |
    digits := aString select: [ :each | each ~= $- ].
    digits size = 10
        ifFalse: [ ^ false ].
    lastDigit := digits last.
    digits := aString allButLast asOrderedCollection
        select: [ :each | each isDecimalDigit ]
        thenCollect: [ :each | each digitValue ].
    ((lastDigit = $X or: [ lastDigit isDigit ]) and: [ digits size == 9 ])
        ifFalse: [ ^ false ].
    acc := digits
        with: (10 to: 2 by: -1)
        collect: [ :digit :multiplier | digit * multiplier ].
    ^ ((acc sum + (lastDigit digitValue min: 10)) % 11) isZero

Can this be improved some way ?

Roelof



Op 4-9-2020 om 07:35 schreef Roelof Wobben via Pharo-users:

Nope, with your idea I cannot make this part work :

he ISBN-10 format is 9 digits (0 to 9) plus one check character (either a digit or an X only). In the case the check character is an X, this represents the value '10'. These may be communicated with or without hyphens, and can be checked for their validity by the following formula:

(x1 * 10 + x2 * 9 + x3 * 8 + x4 * 7 + x5 * 6 + x6 * 5 + x7 * 4 + x8 * 3 + x9 * 2 +

so I mean the calculation.


Roelof

Op 4-9-2020 om 06:45 schreef Roelof Wobben:

oke, then I could use your idea but then I have to make the code for calculating if its a valid  number.
and I wonder if the code will not be too big. I learned that it is good that a method does only 1 thing and this one seems to be doing more then 1 thing.

Roelof



Op 4-9-2020 om 05:24 schreef Richard O'Keefe:

What part of "return false if there are not exactly 10 characters

left after discarding dashes" fails to handle the empty string?

A test case for the empty string is is only valuable if the

empty string is NOT a special case.

On Wed, 2 Sep 2020 at 22:52, Roelof Wobben <r.wob...@home.nl> wrote:

Op 2-9-2020 om 12:38 schreef Richard O'Keefe:

There is simply no point in "taking the first nine numbers out".

And there shouldn't BE a test for the string being empty, anywhere.

'' '-' '---' and so on should all be handled the same way.

Oh well, what stops you doing

   digits := aString select: [:each | each ~= $-].

   digits size = 10 ifFalse: [^false].

   lastDigit := digits la ost.

   digits := digits copyFrom: 1 to: 9.

   ( (lastDigit = $X or: [lastDigit isDigit]) and: [

     digits allSatisfy: #isDigit]

   ) ifFalse: [^false].

Now my code does not do this, but it is just 16 lines of code with

nothing that it would make sense to extract.

Nothing only that I could not think of this one for myself.
If I do it the TDD way I come more on the way Im currently thinking

but does this case then be covered

test14_EmptyIsbn
    | result |
    result := isbnVerifierCalculator isValidIsbn: ''.
    self assert: result equals: false

and still I have to do the calcualation to see if it's valid.
If I understand the code well I can use the digits variable ?


Roelof