[Cuis-dev] feedback on simple code

[Andres Valloud]

Hi, ifTrue: is supposed to take a block argument, so it should be 
ifTrue: [nil].  And to prevent indenting the entire method, consider

	self < 1 ifTrue: [^nil].
	... etc...
	^seq

It's perfectly fine to have multiple return statements.

By the way, use // 2 instead of / 2, because / 2 will try to make a 
fraction to then discover that next was even.  But now you know next is 
even, so you can do next // 2.  Using / is still correct of course.

I wouldn't worry about doing bitShift: -1 instead of // 2 here.  That 
kind of "speedup" is barking at the wrong forest (not even the wrong 
tree) from the perspective of Collatz.  I would familiarize myself with 
the difference between the pairs "//, \\", "quo:, rem:", and "div:, 
mod:", however.

On 6/5/24 11:16 AM, Mark Volkmann via Cuis-dev 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 
> <mailto: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 <mailto: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.
>