<div dir="ltr">Andres, thank you so much for the feedback! I didn't know I could return early in a method like you showed and hadn't considered the difference between / and //.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 5, 2024 at 5:37 PM Andres Valloud via Cuis-dev <<a href="mailto:cuis-dev@lists.cuis.st">cuis-dev@lists.cuis.st</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi, ifTrue: is supposed to take a block argument, so it should be <br>
ifTrue: [nil].  And to prevent indenting the entire method, consider<br>
<br>
        self < 1 ifTrue: [^nil].<br>
        ... etc...<br>
        ^seq<br>
<br>
It's perfectly fine to have multiple return statements.<br>
<br>
By the way, use // 2 instead of / 2, because / 2 will try to make a <br>
fraction to then discover that next was even.  But now you know next is <br>
even, so you can do next // 2.  Using / is still correct of course.<br>
<br>
I wouldn't worry about doing bitShift: -1 instead of // 2 here.  That <br>
kind of "speedup" is barking at the wrong forest (not even the wrong <br>
tree) from the perspective of Collatz.  I would familiarize myself with <br>
the difference between the pairs "//, \\", "quo:, rem:", and "div:, <br>
mod:", however.<br>
<br>
On 6/5/24 11:16 AM, Mark Volkmann via Cuis-dev wrote:<br>
> I took another shot at simplifying the code. I didn't need to use <br>
> recursion. I can do this instead as an instance method added to the <br>
> Integer class:<br>
> <br>
> collatz<br>
>      "answers an OrderedCollection containing the Collatz sequence for <br>
> this integer"<br>
> <br>
>      | next seq |<br>
>      ^self < 1<br>
>          ifTrue: nil<br>
> ifFalse: [<br>
> next := self.<br>
> seq := OrderedCollection new.<br>
> seq addLast: next.<br>
> [next = 1] whileFalse: [<br>
> next := next even ifTrue: [next / 2] ifFalse: [next * 3 + 1].<br>
> seq addLast: next<br>
> ].<br>
> ^seq<br>
> ]<br>
> <br>
> Beside the lack of memoization and the "orbit" concept, what would you <br>
> change about this?<br>
> <br>
> <br>
> On Wed, Jun 5, 2024 at 12:40 PM Mark Volkmann <<a href="mailto:r.mark.volkmann@gmail.com" target="_blank">r.mark.volkmann@gmail.com</a> <br>
> <mailto:<a href="mailto:r.mark.volkmann@gmail.com" target="_blank">r.mark.volkmann@gmail.com</a>>> wrote:<br>
> <br>
>     See my replies inline below with some parts snipped to focus my replies.<br>
> <br>
>     On Wed, Jun 5, 2024 at 12:19 PM Andres Valloud via Cuis-dev<br>
>     <<a href="mailto:cuis-dev@lists.cuis.st" target="_blank">cuis-dev@lists.cuis.st</a> <mailto:<a href="mailto:cuis-dev@lists.cuis.st" target="_blank">cuis-dev@lists.cuis.st</a>>> wrote:<br>
> <br>
>         Always be on the lookout for when a receiver is just self. <br>
>         Otherwise,<br>
>         the code is not using the knowledge it has of where it is<br>
>         implemented<br>
>         (and this leads to all sorts of problems later).<br>
> <br>
> <br>
>     Ah! I did not know that I could use `self` to refer to the class in<br>
>     a class method.<br>
> <br>
>         Also, you can accomplish the same with much less code.  Any time<br>
>         you<br>
>         have a class whose class methods offer a "service" but do not<br>
>         really<br>
>         have instances, you should suspect the code should be placed<br>
>         elsewhere.<br>
> <br>
> <br>
>     I see that I don't need the next: method and can just move that code<br>
>     in the gather:seq: method.<br>
>     But it's not apparent to me what else I can do to make the code shorter.<br>
> <br>
>         Side comment: I've been working on the Collatz problem for a *long*<br>
>         time, so I have quite a few opinions on how to go about writing<br>
>         specifically this code...<br>
> <br>
> <br>
>     I know I could add caching of results to make subsequent calls with<br>
>     different integers much faster.<br>
> <br>
>         But in this particular case, what you're collecting is the orbit<br>
>         of an<br>
>         integer (so, anInteger, not aNumber).<br>
> <br>
> <br>
>     Excellent point! I changed every occurrence of `aNumber` to<br>
>     `anInteger` in my code.<br>
> <br>
>     -- <br>
>     R. Mark Volkmann<br>
>     Object Computing, Inc.<br>
> <br>
> <br>
> <br>
> -- <br>
> R. Mark Volkmann<br>
> Object Computing, Inc.<br>
> <br>
-- <br>
Cuis-dev mailing list<br>
<a href="mailto:Cuis-dev@lists.cuis.st" target="_blank">Cuis-dev@lists.cuis.st</a><br>
<a href="https://lists.cuis.st/mailman/listinfo/cuis-dev" rel="noreferrer" target="_blank">https://lists.cuis.st/mailman/listinfo/cuis-dev</a><br>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div><font face="arial, helvetica, sans-serif">R. Mark Volkmann</font></div><div><span style="font-size:12.8px"><font face="arial, helvetica, sans-serif">Object Computing, Inc.</font></span></div></div></div></div></div></div></div></div>