0
\$\begingroup\$

I wrote the following code for the Collatz exercise: The Collatz Sequence

Any feedback is appreciated.

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.

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

n = int(input("Enter a number: "))
collatz (n)
\$\endgroup\$
  • 3
    \$\begingroup\$ @drapozo It would be welcome if you'd provide either a link or a small description of the problem. \$\endgroup\$ – dfhwze Jun 6 at 6:39
  • 1
    \$\begingroup\$ It´s a problem form the Book automate the boring stuff with python. The problem is "The Collatz Sequence" and can be found in this link. automatetheboringstuff.com/chapter3 \$\endgroup\$ – drapozo Jun 7 at 14:27
  • 2
    \$\begingroup\$ When adding additional information you should edit your question instead of adding a comment. I have added that information to your post. Learn more about comments including when to comment and when not to in the Help Center page about Comments. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 20 at 15:35
3
\$\begingroup\$

A few simple observations:

  •     if n % 2 == 0:
            ...
        elif n % 2 == 1:
            ...
    

    Here, we know that if n % 2 isn't 0, it must be 1, since n is an integer. So that elif can be simply else, making it simpler to read. More experienced programmers will reverse the test, knowing that 1 is the only truthy result of n % 2, so write if n % 2: ... ; else ....

  •     if ...:
            ...
            print (n)
        else:
            ...
            print (n)
    

    The print that's present in both branches could be placed after the if/else, since it doesn't depend on the condition.

  • The results of the arithmetic expressions are already int, so no conversion is necessary.

  • The if/else could be reduced to a single line:

    n = 3 * n + 1 if n % 2 else n // 2
    

    but to me, that looks less clear, so I don't recommend that level of terseness here.


Modified code

Applying the observations above, I get a simpler version of the function:

def collatz(n):
    while n > 1:
        if n % 2:
            n = 3 * n + 1
        else:
            n = n // 2
        print(n)
\$\endgroup\$
  • \$\begingroup\$ Thanks for your observations. In the simpler version you wrote i think you meant if n % 2 == 1. \$\endgroup\$ – drapozo Jun 7 at 14:24
  • \$\begingroup\$ No, I didn't mean if n % 2 == 1. Because n % 2 can only be 0 or 1, it's simpler to just test whether n % 2 is truthy. \$\endgroup\$ – Toby Speight Jun 10 at 8:34

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.