check with the finder and example S.
> On 2 May 2020, at 21:09, Roelof Wobben <r.wob...@home.nl> wrote: > > > From: Roelof Wobben <r.wob...@home.nl> > Subject: Re: [Pharo-users] mentor question 4 > Date: 2 May 2020 at 21:09:12 CEST > To: Ben Coman <b...@openinworld.com>, Any question about pharo is welcome > <pharo-users@lists.pharo.org> > > > 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 <mailto: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 >> >> <mailto: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 >> >>>> <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/ >> <https://talixa.com/blog/why-i-write-simple-code/> >> >> In general, you want to avoid premature optimization. >> https://effectiviology.com/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 %. > > Roelof > > -------------------------------------------- Stéphane Ducasse http://stephane.ducasse.free.fr / http://www.pharo.org 03 59 35 87 52 Assistant: Julie Jonas FAX 03 59 57 78 50 TEL 03 59 35 86 16 S. Ducasse - Inria 40, avenue Halley, Parc Scientifique de la Haute Borne, Bât.A, Park Plaza Villeneuve d'Ascq 59650 France
