[Cuis-dev] feedback on simple code

[Mark Volkmann]

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 //.

On Wed, Jun 5, 2024 at 5:37 PM Andres Valloud via Cuis-dev <
cuis-dev at lists.cuis.st> wrote:

> 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.
> >
> --
> Cuis-dev mailing list
> Cuis-dev at lists.cuis.st
> https://lists.cuis.st/mailman/listinfo/cuis-dev
>


-- 
R. Mark Volkmann
Object Computing, Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20240606/8203b522/attachment.htm>