12
\$\begingroup\$

I am a beginner in Python and am currently learning about making beautiful code. I've started with a small program. Can you please review my code?

def inputAndRes():
    try:
        a = raw_input("Enter no. :- ")
        n = int(a)
    except Exception:
        print "-----------------------------------"
        print "Please Enter No. " , "( The Number you entered is not a number )"
        inputAndRes()
    else:
        if(n>0):
            print "Number is Positive"
        if(n<0):
            print "Number is Negative"
        if(n==0):
            print "Number is ZERO"
        wantRestart()

def checkYN(checkResult):
    if (checkResult == 'Y'):
            inputAndRes()
    if (checkResult == 'N' ):
            print (" !!! Program Quit !!!")
    else:
        print ("Invalid Option Make sure that you type (Y / N)")
        print ("Continue..")
        wantRestart()

def wantRestart():
    print "\nWant to Restart Program, if Yes then Enter 'Y' and if No enter 'N' :- ",
    try:
        checkResult = raw_input(" ")
    except Exception as e:
        print ("Sorry Error Occured: ",e)
    else:
        checkYN(checkResult)        
#...Start of Program...
inputAndRes()
\$\endgroup\$
16
\$\begingroup\$

Flow control

The conditions here are mutually exclusive:

if(n>0):
    print "Number is Positive"
if(n<0):
    print "Number is Negative"
if(n==0):
    print "Number is ZERO"

It would be better to rewrite with elif and else:

if n > 0:
    print "Number is Positive"
elif n < 0:
    print "Number is Negative"
else:
    print "Number is ZERO"

Same thing here:

if (checkResult == 'Y'):
        inputAndRes()
if (checkResult == 'N' ):
        print (" !!! Program Quit !!!")
else:
    print ("Invalid Option Make sure that you type (Y / N)")
    print ("Continue..")
    wantRestart()

Should be:

if checkResult == 'Y':
    inputAndRes()
elif checkResult == 'N' :
    print (" !!! Program Quit !!!")
else:
    print ("Invalid Option Make sure that you type (Y / N)")
    print ("Continue..")
    wantRestart()

Finally, instead of raw_input(" "), it would be simpler to write as raw_input().

Writing style

There is an official coding style guide for Python called PEP8. Give it a good read and try to follow it. Your most notable coding style violations:

  • Spacing around operators: prefer n > 0 instead of n>0
    • Also, avoid unnecessary parenthesis in if statements, for example if n > 0: is better than if (n > 0):
  • Function naming should be snake_case instead of camelCase
  • Put 2 empty lines in front of a function definition

Misc

Instead of running inputAndRestart unconditionally, the common practice is this:

if __name__ == '__main__':
    inputAndRestart()

The advantage of this is that you will be able to import your module without running anything, because the if will only be true when you run the script directly.

Usability

The program wants me to enter Y to restart and N to exit, but I'm kinda lazy to press the shift. It would be great if I could use just y and n. Or even q and Q to exit. An easy way to do that:

if result in 'yY':
    input_and_restart()
elif result in 'nNqQ':
    print (" !!! Program Quit !!!")
\$\endgroup\$
7
\$\begingroup\$

Starting from taking into account janos' really interesting comments about flow control and style, you have :

def input_and_res():
    try:
        a = raw_input("Enter no. :- ")
        n = int(a)
    except Exception:
        print "-----------------------------------"
        print "Please Enter No. ", "( The Number you entered is not a number )"
        input_and_res()
    else:
        if n > 0:
            print "Number is Positive"
        elif n < 0:
            print "Number is Negative"
        else:
            print "Number is ZERO"
        want_restart()


def check_y_n(check_result):
    if check_result == 'Y':
        input_and_res()
    elif check_result == 'N':
        print (" !!! Program Quit !!!")
    else:
        print ("Invalid Option Make sure that you type (Y / N)")
        print ("Continue..")
        want_restart()


def want_restart():
    print "\nWant to Restart Program, if Yes then Enter 'Y' and if No enter 'N' :- ",
    try:
        check_result = raw_input()
    except Exception as e:
        print "Sorry Error Occured: ", e
    else:
        check_y_n(check_result)


def main():
    """Main function"""
    input_and_res()

if __name__ == "__main__":
    main()

No, there is still a major issue : recursion is slow in python and it will fail eventually when you reach the maximum stack size which is not that big if I remember correctly. In your case, while loop can reduce this effect (if not remove it altogether). On top of performance issues, it can make things hard to read/understand to have function calls leading to indirect recursion.

First attempt is to remove the obvious recursive call from input_and_res:

def input_and_res():
    while True:
        try:
            a = raw_input("Enter no. :- ")
            n = int(a)
            if n > 0:
                print "Number is Positive"
            elif n < 0:
                print "Number is Negative"
            else:
                print "Number is ZERO"
            return want_restart()
        except Exception:
            print "-----------------------------------"
            print "Please Enter No. ", "( The Number you entered is not a number )"

Next step is to rewrite want_restart so that it returns a boolean : with minor adaptations, you get :

def input_and_res():
    while True:
        try:
            a = raw_input("Enter no. :- ")
            n = int(a)
            if n > 0:
                print "Number is Positive"
            elif n < 0:
                print "Number is Negative"
            else:
                print "Number is ZERO"
            if not want_restart():
                print (" !!! Program Quit !!!")
                break
        except Exception:
            print "-----------------------------------"
            print "Please Enter No. ", "( The Number you entered is not a number )"


def want_restart():
    print "\nWant to Restart Program, if Yes then Enter 'Y' and if No enter 'N' :- ",
    while True:
        try:
            check_result = raw_input()
            if check_result == 'Y':
                return True
            elif check_result == 'N':
                return False
            else:
                print ("Invalid Option Make sure that you type (Y / N)")
                print ("Continue..")
        except Exception as e:
            print "Sorry Error Occured: ", e


def main():
    """Main function"""
    input_and_res()

if __name__ == "__main__":
    main()

Not that it is much easier to understand which function is responsible for what.

Once this is done, a few things can still be improved : your function names can be clearer and you can had documentation.

Also, you should try to be more precise when catching exceptions.

\$\endgroup\$
  • \$\begingroup\$ Alternatively stated, the "functions" are being used as if they were GOTO labels, and that is wrong. \$\endgroup\$ – 200_success Sep 7 '14 at 17:08
4
\$\begingroup\$

As per janos flow control and PEP8 are important. I agree with what's been said; however there's more to the flow of the code than has been stated so far. Also a few additional points to bear in mind

Exception handling

except Exception:

This is potentially bad practice; it depends on what your code is going to be used for. I would never catch all exceptions within the main part of your code itself. If your code is generating exceptions which you're not expecting then you really need to know about it. Equally, if a user wants to stop the code from running through a KeyboardInterrurpt you've just forbidden this.

Catch only the exceptions that you're expecting to get and that you have a strategy for dealing with. If it's important that your code doesn't error then wrap in a highly generic exception handler only where necessary.

Printing

From Python 3 print is a function; you're obviously using Python 2.x by your use of the print statement:

print "Number is Positive"

As your version of Python supports print() use it and get into the good habits now.

Consistency

  1. Sometimes you print something before using raw_input() without an argument; sometimes you use raw_input() with an argument.
  2. Both the statement print and the function print() are used.

It's important to be consistent as it makes your intent clearer and the code cleaner. All useful when you re-look at code in a years time or a colleague does so after you've left. You want

Comments

Though it's fairly easy to understand what's happening get into the habit of writing comments.

Use docstrings at the top of every method to explain what that method does and how you use it. Use comments to explain why your code is doing what it does and in the case of complex code what as well. To give an example consider the following two situations

while true:
    # increment i by 1
    i += 1

vs

while true:
    # Want to count the number of iterations
    # for use in method check() later
    i += 1

The first comment explains what's happening, which is obvious from the code. The second explains why it's happening, which means that people might find the code easier to understand.

Flow control

If you were to create a decision tree of your code then it would be a little confused; inputAndRes() calls itself and wantRestart(). wantRestart() calls checkYN() and checkYN() calls wantRestart() and inputAndRes().

Careful usage of the break statement which exits a loop and the continue statement which "continues to the next cycle of the nearest enclosing loop" will make your code simpler and remove the need for recursion.

Other stuff

Your if statement to determine if the user wants to continue is quite verbose. You can use Python's ability to return a Boolean based on a test to see whether they want to continue using your program. From a usability perspective I'd only test for a positive. If they want to exit then let someone type anything.

return checkResult in 'yY'

This will return True if the user input Y or y and False otherwise.

Very minor, but there's no need for

a = raw_input("Enter no. :- ")
n = int(a)

You're creating two objects and the statements are simple. You can attempt the conversion to an int immediately.

a = int(raw_input())

The end

Putting all of this together and using the other comments about __main__ and Pep8 etc. I think I'd end up with this:

def want_restart():    
    """Determine if the user wants to continue playing."""

    check = raw_input("Press 'Y' to restart: ")

    # Only care if people want to continue.
    return check in 'Yy'

def main(): 
    """Accepts a number from the user and decides whether
    It's positive, negative or 0."""

    while True:

        try:
            n = int(raw_input("Enter number: "))

            if n > 0:
                print("Number is Positive")
            elif n < 0:
                print("Number is Negative")
            else:
                print("Number is Zero")

            if not want_restart():
                print (" !!! Program Quit !!!")
                break

        except ValueError:
            # If the number is not a number we can force
            # a restart of the process.
            print ("-----------------------------------")
            print ("The Number you entered is not a number; please retry")
            continue

if __name__ == "__main__":
    main()
\$\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.