I'm looking for some feedback on my code for the collatz function exercise in the Python book Automate the Boring Stuff. It works, but doesn't feel very elegant to me. Is there a better way to approach it? I'm trying to avoid just looking at someone else's solution so general advice would be preferred.

def collatz(number): 
 if (number %2 ==0 ) :#even
 newNumber=number//2
 print(newNumber);
 if newNumber!=1:
 collatz(newNumber);
 else:
 return (newNumber)
 elif (number % 2 == 1) : #odd
 newNumber=3*number+1
 print (newNumber)
 collatz(newNumber)
 return newNumber;


#Input validation loop
while True:
 try:
 number = int(input("Enter number:")) 
 except ValueError:
 print("Enter a valid integer")
 continue
 else:
 if number < 1:
 print("enter a valid positive integer")
 continue
 break

collatz(number)

Cleaning the user input validation

• Remove continue because in both cases the loop will continue anyway, only break will stop it, so do not worry.


• Do not make the user think: number = int(input("Enter number:")): which number to enter? Sure you know, but messages are intended to communicate with the user, so do not make him guess what you want, but be clear and precise with him.

Cleaning collatz()

• Please apply the naming conventions and indentation rules.


• Leave space between operators and their operands (example: newNumber=3*number+1 should be written newNumber = 3 * number + 1)


• Do not borrow ; sysntax into Python. You should remove it.


• Remove unnecessary comments: when you code (number %2 ==0 ) :#even, I think everybody is smart enough to guess you are processing the case where number is even. The same thing goes for #odd and #Input validation loop.


• Remove return newNumber, it does not make sense because return (newNumber) is enough to exit the recursion when newNumber == 1.

So under the shadow of what is mentioned above, let us clean collatz():

def collatz(number): 
 if (number %2 == 0 ):
 new_number = number // 2
 print(new_number);
 if new_number!=1:
 collatz(new_number);
 else:
 return new_number
 elif (number % 2 == 1): 
 new_number = 3 * number + 1
 print (new_number)
 collatz(new_number)

Improving the input validation

• What you have done so far is good, I suggest you to handle the case where the user presses Ctrl + c to exit your program, because when I tried to do so, I got an ugly KeyboardInterrupt exception.


• Think of code reuse, I mean the input validation could be re-structured as a function, this way you can use it elsewhere easily if similar purposes are encountered.

Given these last 2 elements, let us re-write the code validation:

def validate_user_input(): 
 while True:
 try:
 number = int(input("Enter a positive integer number ( > 1): ")) 
 except KeyboardInterrupt:
 print('nSee you next time!')
 break
 except ValueError:
 print("That was not even a valid integer!") 
 else:
 if number < 0:
 print("Positive integer, please!") 
 elif number < 2:
 print("Integer should be at least equal to 2 !") 
 else:
 return number

Improving collatz()

You said you do not want to see other solutions and you want only to see if you can improve your own one. So let us see if we can do so.

From your solution we can see:

1. You always call collatz() whether new_number is odd or even.


2. You always print new_number whether it is odd or even


3. The only useful return statement is the once which corresponds to new_number = 1 .

Let us translate the 3 phrases above into code:

1. callatz(new_number)


2. print(new_number)


3. return new_number

We should not forget the shared fact: new_number can be either odd or even (so we do not care about that)

4. new_number = n//2 if n % 2 == 0 else n*3 + 1

Now we are ready to gather the 4 instructions listed above into one following their coherent flow:

def collatz(n):
 if n == 1: 
 return
 else:
 new_number = n // 2 if n % 2 == 0 else n * 3 + 1
 print(new_number) 
 collatz(new_number)

Putting all together

Let us gather the code written so far to create an MCVE:

def validate_user_input(): 
 while True:
 try:
 number = int(input("Enter a positive integer (minimum 2): ")) 
 except KeyboardInterrupt:
 print('nSee you next time!')
 break
 except ValueError:
 print("That was not even a valid integer!") 
 else:
 if number < 0:
 print("Positive integer, please!") 
 elif number < 2:
 print("Integer should be at least equal to 2 !") 
 else:
 return number 

def collatz(n):
 if n == 1: 
 return
 else:
 new_number = n//2 if n % 2 == 0 else n*3 + 1
 print(new_number) 
 collatz(new_number) 

if __name__ == '__main__':

 print('Collatz Conjecture Collatz Conjecture')
 number = validate_user_input()
 collatz(number)

P.S. Just in case: you can read about if __name__ == "__main__":

Some quick thoughts:

1. Linting the code will make it more easily readable to Python developers (including yourself when you get used to it).


2. 
   elif (number % 2 == 1) : could just be else: because a number which is not even must be odd.


3. 
   return (newNumber) should be just return newNumber.


4. The code below #Input validation loop should be in a main function, called if __name__ == '__main__'. That way it's possible to import your function for reuse in other code.


5. 
   

   Rather than returning just the next number in the sequence you probably want to return an infinite iterator so that the caller can get more numbers trivially:

   
   
   

   class Collatz():
    def __init__(self, start):
    self.number = start
   
    def __iter__(self):
    return self
   
    def __next__(self):
    if self.number in [0, 1]:
    raise StopIteration()
    if self.number % 2 == 0:
    self.number = int(self.number / 2)
    else:
    self.number *= 3
    self.number += 1
    return self.number
   

   
   
   

   Use:

   
   
   

   >>> print(list(Collatz(15)))
   [46, 23, 70, 35, 106, 53, 160, 80, 40, 20, 10, 5, 16, 8, 4, 2, 1]
   

   
   

To promote code reuse, you should design the collatz() function so that it doesn't print anything. Functions should either perform computations or I/O, not both. Furthermore, there should be as little "free-floating" code as possible, and the input-prompting loop should be in a function. Ideally, the main body of the program should look like this:

for n in collatz(ask_positive_int()):
 print(n)

Your collatz() function works recursively. For long sequences, that will cause your program to crash due to stack overflow. It should be written using a loop instead.

To avoid mixing I/O with the computations, you should yield the results instead of print()ing them. That makes your function a Python generator.

Your function does not print the initial number. I find that behaviour to be a bit counterintuitive. It also complicates the code a bit, making you write two print(newNumber) calls.

You don't need a newNumber variable at all; you can just overwrite the number parameter.

def collatz(n):
 """
 Generate the Collatz sequence, starting from n, until 1 is reached.
 The output starts with n and ends with 1.
 """
 while True:
 yield n
 if n % 2 == 0:
 n //= 2
 elif n == 1:
 break
 else:
 n = 3 * n + 1

def ask_positive_int():
 """Ask the user to enter a positive integer, retrying on invalid input."""
 while True:
 try:
 n = int(input("Enter a positive integer: "))
 if n < 1:
 print("Number must be positive")
 else:
 return n
 except ValueError:
 print("Number must be an integer")

for n in collatz(ask_positive_int()):
 print(n)

number%2 returns the remainder of number when divided by 2. Python is not a strongly typed language, so this remainder can be used as a boolean; if there is a remainder, number%2 is truthy, and if the remainder is zero, then it is falsy. So you can replace your if (number %2 ==0 ) [even stuff] else [odd stuff] with if (number %2) [odd stuff] else [even stuff].

if newNumber!=1:
 collatz(newNumber);

These lines seem to be based on the assumption that all numbers eventually reach 1. While this has been verified for numbers you are likely to come across, it hasn't been proven for all numbers. You might want to instead check whether you've entered a loop. Also I think it's better to explicitly return whether than relying on Python returning the last line executed by default, so you should have return(collatz(newNumber)). And get rid of the semicolon.

Also, you're repeating a lot of code. The only difference between the odd and even case is that in the former case you have 3*number+1 and in the latter case you have number//2. The print and return statements are the same. So you can simplify your code to the following:

if number%2:
 newNumber = 3*number+1
else:
 newNumber = number//2 
print(newNumber)
if newNumber == 1:
 return(newNumber)
else:
 return(collatz(newNumber))

return newNumber;

try:
number = int(input("Enter number:"))

Keep in mind that this will reject 5.0.