On Sun, 3 May 2020 at 03:09, Roelof Wobben <r.wob...@home.nl> wrote: > Op 2-5-2020 om 19:33 schreef Ben Coman: > > > > On Sat, 2 May 2020 at 15:52, Roelof Wobben via Pharo-users < > pharo-users@lists.pharo.org> wrote: > >> 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... > https://talixa.com/blog/why-i-write-simple-code/ > > In general, you want to avoid premature optimization. > https://effectiviology.com/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 > > > > I think I find readability more important then efficieny. > > But I think I have to alter your code. At first look it does not look like > that every second value from the right is multiplied by 2. > > When the index is for example 7 . > > 7 // 2 gives 3. > Maybe it needs to be %. >
But why are you testing // when my code uses \\ ? :) small details can have a big impact. cheers -ben
