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
