Xjyuk

Can I get a review of my code for the the Collatz Sequence from Chapter three of Automate the Boring Stuff with Python?

The Collatz Sequence

Write a function named collatz() that has one parameter named number.
If number is even, then collatz() should print number // 2 and return
this value. If number is odd, then collatz() should print and return 3
* number + 1.

Then write a program that lets the user type in an integer and that
keeps calling collatz() on that number until the function returns the
value 1. (Amazingly enough, this sequence actually works for any
integer—sooner or later, using this sequence, you’ll arrive at 1! Even
mathematicians aren’t sure why. Your program is exploring what’s
called the Collatz sequence, sometimes called “the simplest impossible
math problem.”)

Remember to convert the return value from input() to an integer with
the int() function; otherwise, it will be a string value.

Hint: An integer number is even if number % 2 == 0, and it’s odd if
number % 2 == 1.

The output of this program could look something like this:

Enter number:
3
10
5
16
8
4
2
1

Input Validation

Add try and except statements to the previous project to detect
whether the user types in a noninteger string. Normally, the int()
function will raise a ValueError error if it is passed a noninteger
string, as in int('puppy'). In the except clause, print a message to
the user saying they must enter an integer.

I am mainly wondering if there is a cleaner way to write my solution.

def collatz(num):
 while num > 1:
 if num % 2 == 0:
 print(num//2)
 num = num //2
 elif num % 2 ==1:
 print(3*num+1)
 num = 3*num+1
 else:
 print(num)

def getNum():
 global num
 num = (input("> "))
 try:
 num = int(num)
 except ValueError:
 print('plese enter a number')
 getNum()
getNum()
collatz(num)

Prompt

The most obvious bad practice here is the use of a global variable. Instead of setting num as a side-effect, your function should return the result.

getNum() is not such a good name for the function. PEP 8, the official style guide for Python, says that function names should be lower_case_with_underscores. Furthermore, "get" implies that the function is retrieving a piece of data that is already stored somewhere, which is not the case here. Finally, "Num" should be more specific.

The use of recursion is not appropriate. If you want a loop, write a loop.

def ask_integer():
 """
 Return an integer entered by the user (repeatedly prompting if
 the input is not a valid integer).
 """
 while True:
 try:
 return int(input("> "))
 except ValueError:
 print("Please enter an integer")

num = ask_integer()

collatz function

Strictly speaking, you didn't follow the instructions. Your solution isn't wrong or bad — you just didn't implement the collatz function according to the specification that was given, which says that you should print and return one single number.

def collatz(num):
 """
 Given a number, print and return its successor in the Collatz sequence.
 """
 next = num // 2 if num % 2 == 0 else 3 * num + 1
 print(next)
 return next

num = ask_integer()
while num > 1:
 num = collatz(num)

First, note how you're duplicating calculations:

print(num//2)
num = num //2

This may not cause issues with this specific code, but it isn't a good practice. You're doing twice as much work as you need to, which can cause performance issues once you start writing more complicated code. Do the calculation once, and save the result. In this case though, all you need to do is reverse those lines and use num:

num = num // 2
print(num)

Also, make sure you have proper spacing around operators, and be consistent.

Your if and elif cases are exclusive of each other, and your else should never happen. If the first condition is true, then other must be false and vice-versa. There's no need for the second check. Once rewritten, you'll see that printing in every case isn't necessary. You can just print after:

while num > 1:
 if num % 2 == 0:
 num = num // 2

 else:
 num = 3 * num + 1

 print(num)

Since you're just reassinging num one of two options based on a condition, a conditional expression can be used here cleanly as well:

while num > 1:
 num = (num // 2) if num % 2 == 0 else (3 * num + 1)
 print(num)

The braces aren't necessary, but I think they're useful here due to the number of operators involved.

Printing the numbers isn't ideal here. In most code, you need to be able to use the data that you produce. If you wanted to analyze the produced sequence, you would have to do something intercept the stdout, which is expensive and overly complicated. Make it a function that accumulates and returns a list. In the following examples, I also added some type hints to make it clearer what the type of the data is:

from typing import List

def collatz(starting_num: int) -> List[int]:
 nums = [starting_num]

 num = starting_num
 while num > 1:
 num = (num // 2) if num % 2 == 0 else (3 * num + 1)
 nums.append(num)

 return nums

Or, a much cleaner approach is to make it a generator that yields the numbers:

# Calling this function returns a generator that produces ints
# Ignore the two Nones, as they aren't needed for this kind of generator
def collatz_gen(starting_num: int) -> Generator[int, None, None]:
 yield starting_num

 num = starting_num
 while num > 1:
 num = (num // 2) if num % 2 == 0 else (3 * num + 1)
 yield num

>>> list(collatz_gen(5))
[5, 16, 8, 4, 2, 1]

There's a few notable things about getNum:

Python uses "snake_case", not "camelCase".

Your use of global num here is unnecessary and confusing. Just like before, explicitly return any data that the function produces:

def get_num() -> int:
 raw_num = input("> ")

 try:
 return int(raw_num)

 except ValueError:
 print('Please enter a number')
 return get_num()

Note how instead of reassigning a global num, we're just returning the number. I also spaced things out a bit, and used some more appropriate names. Conceptually, I'd say that num = input("> ") is wrong. At the time that that runs, num does not contain a number (it contains a string).

This isn't a good use of recursion. It likely won't cause you any problems, but if your user is really dumb and enters wrong data ~1000 times, your program will crash. Just use a loop:

def get_num() -> int:
 while True:
 raw_num = input("> ")

 try:
 return int(raw_num)

 except ValueError:
 print('Please enter a number')

In languages like Python, be careful using recursion in cases where you have no guarantees about how many times the function will recurse.

I'd also probably name this something closer to ask_for_num. "get" doesn't make it very clear about where the data is coming from.

Taken altogether, you'll end up with:

from typing import Generator

def collatz_gen(starting_num: int) -> Generator[int, None, None]:
 yield starting_num

 num = starting_num
 while num > 1:
 num = (num // 2) if num % 2 == 0 else (3 * num + 1)
 yield num

def ask_for_num() -> int:
 while True:
 raw_num = input("> ")

 try:
 return int(raw_num)

 except ValueError:
 print('Please enter a number')

Which can be used like:

num = ask_for_num()

for n in collatz_gen(num):
 print(n)