5
\$\begingroup\$

This script is for a student who has never programmed before. The goal is that she can quickly start to understand what is going on and start to tinker with it.

As I am not very familiar with Python I wonder whether I am breaking any important style paradigm and whether there are things that can make the code further readable, by making it shorter or simpler.

# Define function
def npn(n):
    """ Find the next perfect number after integer n
    """
    org = n = abs(n)
    while True:
        n += 1
        # Keep track of sum of the positive divisors
        spd = 1
        # Now check (other) possible divisors.
        # Note that subset of divisors to check can be greatly reduced...
        # which will increase efficiency
        for i in range(2, n):
            # Check divisibility with the modulo operator
            if (n % i == 0):
                spd += i
        if n == spd:
            print("The next perfect number after", org, "is", n)
            break

# Get user input
print("\n", "Check what is the next perfect number after n.")
while True:
    try:
        x = int(input("Enter your n: "))
    except ValueError:
        print("n should be a postive integer!")
        continue
    if x < 8128:
        break
    print("Oh. Finding perfect numbers is hard. Choose a lower number. Plz.")

# Run function with user input
npn(x)
\$\endgroup\$
6
\$\begingroup\$

The greatest hindrance to understanding is the naming:

  • What is npn? Some type of transistor? next_perfect_number would be a much better name for the function!
  • What is spd? An abbreviation for "speed"? sum_factors would be an improvement… but see below.
  • You prompt for "n", but assign the result to a variable named x. That's confusing.

The next biggest problem is that the npn() function tries to do too much. It would be much clearer if the while loop were moved into a separate is_perfect_number(n) function.

A function that performs a complex calculation should not also perform I/O, as such mixing will prevent code reuse (for unit testing or for a future GUI, for example). That is, npn() should return its result rather than printing it.

Idiomatic Python would use more elegant ways to perform the loops. The n += 1 loop could be done using itertools.count(). The loop to calculate spd would be much more succinctly expressed using sum() with a generator expression. It would certainly be easier to read the intent behind the code that way, but you would also have to introduce a Python-specific language feature. It's hard to say which approach a beginner would find easier to understand.

It would be good practice to reduce the amount of free-floating code. I suggest packaging the prompting and printing code in a main() function.

from itertools import count

def next_perfect_number(n):
    """Find the next perfect number after integer n"""
    for i in count(n + 1):
        if is_perfect_number(i):
            return i

def is_perfect_number(n):
    """Test whether n is a perfect number"""
    # Simple but inefficient algorithm
    return n == sum(i for i in range(1, n) if n % i == 0)

def main():
    print("Find the next perfect number after n.")
    while True:
        try:
            n = int(input("Enter your n: "))
            if 1 <= n < 8128:
                break
            print("Choose a number between 1 and 8128, please.")
        except ValueError:
            print("n should be a positive integer!")
    print(
        "The next perfect number after {0} is {1}.".format(
            n,
            next_perfect_number(n)
        )
    )

if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ Maybe, to stay idiomatic, in case n > 8128, raise NotImplementedError? \$\endgroup\$ – enedil Jun 15 '17 at 1:40
  • 1
    \$\begingroup\$ @enedil Why? So that the program will crash instead of gracefully handling the situation? \$\endgroup\$ – 200_success Jun 15 '17 at 1:42
2
\$\begingroup\$

When I was a beginner, I'd have found this hard to understand. There are two reasons I'd find this hard:

  1. I, and presumably she, understand English, however x, n, npm, spd are not English words. You should write them out.

    Actually, you should do this even if the readers of your code aren't beginners, as you can get an understanding of what the variable is if it has a good name. Say for example sum_positive_divisors.

  2. You expect a beginner to grasp 15 different keywords / functions off the bat. With pretty much no comments on what they do. I'd genuinely be surprised if they can grasp what it's doing 100%. Do you expect to sit down with them and explain what everything does? If not you need to break them in. Only teach them a couple of things over the space of a little while. Don't expect them to know all the things as soon as they start.

And so keeping new things to learn to a minimum, you can get:

while True:
    user_number = int(input("Enter your n: "))

    if user_number < 0:
        user_number = -user_number

    if user_number < 8128:
        original_number = user_number
        while True:
            user_number = user_number + 1
            sum_positive_divisors = 1
            divisor = 2
            while divisor < user_number:
                if user_number % divisor == 0:
                    sum_positive_divisors = sum_positive_divisors + 1

            if user_number == sum_positive_divisors:
                print("The next perfect number after", original_number, "is", user_number)
                break

I didn't put any comments, as I don't really know what you'd expect of the person you're helping out.

\$\endgroup\$
2
\$\begingroup\$

This is not specific to Python: Avoid magic numbers. There are no constants in Python. Upper case names are used for variables used as constants. The magic number 8128 should be replaced by a variable, e.g.

HIGHEST_NUMBER_ALLOWED=8128
...
    if x < HIGHEST_NUMBER_ALLOWED:
        break
    print("Oh. Finding perfect numbers is hard. Choose a number lower than {0}. Plz."
       .format(HIGHEST_NUMBER_ALLOWED))    

Also I would shift the whole user interaction out of the function that does the calculation.

    if n == spd:
        return(n)
....
print("The next perfect number after", org, "is", npn(n))

Therefore I would change the function description to

def npn(n):
   """ Returns the next perfect number after positive integer n"""

There is no need to epand the docstring over two lines , but if it pleases you more, it is ok, too.

If you expect a positive integer as input you should test if the input is a positive integer. You are tricky and allow negative integers and in your function you turn them to positive ones. But now you have the problem that the user may have input a number smaller than -8128 and now your function may have to handle numbers larger than 8128. So I think you should check the user input carefully and reject negative numbers. In the function you can assume that the input is as you define. You can validate your assumption by using asserts e.g.

def npn(n):
...
    assert(type(n)=='int')
    assert(n>=0)

Maybe you should revert your test logic, the correct input is processed at the end of the input loop. That makes it easier to add checks.

while True:
    try:
        x = int(input("Enter your n: "))
    except ValueError:
        print("n should be a postive integer!")
        continue
    if x > 8128:
        print("Oh. Finding perfect numbers is hard. Choose a lower number. Plz.")
        continue
    if x <1:
        print("Choose a positive number. Plz.")
        continue
    # input is fine
    break

This is more consistent and it is more clear how the loop works:

  • wrong input: continue
  • right input: break

And of course better variable and function names: longer, descriptive names.

I checked your program with http://pep8online.com and it does not complain anything. (PEP 8 is the Style Guide for Python Code)

\$\endgroup\$

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.