7
\$\begingroup\$

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)
\$\endgroup\$
  • 2
    \$\begingroup\$ Please add some description indicating what this chapter 3 thing is recommending with examples of input and output, nobody's going to guess what that is \$\endgroup\$ – user203258 Sep 18 at 20:49
  • \$\begingroup\$ @Edmad Broctor and @ Carcigenicate Thanks for your feedback. I went ahead and revised my post. \$\endgroup\$ – cyberprogrammer Sep 18 at 21:07
  • 6
    \$\begingroup\$ "Amazingly enough, this sequence actually works for any integer—sooner or later, using this sequence, you’ll arrive at 1!" Actually we don't know that it works for any integer, the idea that it will always reach 1 is a conjecture. \$\endgroup\$ – Pierre Cathé Sep 19 at 8:05
  • 1
    \$\begingroup\$ @PierreCathé I have a truly marvelous demonstration of this proposition which this comment box is too small to contain. \$\endgroup\$ – Acccumulation Sep 19 at 17:11
  • \$\begingroup\$ @Acccumulation Wow ! That's impressive, in that case you should update the Wikipedia article, and contact the International Mathematical Union to collect your Fields Medal. \$\endgroup\$ – Pierre Cathé Sep 20 at 6:45
7
\$\begingroup\$

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)
\$\endgroup\$
11
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ Perhaps reverse the test and eliminate the comparison to 0: next = 3 * num + 1 if num % 2 else num // 2 \$\endgroup\$ – RootTwo Sep 18 at 23:04
  • 4
    \$\begingroup\$ @RootTwo Honestly, I find that that muddies the intent. We're not checking if num % 2 is falsey; it's not a predicate. We're checking if it's equal to 0. It just so happens that they're equivalent in Python. \$\endgroup\$ – Carcigenicate Sep 19 at 1:02
  • \$\begingroup\$ next = ... overshadows the built-in next. In this context it's probably not hazardous, but just good to be aware of \$\endgroup\$ – DeepSpace Sep 19 at 8:16
  • \$\begingroup\$ While technically you could use if num % 2 else, I agree that showing the intent here is more important. and if num % 2 == 0 else communicates that intent better. I do agree you should rename your next variable, though. It's a bad habit. \$\endgroup\$ – Gloweye Sep 19 at 9:01
  • \$\begingroup\$ @200_success Thanks for pointing out my failure to return the number not simply print it. That was very helpful. \$\endgroup\$ – cyberprogrammer Sep 20 at 18:25
1
\$\begingroup\$

For sake of completeness, a recursive implementation for collatz (you already got enough good suggestions for inputting num):

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

collatz(3)

Outputs

3
10
5
16
8
4
2
1
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think this answer helps OP. Although a recursive definition is certainly possible, it doesn't fit the specification they are trying to meet. Furthermore, this code has some redundancy: the function always returns 1 (when it returns at all), so why even have a return value? \$\endgroup\$ – Mees de Vries Sep 19 at 9:45
  • \$\begingroup\$ @MeesdeVries the function always returns 1 (when it returns at all), so why even have a return value I'm not sure I follow. The recursion must have a base case. It just so happens that the base case in this example is if num == 1. The example output shows that the function does not always returns 1. \$\endgroup\$ – DeepSpace Sep 19 at 9:58
  • 1
    \$\begingroup\$ The function prints other values, but it returns only the value 1. \$\endgroup\$ – Mees de Vries Sep 19 at 10:04
  • 1
    \$\begingroup\$ I am not sure what your point is. My original comment noted that there is no need for the function you wrote to return anything, because it will always be the number 1, regardless of input. Therefore a better implementation would avoid returning any value at all, to avoid the suggestion that it is meaningful. E.g., I would say a better implementation (of the recursion part) is if num % 2 == 0: collatz(num//2); elif num > 1: collatz(3 * num + 1). \$\endgroup\$ – Mees de Vries Sep 19 at 14:46
  • 1
    \$\begingroup\$ I invite you to try the code I wrote out for yourself to see that it does not (at least, for positive integer input). If you wish to discuss this any further I suggest we take it to chat. \$\endgroup\$ – Mees de Vries Sep 19 at 15:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.