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
