[Cuis-dev] feedback on simple code

[Mark Volkmann]

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/20240605/09b2e1f0/attachment.htm>