Can it do this way ?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Can it do this way ?

Pharo Smalltalk Users mailing list
Hello,

I have now a challenge where I have to validate a ISBN number.

Can I do something like this on the class side :

(string isEmpty)
    ifTrrue: [ ^ false]
   ifFalse:  [ digits:= something.
                    controlDigit := something.
                    self validateISBNNumber]

where on the validateISBNNumber I use the instance variables digits and
controlDigit to validate the ISBN number on the instance side.

Roelof







Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

jtuchel
Roelof,

I don't think so.

A Class cannot access instance variables. Why? because a Class doesn't know which of its instances to ask for it.
Ask yourself: who is "self" in a Class method? Is it the Class or is it an individual instance of the Class?

(Hint: in a Class, #self is the Class, while in an instance, #self is the instance) .


"Who" do you want to validate the ISBN number? An individual instance of your Class (like Book)? Or is validating an ISBNNumber a Task that is independent of an individual item? If it is independent, then why would you want to use semething specific in that method?

You need to understand the differences between Class and Instance variables. I suggest reading Chapter 4 of "The Art and Science of Smalltalk" or better: read the whole book. It's easy to read and sparks quite a few A-HA's per hour.


HTH

Joachim


Am 02.09.20 um 08:18 schrieb Roelof Wobben:
Hello,

I have now a challenge where I have to validate a ISBN number.

Can I do something like this on the class side :

(string isEmpty)
   ifTrrue: [ ^ false]
  ifFalse:  [ digits:= something.
                   controlDigit := something.
                   self validateISBNNumber]

where on the validateISBNNumber I use the instance variables digits and controlDigit to validate the ISBN number on the instance side.

Roelof









-- 
-----------------------------------------------------------------------
Objektfabrik Joachim Tuchel          [hidden email]
Fliederweg 1                         http://www.objektfabrik.de
D-71640 Ludwigsburg                  http://joachimtuchel.wordpress.com
Telefon: +49 7141 56 10 86 0         Fax: +49 7141 56 10 86 1


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Richard O'Keefe
Everything Joachim Tuchel wrote about this is fine EXCEPT that
this is an Exercism task, where Roelof has to implement an API
that he is given and is not at liberty to do it right.

Thanks to the way that IsbnVerifierTest is structured,
- the IsbnVerifier class has NO use for ANY variables of ANY
  kind: no instance variables, no class variables, and no
  class instance variables.
- the IsbnVerifier class has only ONE method that is called,
  and that is on the INSTANCE side.
That is, it's a simple pure function disguised as an object.

A good design might have an ISBN class, and would put
#isValidISBN10: and #isValidISBN13: on the class side.

But to solve the Exercism exercises, you HAVE to implement
the API that the test class is expecting, not something
better.  If it passes the tests, it is by definition right.
If it doesn't, it is by definition wrong, even if it is a
far better design.  So
 - one class, IsbnVerifier
 - with no variables of any kind
 - with one and only one method, #isValidIsbn,
   on the instance side.

It all gets so much clearer when you realise that the Exercism
tasks are NOT designed to teach Object-Oriented Design.  The
problems have to serve many programming languages, some of
which, like bash and x86 assembly code, are not object-oriented.


On Wed, 2 Sep 2020 at 18:42, [hidden email] <[hidden email]> wrote:
Roelof,

I don't think so.

A Class cannot access instance variables. Why? because a Class doesn't know which of its instances to ask for it.
Ask yourself: who is "self" in a Class method? Is it the Class or is it an individual instance of the Class?

(Hint: in a Class, #self is the Class, while in an instance, #self is the instance) .


"Who" do you want to validate the ISBN number? An individual instance of your Class (like Book)? Or is validating an ISBNNumber a Task that is independent of an individual item? If it is independent, then why would you want to use semething specific in that method?

You need to understand the differences between Class and Instance variables. I suggest reading Chapter 4 of "The Art and Science of Smalltalk" or better: read the whole book. It's easy to read and sparks quite a few A-HA's per hour.


HTH

Joachim


Am 02.09.20 um 08:18 schrieb Roelof Wobben:
Hello,

I have now a challenge where I have to validate a ISBN number.

Can I do something like this on the class side :

(string isEmpty)
   ifTrrue: [ ^ false]
  ifFalse:  [ digits:= something.
                   controlDigit := something.
                   self validateISBNNumber]

where on the validateISBNNumber I use the instance variables digits and controlDigit to validate the ISBN number on the instance side.

Roelof









-- 
-----------------------------------------------------------------------
Objektfabrik Joachim Tuchel          [hidden email]
Fliederweg 1                         http://www.objektfabrik.de
D-71640 Ludwigsburg                  http://joachimtuchel.wordpress.com
Telefon: +49 7141 56 10 86 0         Fax: +49 7141 56 10 86 1


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Richard O'Keefe
In reply to this post by Pharo Smalltalk Users mailing list
Briefly, you cannot and (for a different reason) you should not.
The IsbnVerifierTest class defines the API; the method must be
isValidIsbn: aString
and it MUST be on the instance side.
The reason why you shouldn't is that an empty string should NOT
be a special case.  This is a problem where the design style
described by Dijkstra in "A Discipline of Programming"
and further fleshed out by Reynolds and Gries in related books
pays off.  You want to inspect all the characters of the string
once each in a single left-to-right loop.

On Wed, 2 Sep 2020 at 18:19, Roelof Wobben via Pharo-users <[hidden email]> wrote:
Hello,

I have now a challenge where I have to validate a ISBN number.

Can I do something like this on the class side :

(string isEmpty)
    ifTrrue: [ ^ false]
   ifFalse:  [ digits:= something.
                    controlDigit := something.
                    self validateISBNNumber]

where on the validateISBNNumber I use the instance variables digits and
controlDigit to validate the ISBN number on the instance side.

Roelof







Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
Yep, I know that isValidIsbn is the method that must output if a isbn is
valid or not.

What I want to do is take the first 9 characters out so I can convert
them to a array of numbers where I can do the calculation on.
And take out the last char so I can seperate test if that is a valid
char. So between the 0 and 9 or a X

I do not think I would have do all the checks in that only method
because it would be a very big method then.

but if I understand you well  also the test if a string is empty should
be called from the isValidIsbn method or even checked there.

Roelof



Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Richard O'Keefe
In reply to this post by 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.

On Wed, 2 Sep 2020 at 22:24, Roelof Wobben <[hidden email]> wrote:
Yep, I know that isValidIsbn is the method that must output if a isbn is
valid or not.

What I want to do is take the first 9 characters out so I can convert
them to a array of numbers where I can do the calculation on.
And take out the last char so I can seperate test if that is a valid
char. So between the 0 and 9 or a X

I do not think I would have do all the checks in that only method
because it would be a very big method then.

but if I understand you well  also the test if a string is empty should
be called from the isValidIsbn method or even checked there.

Roelof


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
In reply to this post by Richard O'Keefe

Correct. This is a exercism task.

and yes, there are no variables given but that does not in my oponion
means that I cannot add a instance or a class variable
I myself was thinking about adding two instance variables named digits
which hold the first 9 characters and a variable named controlDigit

Can I do it then this way

1) Check if the string is empty , if so return false otherwise made a
new object like this in IsValidIsbn

isValidIsbn: aString
     if aString isEmpty
       ifTrue: [False]
      ifFalse: [ self new
                          digits := something.
                          controlDigit := something.]


I can also do what I think you are describing

isValidIsbn: AString
     if aString isEmpty
        ifTrue: [ ^false]
        ifFalse: [ISBN isValidIsbn10]

and then as you describe so have a isValidISBN10 method in the class
side where I  do the check there so  the class ISBN has the two instance
variables digits and controlDigit

Roelof






Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
In reply to this post by Richard O'Keefe
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


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
Op 2-9-2020 om 12:52 schreef Roelof Wobben via Pharo-users:

So no more hints from other people?

Roelof


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Richard O'Keefe
In reply to this post by 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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
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 <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
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 <[hidden email]> 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




Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Steffen Märcker
Maybe this is a naive question, but can you just split the task into the
following two?

1. Check whether whether the string is syntactically an ISBN number.
This can be done, e.g., using a regex.

2. Check the the check character.
Calculate the check character from the (now to be known) syntactically
valid string.

ISBNValidator>>isValidISBN: aString
   ^(self isSyntacticallyValid: aString) and: [self isCheckCharacterValid:
aString]

Kind regards,
Steffen

Am .09.2020, 07:35 Uhr, schrieb Roelof Wobben via Pharo-users
<[hidden email]>:

> 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 <[hidden email]> 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
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
In reply to this post by Pharo Smalltalk Users mailing list
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 <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
In reply to this post by Steffen Märcker
Op 6-9-2020 om 10:07 schreef Steffen Märcker:

> Maybe this is a naive question, but can you just split the task into the
> following two?
>
> 1. Check whether whether the string is syntactically an ISBN number.
> This can be done, e.g., using a regex.
>
> 2. Check the the check character.
> Calculate the check character from the (now to be known) syntactically
> valid string.
>
> ISBNValidator>>isValidISBN: aString
>   ^(self isSyntacticallyValid: aString) and: [self isCheckCharacterValid:
> aString]
>
> Kind regards,
> Steffen nder if the code will not be too big. I learned that it is good
>


Sorry to respond not earlier but your respons seems to be in the spam
folder of my provider.

I could do that but if very bad in regex so I do not know a regex which
van validate 123456789 or 123-456-78-9

Roelof


Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Steffen Märcker
No problem. I am not knowledgeable about isbn numbers. At which places may a dash occur?

Kind regards,
Steffen

07.09.2020 16:18:22 Roelof Wobben via Pharo-users <[hidden email]>:

> Op 6-9-2020 om 10:07 schreef Steffen Märcker:
>> Maybe this is a naive question, but can you just split the task into the
>> following two?
>>
>> 1. Check whether whether the string is syntactically an ISBN number.
>> This can be done, e.g., using a regex.
>>
>> 2. Check the the check character.
>> Calculate the check character from the (now to be known) syntactically
>> valid string.
>>
>> ISBNValidator>>isValidISBN: aString
>> ^(self isSyntacticallyValid: aString) and: [self isCheckCharacterValid:
>> aString]
>>
>> Kind regards,
>> Steffen nder if the code will not be too big. I learned that it is good
>>
> Sorry to respond not earlier but your respons seems to be in the spam folder of my provider.
>
> I could do that but if very bad in regex so I do not know a regex which van validate 123456789 or 123-456-78-9
>
> Roelof
>

Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Pharo Smalltalk Users mailing list
Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Steffen Märcker
Hi,

after reading the link and some additional sources, it turns out that a
valid ISBN-10 has either no separators or four blocks separated by either
dashes or spaces:
   Group-Publisher-Title-CheckDigit

Assuming Regex11 (and that I made no mistake), the following should do the
trick:

IsbnVarifier>>isSyntacticIsbn: aString
   "no groups"
   noGrouped := '\d{9}[0-9X]' asRegex.
   "groups separated by either dashes or spaces"
   dashes := '\d{1,7}-\d{1,7}-\d{1,7}-[0-9X]'
   spaces := '\d{1,7} \d{1,7} \d{1,7} [0-9X]'
   grouped := (dashed , '|' , spaces) asRegex.

   ^(aString matches: nonGrouped) or:
     [(aString matches: grouped) and:
       [aString size = 10 + 3]]

Surely, you could cleverly compress the regex even further but that does
not matter for this example. After checking the syntax, you can just
iterate over the string and compute the check-digit on the fly.

Kind regards,
Steffen

Am .09.2020, 18:19 Uhr, schrieb Roelof Wobben via Pharo-users
<[hidden email]>:

> See here for all the tests :
> https://github.com/exercism/pharo-smalltalk/blob/master/exercises/isbn-verifier/IsbnVerifierTest.class.st#L88
>
> Roelof
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Steffen Märcker
Now having a Workspace at hand, I fixed some minor typos:

IsbnVarifier>>isSyntacticIsbn: aString
   | nonGrouped dashes spaces grouped |
   nonGrouped := '\d{9}[0-9X]' asRegex.
   "groups separated by either dashes or spaces"
   dashes := '\d{1,7}-\d{1,7}-\d{1,7}-[0-9X]'.
   spaces := '\d{1,7} \d{1,7} \d{1,7} [0-9X]'.
   grouped := (dashes , '|' , spaces) asRegex.
   ^(aString matchesRegex: nonGrouped) or:
     [(aString matchesRegex: grouped) and:
       [aString size = (10 + 3)]]

Best, Steffen


Am .09.2020, 19:01 Uhr, schrieb Steffen Märcker <[hidden email]>:

> Hi,
>
> after reading the link and some additional sources, it turns out that a
> valid ISBN-10 has either no separators or four blocks separated by either
> dashes or spaces:
>    Group-Publisher-Title-CheckDigit
>
> Assuming Regex11 (and that I made no mistake), the following should do  
> the
> trick:
>
> IsbnVarifier>>isSyntacticIsbn: aString
>    "no groups"
>    noGrouped := '\d{9}[0-9X]' asRegex.
>    "groups separated by either dashes or spaces"
>    dashes := '\d{1,7}-\d{1,7}-\d{1,7}-[0-9X]'
>    spaces := '\d{1,7} \d{1,7} \d{1,7} [0-9X]'
>    grouped := (dashed , '|' , spaces) asRegex.
>
>    ^(aString matches: nonGrouped) or:
>      [(aString matches: grouped) and:
>        [aString size = 10 + 3]]
>
> Surely, you could cleverly compress the regex even further but that does
> not matter for this example. After checking the syntax, you can just
> iterate over the string and compute the check-digit on the fly.
>
> Kind regards,
> Steffen
>
> Am .09.2020, 18:19 Uhr, schrieb Roelof Wobben via Pharo-users
> <[hidden email]>:
>
>> See here for all the tests :
>> https://github.com/exercism/pharo-smalltalk/blob/master/exercises/isbn-verifier/IsbnVerifierTest.class.st#L88
>>
>> Roelof
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: Can it do this way ?

Richard O'Keefe
In reply to this post by Steffen Märcker
There are two quite different questions.
(1) Where may dashes occur in a real ISBN-10?
(2) What does Exercism require in the specification and check in the
test cases?

For (1) the rules are

Each ISBN consists of 5 elements with each section being separated by spaces or hyphens. Three of the five elements may be of varying length:

  • Prefix element – currently this can only be either 978 or 979. It is always 3 digits in length
  • Registration group element – this identifies the particular country, geographical region, or language area participating in the ISBN system. This element may be between 1 and 5 digits in length
  • Registrant element - this identifies the particular publisher or imprint. This may be up to 7 digits in length
  • Publication element – this identifies the particular edition and format of a specific title. This may be up to 6 digits in length
  • Check digit – this is always the final single digit that mathematically validates the rest of the number. It is calculated using a Modulus 10 system with alternate weights of 1 and 3.
An ISBN-10 does not have the three-digit prefix.  So we have
  [0-9]{1,5}   -- prefix
  [0-9]{1,7}   -- registrant
  [0-9]{1,6}   -- publication
  [0-9X]
      -- check digit

As an examplw, "Standard C++ IOStreams and Locales" by Langer & Kreft has
ISBN-10 0-201-18395-1
ISBN-13 9780201183955
so I shall assume the separators are optional.
/^[0-9]{1,5}[- ]?[0-9]{1,7}[- ]?[0-9]{1,6}[- ]?[0-9X]$/
Of course the elements cannot all have their maximum length at the same
time.  In AWK I would write
   x = a_putative_ISBN_10
   y = x
   gsub(/[- ]+/, "", y)
   if (x ~ /^[0-9]{1,5}[- ]?[0-9]{1,7}[- ]?[0-9]{1,6}[- ]?[0-9X]$/ \
    && y ~ /^[0-9]{9,9}[0-9X]$/ \
   ) {
       x *might* be valid, we still need to check the checksum
   }

For (2), there appear to be no restrictions on where dashes may occur
or how many: "These may be communicated with or without hyphens".
Exercism doesn't allow spaces.

Regular expressions are elegant in their own way, BUT for this problem
they are (a) excessive, (b) inefficient, and (c) insufficient.

   digit count := 0.
   check sum := 0.
   for each character c of the string
       if c is not a hyphen then
           if c is a digit then
               digit value := c's value as a digit
           else if c is X and digit count = 9 then
               digit value := 10
           else
               return false.
           digit count := digit count + 1.
           if digit count > 10 then return false.
           check sum := (11 - digit count) * digit value + check sum.
    return check sum mod 11 is zero.

Part of the insight here is "don't DO it, just PRETEND you did."
That is, instead of copying the string without the hyphens,
just ignore the hyphens as they turn up.
Another part is "if you are only going to use it once, don't store it."
That is, we need a digit's value just once, in the update to check sum,
so we should compute it just before we need it, not store it.

Now the pseudo-code above is classic sequential imperative coding.

Classic functional coding does something like
   let no_dashes = filter (/= '-') (explode string) in
   length no_dashes = 10 and
   let check = last no_dashes in
   (is_digit check or check = 'X') and
   all is_digit (take 9 no_dashes) and
   let xval c = if x = 'X' then 10 else digit_value c in
   dot (map xval no_dashes) [10,9..1]) mod 11 = 0

This pseudo-code translates nicely to Smalltalk too.
You might want to add

SequenceableCollection>>
  with: other inject: initial into: aBlock
    |r|
    r := initial.
    self with: other do: [:x :y |
      r := aBlock value: r value: x value: y].
    ^r
  dot: other
    ^self with: other inject: 0 into: [:acc :x :y | x*y + acc]

(These methods are so obvious that it would be absurd to claim
any intellectual property rights to them.)

I also have "fusion" methods like
SequenceableCollection>>
from: start to: finish allSatisfy: testBlock
  self from: start to: finish do: [:each |
    (aBlock value: each) ifFalse: [^false]].
  ^true

so that ((seq copyFrom: a to: z) allSatisfy: blk)
can be done as (seq from: a to: z allSatisfy: blk)
without making a copy.

Fusion methods are useful because Smalltall compilers
don't work as hard at eliminating intermediate data
structures as functional language compilers.  (Having
other priorities.)

   (isdigit (last no_dashes
           return false if digit count > 10.
          



On Tue, 8 Sep 2020 at 03:02, Steffen Märcker <[hidden email]> wrote:
No problem. I am not knowledgeable about isbn numbers. At which places may a dash occur?

Kind regards,
Steffen

07.09.2020 16:18:22 Roelof Wobben via Pharo-users <[hidden email]>:

> Op 6-9-2020 om 10:07 schreef Steffen Märcker:
>> Maybe this is a naive question, but can you just split the task into the
>> following two?
>>
>> 1. Check whether whether the string is syntactically an ISBN number.
>> This can be done, e.g., using a regex.
>>
>> 2. Check the the check character.
>> Calculate the check character from the (now to be known) syntactically
>> valid string.
>>
>> ISBNValidator>>isValidISBN: aString
>> ^(self isSyntacticallyValid: aString) and: [self isCheckCharacterValid:
>> aString]
>>
>> Kind regards,
>> Steffen nder if the code will not be too big. I learned that it is good
>>
> Sorry to respond not earlier but your respons seems to be in the spam folder of my provider.
>
> I could do that but if very bad in regex so I do not know a regex which van validate 123456789 or 123-456-78-9
>
> Roelof
>

12