7
\$\begingroup\$

In chapter 3 of Automate the Boring Stuff with Python by Al Sweigart, one exercise asks to write a program which performs the Collatz sequence.

I wrote a basic version first, then added and restructured some of it to account for an invalid input. Any thoughts or criticisms to make it more concise, organized or legible would be appreciated.

def collatz(number): # This evaluates and prints the sequence of numbers.
        if number % 2 == 0: # If it's an even number.
            print(number // 2)
            return number // 2
        else: # If it's an odd number.
            print(3 * number + 1)
            return 3 * number + 1
        
def main(): # The main loop inside a function so the ValueError message can appear as many times as needed.
    try:
        user_input = int(input())
        while user_input > 1:
            user_input = collatz(user_input)
        if user_input <= 0: # If a negative integer is entered.
            print('Please enter a positive integer.')
        else:
            print('Enter another integer.')

    except ValueError: # If something other than an integer is entered.
        print('Please enter a valid integer.')

print('Type any positive integer.') # Main prompt.
        
while True: # This calls the main loop so you can repeat the equation as many times as needed.
    main()
\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$

Most of your code deals with obtaining and validating user input. That's tedious code to write and maintain. In addition, programs premised on user-interactivity are difficult to execute during development, testing, and debugging: every execution requires a manual dialogue between the user (you, the programmer) and the computer. That get annoying very fast. Moreover, your program provides no clean way to quit. A more powerful approach based on command-line arguments and the built-in argparse library is shown below.

The second issue is that your collatz() function intermixes algorithmic computation and reporting (i.e. printing). That's almost always a bad idea. A more flexible approach is to restrict collatz() purely to the algorithm and do the printing at a higher level in the program -- either in main(), if the reporting remains extremely simple, or in a function called by it.

The essence of most of these suggestions is to organize your code more carefully to keep different tasks more separated: user input collection; input validation; computation; and reporting.

import argparse
import sys

def main(args):
    # Get user arguments.
    opts = parse_args(args)

    # Make computations.
    sequences = [list(collatz(val)) for val in opts.initial_values]

    # Report to user. Enhance as needed.
    for seq in sequences:
        print(seq)

def parse_args(args):
    # You want the user to provide one or more integers
    # on the command line. The argparse library will handle that
    # automatically for many use cases (e.g, any integer). In your situation,
    # you do need to write a type validation function, but that process
    # is a bit simpler and more focused than your previous validation
    # code, which was mixed with the rest of the program's logic.
    ap = argparse.ArgumentParser()
    ap.add_argument('initial_values', nargs = '+', type = positive_int)
    return ap.parse_args(args)

def positive_int(val):
    # You can make this more robust if needed (eg to prevent values like
    # '12.34' from being accepted), but here's the basic technique.
    try:
        n = int(val)
        1 / n
        return n
    except Exception:
        raise argparse.ArgumentTypeError(f'Invalid positive integer: {val}')

def collatz(n):
    # Relieved of printing, this function becomes simpler, more testable,
    # and more general purpose.
    while n > 1:
        yield n
        if n % 2 == 0:
            n = n // 2
        else:
            n = 3 * n + 1
    yield n

# Invoke main() if the program was executed rather than imported.
# This is important for testability, because testing tools need
# to import your code without executing the script in its entirety.
if __name__ == '__main__':
    main(sys.argv[1:])
\$\endgroup\$
6
  • \$\begingroup\$ "code review" does not mean to write a program but give the user input on his program. How should your program help a user that has written his first python program? Besides that your program contains a lot of things that are really bad, e.g. checking user input with the assert statement.. \$\endgroup\$
    – miracle173
    Feb 16, 2021 at 7:50
  • \$\begingroup\$ @miracle173 I think you misjudge the OP's ability to learn quite a lot from this answer. And your comment implying that using assert as a tool for validation is "bad" has no basis at all. \$\endgroup\$
    – FMc
    Feb 16, 2021 at 8:27
  • \$\begingroup\$ Regarding assert, there's a discussion on Stack Overflow where the prevailing recommendation is that asserts should only be used to test conditions that should never happen and thus are not appropriate for validating input coming from external sources. From the same discussion, there's also the fact that assert statements are removed when compilation is optimized, which could be a big gotcha if you were relying on asserts to maintain certain invariants or control program flow. \$\endgroup\$
    – Setris
    Feb 16, 2021 at 22:28
  • \$\begingroup\$ @Setris I do have a vague recollection of reading that particular StackOverflow question back in the golden age. Most of the discussion strikes me as heavy on dogma (sort of like the narrow view of "code review" espoused a few comments up). But you do raise a valid point about optimized compilation. Point taken and code edited – albeit in a slightly sarcastic way. \$\endgroup\$
    – FMc
    Feb 16, 2021 at 23:44
  • 1
    \$\begingroup\$ @astralcheeks You are very welcome. The specific details in my code example are not too important and are easily learned when needed. But keeping different aspects of your program cleanly separated is the key and will serve you very well, both as a beginner and beyond. \$\endgroup\$
    – FMc
    Feb 17, 2021 at 2:49
1
\$\begingroup\$

Congratulation to your first Python program. As far as I can see it is correct. For comments I obey the following rules:

  1. comments should be correct and precise and useful
  2. use as few comments as possible

The reason for the first rule is that if somebody who reads the code he make wrong assumptions on the code when the comments are wrong. That makes it harder to analyze the code.

The second rule is a consequence of the first rule. The more comments you have the more incorrect comments you may have. If you modify your code you must modify your comments, too, and check them again. This is unnecessary work and increases the chances of incorrect comments. There is not automatic way to check the comments.

Lets analyze the program from the highest level using the comments

def main(): # The main loop inside a function so the ValueError message can appear as many times as needed.
    ...

print('Type any positive integer.') # Main prompt.
        
while True: # This calls the main loop so you can repeat the equation as many times as needed.
    main()

For me none of these comments obey my rules

  • def main(): # The main loop inside a function so the ValueError message can appear as many times as needed.: the function main is not a loop, but it is called in the body of a loop. I don't know what you want to say by "the ValueError message can appear as many times as needed." I think if you use a comment like "the user input is requested, checked and if correct a positive integer, the collatz sequence of this number is printed"
  • print('Type any positive integer.') # Main prompt. I don't know what the meaning of "Main prompt" is, but I see that this statements prints a message to the youuser that he should 'Type any positive integer.' So I think the comment isn't needed here.
  • while True: # This calls the main loop so you can repeat the equation as many times as needed. I see that this loop calls the function main. If this is what you want to say, it is not necessary to write this down in a comment because this is absolutely clear. This comment is not useful.

If we go one level deeper we see further comments we see

def collatz(number): # This evaluates and prints the sequence of numbers.

Is incorrect. The function does not evaluate a sequence of number. It calculates one number and print this one number. So the comment should be something like

# calculates and prints the next Collatz number

I prefer short lines so I would put the comment below the def statement. In this special case one should comment in the following way

def collatz(number):
'''calculates and prints the next collatz number'''

according to PEP 257 -- Docstring Conventions This kind of docstring associates the docstring to Collatz procedure object and some tools can use this fact. e.g. tools that create documentation of code. In this "PEP 257" you may find a lot of other tips how to document the function, like documenting the arguments, the return value and the side effects like printing to the terminal.

if user_input <= 0: # If a negative integer is entered.

This comment is wrong. The correct formulation is # If a negative integer or 0 is entered.. But even the correct is nor really helpful. If the variable name is chosen appropriate the it is clear that user__input holds the user_input. And every programmer knows what "<=0" means. So instead writing an incorrect and unneeded comment remove it.

But now let's stop with the comments. You can analyze the remaining comments by yourself. From my point of view all of them have problems.

In your first function

def collatz(number): 
        if number % 2 == 0: # If it's an even number.
            print(number // 2)
            return number // 2
        else: # If it's an odd number.
            print(3 * number + 1)
            return 3 * number + 1

I see that some expression are reused, number // 2 and 3 * number + 1

Whenever an expression is used more than once I assign it to a variable and use this variable instead. If the expression changes there is only one place where I have to modify it. So I would write

next_number = number // 2
print(next_number)
return next_number

But a second look at this code shows that number is never used after calculating next_number because you call return. So you can use number instead of next_number and get

number = number // 2
print(number)
return number

and the if-block changes to

if number % 2 == 0: # If it's an even number.
    number = number // 2
    print(number)
    return number
else: # If it's an odd number.
    number = 3 * number + 1
    print(number )
    return number 

Whenever if have similar code in both branches of an if statement I try to pull this before or after the if statement to redduce the amount of code that must be read and checked. So I prefer

def collatz(number): 
    if number % 2 == 0: # If it's an even number.
        number = number // 2
    else: # If it's an odd number.
        number = 3 * number + 1
    print(number )
    return number 

Finally it is always a good idea to separate IO and calculations if this is possible. It is easier to reuse the function that does the calculation, it is easier to test it and it is easier to change the IO behaviour. In this case it is easy to do this. In your code you use the collatz funtion in the follwing way:

while user_input > 1:
    user_input = collatz(user_input)

Remove the print statement from the collatz function

def collatz(number): 
    if number % 2 == 0: # If it's an even number.
        number = number // 2
    else: # If it's an odd number.
        number = 3 * number + 1
    return number 

and change the code to

while user_input > 1:
    user_input = collatz(user_input)
    print(user_input)

Now you have separated IO and the calculation. If you want to write sequence to a file instead of the terminal, you can change it and do not have to change the collatz function. If you want to calculate the number of iterations that are needed until 1 is reached for a given start value you can use your collatz function without changing it

cnt=0
while user_input > 1:
    user_input = collatz(user_input)
    cnt+=1
print(cnt)

The your initial version of the collatz function would print all numbers taht are calculated to the terminal.

Naming conventions are proposed in PEP 8 -- Style Guide for Python Code and I will not discuss them here. But concerning your naming I think user_input is not a goud name for your variable because most of the time and on some places of the program the variable does not contain user input. So I would prefer

collatz_number=user_input
while collatz_number> 1:
    collatz_number= collatz(collatz_number)
if collatz_number == 1:
    return

Now let's analyze main.

try:
    user_input = int(input())
    while user_input > 1:
        user_input = collatz(user_input)
    if user_input <= 0: # If a negative integer is entered.
        print('Please enter a positive integer.')
    else:
        print('Enter another integer.')

except ValueError: # If something other than an integer is entered.
    print('Please enter a valid integer.')

If int(input()) raise a conversion error because the user entered an invalid string then an error is raised and the user gets the message 'Please enter a valid integer.'. That is fine. But the user gets these message even if an error is raised in your collatz function. Of course there will not be such an error in your case as far as I can see. But if the function is more complex this could happen. And if it happens the user will be irritated by a wrong error message on the one hand and on the other hand you will not get an information about where the actual error occurs. So you should restrict the try...except clause to the statements that you really want to guard, so put this clause around the statement containing the input.

try:
    user_input = int(input())
except ValueError: # If something other than an integer is entered.
    print('Please enter a valid integer.')
    return
while user_input > 1:
    ....

In this block you do two things:

  • you check if the user input is correct, that is you check if it is a positive integer number
  • you calculate the Collatz sequence, if the input is an integer number greater or equal 2.

But you mix up these tasks:

  • you check if the input is an integer
  • if it is an integer greater or equal 2 you print the Colatz sequence of this integer
  • then you again check the user input and issue an error if it is negative

You should not do this. Split these two tasks in two block of consecutive statements. So more clear is the following.

# check user input
try:
    user_input = int(input())
except ValueError: # If something other than an integer is entered.
    print('Please enter a valid integer.')
    return
if user_input <= 0: # If a negative integer is entered.
    print('Please enter a positive integer.')
    return

# print Collatz sequence
while user_input > 1:
    user_input = collatz(user_input)
print('Enter another integer.')
return

First check the user input, if it is valid then do the calculations.

You have three massages to the user that should tell him that he should enter a positive integer:

  1. 'Type any positive integer.'
  2. 'Please enter a valid integer.'
  3. 'Please enter a positive integer.'
  4. 'Enter another integer.'

They are different an maybe this is intended. But I think this is not necessary, I would replace it yb tthe same text. And therefor I would use a variable for the text. I use upsercase names for the constan according to one of these PEP document:

INPUT_PROMPT='Please enter a positive integer.'

and replace the strings by this variable. If one does this, one can remove the "Main Prompt" if one reorders the statement in the procedure main.

# check user input
print(INPUT_PROMPT)
try:
    user_input = int(input())
except ValueError: # If something other than an integer is entered.
    print(INPUT_PROMPT)
    return
if user_input <= 0: # If a negative integer is entered.
    print(INPUT_PROMPT)
    return

# print Collatz sequence
while user_input > 1:
    user_input = collatz(user_input)
# here user_input=1, either because the user entered 1 or the while loop was exited because user_input became 1
return

Both blocks can be put in appropriate functions, and you get the following loop

while True:
    user_input=get_user_input()
    print_collatz_sequence(user_input)

this make the program more clear and you can test the input-logic and the collatz-sequence calculation independently. Using appropriate function names eliminates the need for comments.

\$\endgroup\$
1
  • \$\begingroup\$ This is great and very comprehensive. Thank you for your time and effort; I hadn't even considered how bloated my comments were. I'll revisit my code, make the adjustments and keep these conventions in mind for the future as well. \$\endgroup\$ Feb 17, 2021 at 15:47

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.