CODEWITHSUNDEEP
python-2.7readability
Nolub hay
Nolub hay

Reputation: 3

Python readability

Greetings stackoverflow, I am pretty new to both this website and python. I wrote a simple program to create a list of devisors based on a user inputted number, the program works (as far as I know)

But I am pretty sure my syntax is extremely amateurish, so I ask the fluent among you, what can I do to make this:

divisors = []
def devisors(x):
    while True:
        try:
            for i in range(1,x+1):
                if x%i==0:
                    divisors.append(i)
            if x==i:
                break
        except ValueError, TypeError:
            return "Invalid input."
            continue
    print "The list of divisors of",x,"Are: ", divisors
choice1= raw_input("Would you like to use devisors program?(Y/N): ")
while True:
    try:        
        if choice1 in yes:
            x = int(raw_input("Please enter a number: "))
            devisors(x)
        elif choice1 in no:
            print "Alright, bye bye."
            break
        else:
            print "invalid input"
            choice1= raw_input("Would you like to use devisors program(Y/N): ")
            continue
    except ValueError, TypeError:
        print "Invalid input, try again."
    try:
        choice2= raw_input("Would you like to try again?(Y/N): ")
        if choice2 in yes:
            divisors = []
            continue
            devisors(x)
        elif choice2 in no:
            print "Alright, bye bye. "
            break
        else:
            print "invalid input"
            choice2= raw_input("Would you like to try again?(Y/N): ")
            continue
    except ValueError, TypeError:
        print "Invalid input, try again."

Better written? I want you to go berserk on this code, tell me (If you feel like it) what could be done to improve it, to write less lines, and mistakes I made.

Also I am not sure if this is something allowed on stackoverflow, if not, please let me know and I'll delete this.

Thank you. Looking forward to the feedback.

Upvotes: 0

Views: 165

Answers (1)

Julian A
Julian A

Reputation: 344

First of all, we do have a dedicated Stack Exchange Code Review site, but you're not the only one asking for a code review here.

The first thing I noticed is that you are trying to hard to put everything in a single method.

Whenever you are faced with a problem or task you should try to divide it into the smallest possible problems. Afterwards we can implement the solution to these small problems as functions.

In your example your tasks are the following:

  1. Get user input of a number
  2. Get the divisors of that number
  3. Print the divisors
  4. Ask if the user wants to give it another try

Let's start with the 2nd point of our list:

def divisors_of(x):
    if x < 0:
        raise ValueError("Value cannot be smaller than 0")

    divisors = []
    for i in range(1, x + 1):
        if x % i == 0:
            divisors.append(i)

    return divisors

That doesn't look too bad, does it?

After implementing the user input for a number and printing the result, we already get something like this:

try:
    num = int(raw_input("Please enter a number: "))
    divisors = divisors_of(num)
    print "The divisors of ", num, " are: ", divisors

except (ValueError, TypeError) as ex:
    print ex.message

Looking at our task list we still don't ask the user for another try. Again, IMHO it's more readable if we put the logic for that in a function. This has two additional benefits, we can convert the yes/no choice to a boolean and the written code is reuseable:

def yes_no_choice(text):
    while True:
        choice = raw_input(text)
        if choice in "yes":
            return True
            # Returning like this is called early return
            # As we jump out of the function, the code after
            # the if is automatically the else statement
            # The code is shorter and more readable
        if choice in "no":
            return False
        print "Invalid answer"

Our final result looks like this:

while True:
    try:
        num = int(raw_input("Please enter a number: "))
        divisors = divisors_of(num)
        print "The divisors of ", num, " are: ", divisors

    except (ValueError, TypeError) as ex:
        print ex.message

    if not yes_no_choice("Would you like to try again?(Y/N): "):
        print "Bye!"
        break
    # else we will loop

Oh, and try not to use global variables if you don't have to. I'm looking at your divisors = []

Upvotes: 1

Related Questions