On Sun., 3 May 2020, 3:48 pm Roelof Wobben, <r.wob...@home.nl> wrote:
> Op 3-5-2020 om 04:24 schreef Ben Coman: > > > > 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 > > > Chips, but then it does exactly the other way around that I wanted. > > 7 is a non-even index 7 \\ 2 gives 1 + 1 gives 2 so the number gets > multiplied by 2 where its must be multiplied by 1. > 8 is a even index 8 \\ 2 gives 0 + 1 gives 1 so the number gets > muliplied by 1 where it must be multiplied by 2. > Sorry, I worked from the example of your original post. >>> 4539 1488 0343 6467 >>> 4_3_ 1_8_ 0_4_ 6_6_ >>> 8569 2478 0383 3437 >>> 8+5+6+9+2+4+7+8+0+3+8+3+3+4+3+7 = 80 where it looked like the odd indexes needed doubling and I was getting a checksum of 80 so that seemed correct. If you want to double the even indexes try (2 - (idx\\2)). cheers -ben
