[Cuis-dev] feedback on simple code

[Andres Valloud]

Hello, couple things...

Collatz class>>gather: aNumber seq: aSeq

	aNumber = 1
		ifTrue: [aSeq addLast: 1]
		ifFalse: [
			| next |
			next := Collatz next: aNumber.
			aSeq addLast: next.
			next = 1 ifFalse: [Collatz gather: next seq: aSeq]
		]


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).  So the above really means

Collatz class>>gather: aNumber seq: aSeq

	aNumber = 1
		ifTrue: [aSeq addLast: 1]
		ifFalse: [
			| next |
			next := Collatz next: aNumber.
			aSeq addLast: next.
			next = 1 ifFalse: [self gather: next seq: aSeq]
		]


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.

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

But in this particular case, what you're collecting is the orbit of an 
integer (so, anInteger, not aNumber).  Here, "orbit" is a mathematical 
term from dynamical systems and it refers to the sequence n, f(n), 
f(f(n)), etc.  Collatz orbits are very interesting by themselves, and 
you can ask them lots of very interesting questions.  So, rather than an 
ordered collection, I would create a new class called CollatzOrbit to 
hold the orbit.  And I would not make it a subclass of Object, not 
OrderedCollection, because the math properties of such orbits do not 
really belong in the Collection hierarchy.

Then, the orbit class would have an instance variable, e.g. "orbit", 
initialized when the object is created, like this:

	CollatzCkOrbit class>>startingAt: anInteger

		^self new
			startingAt: anInteger;
			yourself

	CollatzCkOrbit>>startingAt: anInteger

		| newOrbit |
		newOrbit := self newOrbitStartingAt: anInteger.
		self orbit: newOrbit


In addition to all this, strive to accomplish the same goal with the 
least possible code (within reason, this is not an obfuscated C 
contest).  So,

	CollatzCkOrbit>>newOrbitStartingAt: anInteger

		| answer |
		anInteger < 1 ifTrue: [self error: 'Very interesting, but for 
simplicity not here please'].
		answer := OrderedCollection with: anInteger.
		[answer last = 1] whileFalse:
			[answer add: answer last ck].
		^answer

	Integer>>ck

		^self even ifTrue: [self // 2] ifFalse: [self * 3 + 1]

	Integer>>ckOrbit

		^CollatzCkOrbit startingAt: anInteger


That name ck refers to C(k) in the literature.  But if you will look at 
this further, then use T(k) instead because it has way more interesting 
properties.

	Integer>>tk

		^self even
			ifTrue: [self // 2]
			ifFalse: [self * 3 + 1 // 2]

	Integer>>tkOrbit

		^CollatzTkOrbit startingAt: anInteger


And the class hierarchy should look like this (for mathematical reasons):

	AbstractCollatzOrbit
		CollatzCkOrbit
		CollatzTkOrbit


so newOrbitStartingAt: should be in the Abstract superclass etc to 
prevent code duplication.

I just wrote all this code in the email, I didn't test it.  Finally, 
note the effort to have code that references class names the least 
possible times.


On 6/5/24 9:56 AM, Mark Volkmann via Cuis-dev wrote:
> I've been learning Smalltalk for the past week or so. I wrote some code 
> to compute the Collatz sequence of a given number described here: 
> https://en.wikipedia.org/wiki/Collatz_conjecture 
> <https://en.wikipedia.org/wiki/Collatz_conjecture>
> 
> I'd love it if an experienced Smalltalker could look at my 
> implementation and tell me what they would do differently. Here's a 
> fileOut of my code. It's not very long.
> 
> -- 
> R. Mark Volkmann
> Object Computing, Inc.
>