Op
1-5-2020 om 08:35 schreef Roelof
Wobben:
>> On Fri, 1 May 2020 at
02:16, Roelof Wobben <r.wob...@home.nl>
wrote:
>>> Op 30-4-2020 om 16:06
schreef Richard O'Keefe:
>>>> This sounds very
much like the Luhn test task at
RosettaCode.
>>>> https://rosettacode.org/wiki/Luhn_test_of_credit_card_numbers
>>>> except that there
it is described as working on the
digits of an
>>>> integer.
>>>>
>>>> (1) There are two
approaches to traversing a
sequence in reverse.
>>>> (A) Reverse
the sequence, then traverse the
copy forward.
>>>> aString
reverse do: [:each | ...]
>>>> (B) Just
traverse the sequence in reverse
>>>> aString
reverseDo: [:each | ...]
>>>> My taste is
for the second.
>>>>
>>>> (2) There are two
approaches to deleting spaces.
>>>> (A) Make a
copy of the string without spaces.
>>>> x :=
aString reject: [:each | each =
Character space].
>>>> x do:
...
>>>> (B) Ignore
spaces as you go:
>>>> (i)
aString do: [:each | each =
Character space ifFalse:
>>>> [...]]
>>>> (ii)
aString select: [:each | each ~=
Character space]
>>>> thenDo:
>>>> [:each | ...]
>>>>
>>>> Combining (1A)
and (2A) you get very obvious
code:
>>>> (aString
reject: [:each | each = Character
space]) reverse do:
>>>> [:digit } ...]
>>>> Combining (1B)
and (2Bi) you get more efficient
code:
>>>> aString
reverseDo: [:digit |
>>>> digit =
Character space ifFalse: [ ...]]
>>>>
>>>> By the way, let's
start by checking that the
character in the
>>>> string *are*
>>>> digits or spaces:
>>>> (aString
allSatisfy: [:each | each isDigit
or: [each =
>>>> Character
s[ace]])
>>>>
ifFalse: [^false],
>>>>
>>>> (3) There are two
approaches to doubling the even
digits.
>>>> (A) Make a
new string that starts as a copy
and change every
>>>> second
>>>> digit
from the right.
>>>> (B) Simply
*act* as if this has been done;
keep track of
>>>> whether the
>>>> current
digit position is even or odd and
multiply by 1
>>>> or 2 as
>>>>
appropriate.
>>>> nextIsOdd
:= true.
>>>> aString
reverseDo: [:digit |
>>>> digit =
Character space ifFalse: [
>>>>
nextIsOdd
>>>>
ifTrue: [oddSum := ...]
>>>>
ifFalse: [evenSum := ...].
>>>>
nextIsOdd := nextIsOdd not]].
>>>>
>>>> I *like* code
that traverses a data structure
exactly once and
>>>> allocates no
intermediate garbage, so I'd be
making (B) choices.
>>>>
>>>>
>>> For me , I use this
to practice solving problems and
doing the
>>> "right"
>>> steps.
>>> So I love it , that
so many people share there way of
solving it.
>>> I can learn a lot
from it
>>> Expecially when they
explain there thinking process so
detailed.
>>>
>>> I like this code also
a lot.
>>> Am I correct for
testing if it is a valid string by
doing this ^
>>> (oddSum + evenSum)
dividedBy: 10
>>>
>>> Roelof
>>>
>
>
> oke,
>
> so this is better
>
> cardNumber := '8273 1232 7352
0569'.
> oddSum := 0.
> evenSum := 0.
> nextIsOdd := false.
> cardNumber reverseDo:
[:character |
> digit := character
digitValue.
> character =
Character space ifFalse: [
> nextIsOdd
> ifFalse:
[oddSum := oddSum + digit ]
> ifTrue: [(digit
>= 5 )
> ifTrue: [evenSum :=
evenSum + (digit * 2) - 9 ]
> ifFalse: [ evenSum :=
evenSum + (digit * 2) ]].
> nextIsOdd :=
nextIsOdd not]].
> ^ evenSum + oddSum // 10 ==
0.
>
>
> where I could even make a
seperate method of the ifTrue
branch when
> the digit is greater then 5.
>
>
nobody who can say if this is a
good solution ?
You didn't actually ask a
question :)
btw, Its good you are
stretching yourself to do it the
most complicated way,
but pay attention that when
Richard says HE "likes code that
traverses a data structure exactly
once"
its because he is starting with
the philosophy to OPTIMISE for
EFFICIENCY.
Often in programming that is
the LAST thing you should do
because to do so your code often
becomes more complex,
and Kernagan's law applies...
In general, you want to avoid
premature optimization.
The catch-phrase I see around
this forum priorities things in
this order:
1. Make it work.
2. Make it right.
3. Make it fast.
and often you can stop at 2
because it is already "fast
enough".
Considering your code above, if
I take TIME to go through it, I
can evaluate that it seems right
but I remain feeling that I
could have missed something.
Something like Richard's (A)
examples without loops I would
consider to be "more good".
since each line can be dealt
with individually.
cardNumber := '4539 1488 0343
6467'.
cleanDigits := cardNumber
copyWithout: Character space.
preWrapDigits := cleanDigits
asArray collectWithIndex: [
:char :idx | (idx\\2 + 1) * char
digitValue ].
wrappedDigits :=
preWrapDigits collect: [ :d | d
> 9 ifFalse: [ d ] ifTrue: [
d-9 ] ].
^ wrappedDigits sum
isDivisibleBy: 10.
A side benefit is having full
result of each transformation
available to be reviewed,
rather than having only
pieces of it in view when you
debug a loop.
In the end though, yours is a
good solution first and foremost
because tried a more detailed
way and you got it working!!
Remember there is more than
one "good way" to do it
depending on what you are
optimising for - efficiency or
readability.
cheers -ben
