4
\$\begingroup\$

I tried to write this code as concisely as possible. Is this the best way to do it?

def collatz(n):
    """
    Generator for collatz sequence beginning with n

    >>> list(collatz(10))
    [5, 16, 8, 4, 2, 1]
    """
    while n != 1:
        n = n / 2 if n % 2 == 0 else 3*n + 1
        yield int(n)
\$\endgroup\$
4
\$\begingroup\$

The only improvement I see here is to divide n by 2 using // (since we are dealing with Python 3.x) and to remove the explicit conversion to int (int(n)):

while n != 1:
    n = n // 2 if n % 2 == 0 else 3*n + 1
    yield n

Also, I suggest you put a single space before and after the multiplication operator in 3*n, so that it becomes 3 * n.

Hope that helps.

\$\endgroup\$
4
\$\begingroup\$

I expect collatz(1) to produce 1. Rather, it generates an empty sequence. So, in my opinion, the function should immediately yield n before the loop.

\$\endgroup\$
  • \$\begingroup\$ You're right, that was an oversight \$\endgroup\$ – Vermillion Oct 21 '16 at 3:19
3
\$\begingroup\$

I would use while n > 1, just in case someone tries to call this function with n=0 or even a negative n.

\$\endgroup\$
  • 2
    \$\begingroup\$ Along those lines OP should probably mention in the docstring that n is a positive integer. \$\endgroup\$ – Dair Oct 20 '16 at 23:36

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.