[Cuis-dev] feedback on simple code

[Jaromir Matas]

Hi Mark,

just for fun I tried to "make the code shorter":

Collatz class >> gather: anInt seq: aSeq
     "populates an OrderedCollection with the Collatz sequence for a 
given positive integer"

     anInt = 1 ifTrue: [^aSeq].
     ^self gather: (aSeq addLast: (self next: anInt)) seq: aSeq


Collatz class >> sequence: anInteger
     "answers an OrderedCollection containing the Collatz sequence for a 
given positive integer"

     ^self gather: anInteger seq: (OrderedCollection with: anInteger)

The #next methods remains the same.

You could even get rid of the second parameter of #gather:seq: method 
byt making the sequence a class instance variable.

I'm aware readability may degrade this way so take it just as a fun 
exercise :)

Re your simplified code under the Integer class I'd remove the return 
from ^seq at the end - it's redundant.

regards,
Jaromir


On 05-Jun-24 8:16:57 PM, "Mark Volkmann via Cuis-dev" 
<cuis-dev at lists.cuis.st> wrote:

>I took another shot at simplifying the code. I didn't need to use 
>recursion. I can do this instead as an instance method added to the 
>Integer class:
>
>collatz
>     "answers an OrderedCollection containing the Collatz sequence for 
>this integer"
>
>     | next seq |
>     ^self < 1
>         ifTrue: nil
>         ifFalse: [
>             next := self.
>             seq := OrderedCollection new.
>             seq addLast: next.
>             [next = 1] whileFalse: [
>                 next := next even ifTrue: [next / 2] ifFalse: [next * 3 
>+ 1].
>                 seq addLast: next
>             ].
>             ^seq
>         ]
>
>Beside the lack of memoization and the "orbit" concept, what would you 
>change about this?
>
>
>On Wed, Jun 5, 2024 at 12:40 PM Mark Volkmann 
><r.mark.volkmann at gmail.com> wrote:
>>See my replies inline below with some parts snipped to focus my 
>>replies.
>>
>>On Wed, Jun 5, 2024 at 12:19 PM Andres Valloud via Cuis-dev 
>><cuis-dev at lists.cuis.st> wrote:
>>
>>>Always be on the lookout for when a receiver is just self.  
>>>Otherwise,
>>>the code is not using the knowledge it has of where it is implemented
>>>(and this leads to all sorts of problems later).
>>
>>Ah! I did not know that I could use `self` to refer to the class in a 
>>class method.
>>
>>>Also, you can accomplish the same with much less code.  Any time you
>>>have a class whose class methods offer a "service" but do not really
>>>have instances, you should suspect the code should be placed 
>>>elsewhere.
>>
>>I see that I don't need the next: method and can just move that code 
>>in the gather:seq: method.
>>But it's not apparent to me what else I can do to make the code 
>>shorter.
>>
>>>Side comment: I've been working on the Collatz problem for a *long*
>>>time, so I have quite a few opinions on how to go about writing
>>>specifically this code...
>>
>>I know I could add caching of results to make subsequent calls with 
>>different integers much faster.
>>
>>>But in this particular case, what you're collecting is the orbit of 
>>>an
>>>integer (so, anInteger, not aNumber).
>>
>>Excellent point! I changed every occurrence of `aNumber` to 
>>`anInteger` in my code.
>>
>>--
>>R. Mark Volkmann
>>Object Computing, Inc.
>
>
>--
>R. Mark Volkmann
>Object Computing, Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20240606/17dc0a15/attachment-0001.htm>