3
\$\begingroup\$

I made a calculator for a class I am taking. It works like this:

  • Tells user the input range is between two numbers, and asks if want to change the range. If yes, asks the user input new numbers (if they input anything that isn't a number, we catch it with exception handling). If no, proceeds and ask the user for two numbers, and performs addition, subtraction, multiplication, and division of the numbers. After the calculations it ask the user if they want to continue using the calculator. Depending on the answer, the calculator restarts or the application stops.

TL;DR: Everything works correctly but I am asking for suggestions to improve my code. I believe its excessively long.

#---------------------------------------
My Calculator
#---------------------------------------


def AddNum(FN, SN): 
    print("\nThe Addition result is\t", FN, "+", SN, "\t=", (FN + SN))

def SubNum(FN, SN):
    print("\nThe Subtraction result is\t", FN, "-", SN, "\t=", (FN - SN))

def MulNum(FN, SN):  
    print("\nThe Multiplication result is\t", FN, "*", SN, "\t=", (FN * SN))

def DivNum(FN, SN):  
    if FN == 0 or SN == 0:  
        print("\nThe Division result is\t\t\t", FN, "/", SN, "\t=",
              "You cannot divide by Zero") 
    elif FN != 0 or SN != 0:  
        print("\nThe Division result is\t", FN, "/", SN, "\t=",
              (FN / SN))  

def IsInRange(LR, HR, FN, SN):

    if LR <= FN <= HR and LR <= SN <= HR:
        return
    else:  
        print("\n[The values are outside the input ranges.] \n\nPlease check the numbers and try again")
        myLoop()  

print("""------------------------------------------------------
\nWelcome to my Calculator.
\nGive me two numbers and I will calculate them for you.
------------------------------------------------------""")

def myLoop(): 

    Loop4Ever = "Y"
    ChngRng = ""
    FN, SN = 0, 0 
    while 1: 
        if Loop4Ever == "Y" or "y":

            LR = -100
            HR = 100

            print("\n{--- My current input range is from", LR, "to", HR, "for each of the two numbers. ---}")

            while 1: 
                try: 
                    CRlist = ["Y", "y", "N", "n"] 
                    ChngRng = input("\nWould you like to change the input range numbers (Y or N)?")
                    if ChngRng in CRlist:
                        break
                    else:
                        print("\nIncorrect input, only use Y or N.")
                except ValueError:
                    continue

            if ChngRng == "Y" or ChngRng == "y":
                while 1:
                    try:
                        LR = float(input("\nSet new Lower Range?\t"))
                        break
                    except ValueError:
                        print("Incorrect input, only enter numbers.")
                        continue
                while 1:
                    try:
                        HR = float(input("\nSet new Higher Range?\t"))
                        break
                    except ValueError:
                        print("Incorrect input, only enter numbers.")
                        continue
            elif ChngRng == "N" or ChngRng == "n":
                pass

            print("\nHigher Range--->", HR)
            print("\nLower Range--->", LR)

            while 1: 
                try: 
                    FN = int(input("\nFirst number?\t"))
                    break
                except ValueError: 
                    print("\nIncorrect input, only enter numbers.")
                    continue

            while 1: 
                try: #Try block to catch and handle incorrect user input.
                    SN = int(input("\nSecond number?\t"))
                    break
                except ValueError:
                    print("\nIncorrect input, only enter numbers.")
                    continue

            IsInRange(LR, HR, FN, SN)

            AddNum(FN, SN)
            SubNum(FN, SN) 
            MulNum(FN, SN) 
            DivNum(FN, SN) 

            Loop4Ever = "0" 
            LpList = ["Y", "y", "N", "n"] 

            while 1: 
                try: 
                    Loop4Ever = str(input("\nContinue using the Simple Calculator (Y or N)? "))
                    if Loop4Ever not in LpList:
                        print("\nIncorrect input, only use Y or N.") 
                        continue 
                except ValueError: 
                    print("\nIncorrect input, only use Y or N.") 
                    continue 
                else: #If user input not in our list.
                    if Loop4Ever == "Y" or Loop4Ever == "y": 
                        while 1: 
                            try: 
                                myLoop() 
                                break 
                            except ValueError: 
                                continue
                    elif Loop4Ever == "N" or Loop4Ever == "n":
                        print("\nThanks for using our calculator.")
                        exit()

myLoop() #Initiate Calculator.
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

There are a few things that strike me in your code, even before trying to reduce it in size.

First, the if-condition in divNum is weird: why would you prevent the user from dividing 0 by something? The answer is 0 but that's totally acceptable. I would replace if FN == 0 or SN == 0: by if SN == 0:. Then, instead of your elif clause, you could simply use an else one: everything that is not caught by your if is legal.

Then, even though it is perfectly legal, I don't like while 1:. If you want to loop for ever, why don't you simple use while True: ? It does the same, but in a (slightly) clearer way.

Your variables have understandable but improvable names: ChngRng could be changeRange and it would definitely not hurt, for example.

Finally, your code may go into infinite recursion. I would frown on it in Java but it's even worse in Python because it is not particularly efficient when dealing with it. Indeed, myLoop is made to loop infinitely but even so, you call it from isInRange and from itself. What happens when you change the range, then input numbers that are outside the range? I haven't run your code but I'm pretty sure unorthodox stuff happens (an inner Calculator is created with the standard range and when you exit it, you go back to the outer Calculator that computes the numbers you did not want to compute because they were outside the range).

Now to the structure of your code. Using functions is good. I may have done it slightly differently, replacing your four functions by only one:

def compute(first, second, opName, operator, opFunction):
    print("\nThe", opName, "result is\t", first, operator, second, "\t=", opFunction(first, second))

This means you must do the check for division outside of this function before calling it so I won't say that my way is better, simply different. Then, you replace your four calls with :

compute(FN, SN, "Addition", "+", lambda x,y:x+y)
compute(FN, SN, "Subtraction", "-", lambda x,y:x-y)
compute(FN, SN, "Multiplication", "*", lambda x,y:x*y)
if SN != 0:
    compute(FN, SN, "Division", "/", lambda x,y:x/y)
else:
    print("Could not perform division: second number is 0")

I would also rewrite isInRange and the way you call it. I would replace all of it by a getInRangeNumber defined as follows:

def getInRangeNumber(LR, HR, question):
    while True:
        try:
            value = float(input("\n" + question + "\t"))
        except ValueError:
            print("\nIncorrect input, only enter numbers.")
        else:
            if LR <= value <= HR:
                return value
            else:
                print("\n[The values are outside the input ranges.] \n\nPlease check the numbers and try again")

To get the first number, I would replace all your corresponding while 1: loop by getInRangeNumber(LR, HR, "First number?"). Idem for the second number. You could do something similar for HR and LR with a getRangeNumber(question) function and similarly replace the loop to define them with two calls to that function, one for each bound.

I would replace Loop4ever and ChngRng by boolean values and also define validBooleanAnswers = {"Y","N"} at the same place. Your top-level while 1: becomes while Loop4ever: and your loop to redefine Loop4ever becomes:

while True:
    try: 
        Loop4Ever = str(input("\nContinue using the Simple Calculator (Y or N)? ")).upper()
    except ValueError:
        print("\nIncorrect input, only use Y or N.")
    else:
        if Loop4Ever not in validBooleanAnswers:
            print("\nIncorrect input, only use Y or N.")
        else:
            Loop4Ever = Loop4Ever == "Y"#convert Loop4Ever to its boolean value
            break

Even better, put that into a function that returns a boolean and use that function to get the value for Loop4Ever and ChngRng. Then, your elif ChngRng == "N" or ChngRng == "n": becomes useless: either it is True and you do stuff, or it is not and you don't need to check its exact value (because you know it is False) and you don't do anything.

As your goal is to learn, I will not provide you with a complete working code taking all my points into account to let you try them around. If anything is unclear or does not work the way I say, do not hesitate to ask for clarification. To be completely honest, I have actually run neither your code nor my suggestions so there might be glitches to fix.

\$\endgroup\$
1
\$\begingroup\$

Your names use non-standard casing. The most common Python naming convention is snake_case for variables, functions, and methods; BumpyCase for user-defined classes; and UPPER_CASE for constants. In particular, its a bad idea to use uppercase other than for constants or in specific domains where capitalized variables are an important convention.

Continuation logic too complex. The logic for whether to continue using the calculator is much more complex than needed: if the user enters yes, you enter a lower-level while-true loop that recursively calls myLoop(). None of that is needed, because you're already residing within the outer while-true loop. That means the following will work: (1) if user replies yes, no nothing; (2) otherwise, just return from the myLoop() function, and the program will simply end. On a related matter, the outer while-true loop does not need to begin by checking whether Loop4Ever is yes. Indeed, that variable is not needed at all: just get the user's reply and either continue or return; no need for remembering status, just act.

Use substantive names, not names that superficially describe the code. Substantive names tell the reader something useful about the variable, function, class, whatever. Your program is a calculator, so call the function calculator() or calculate() or run_calculator(). By contrast, myLoop() is mostly empty: the "my" tells the reader nothing and the "loop" tells the reader only that a loop is involved.

Prefer substantive names over cryptic names. For example, lower and upper rather than LR and HR. Help your reader out when it's practical and easy to do so. In some contexts, much shorter variables can work great: but make sure the context fits and also make sure the gains from brevity outweigh the costs that would come from bulkier names.

If code is repetitive, maybe you need a function. A lot of the code is devoted to getting valid user input. In some cases, you need a yes/no response. For example:

def get_yesno(prompt):
    while True: 
        reply = input(prompt).lower()
        if reply in ('yes', 'y'):
            return True
        elif reply in ('no', 'n'):
            return False
        else:
            print('\nInvalid input (choices: yes, no, y, n).')

If code is repetitive, maybe you need a function. Another big chunk of code is devoted to getting user input in the form of valid floats. Same as before, write a function:

def get_float(prompt)
    while True:
        try:
            return float(input(prompt + ' '))
        except ValueError:
            print('Invalid input, only enter numbers.')

Be nice to users by reporting errors early. The calculator is annoying to use in the sense that it collects all of the replies and then check whether the numbers are within the accepted range. It would be more user-friendly to report problems immediately. One way to do that is to enhance get_float() a bit so that it takes optional boundaries:

def get_float(prompt, lower = float('-inf'), upper = float('inf')):
    while True:
        try:
            n = float(input(prompt + ' '))
            if lower <= n <= upper:
                return n
            else:
                print(f'Invalid input, must be in range {lower} through {upper}.')
        except ValueError:
            print('Invalid input, only enter numbers.')

If code is repetitive, maybe you need a data structure. The final chunk of repetition in the program comes from the functions that perform arithmetic and print the results. What do those four functions have in common: a label, an arithmetic symbol, a mathematical operation to perform, and two numbers. So we need a data structure that links the labels, symbols, and operations.

OPERATIONS = [
    ('Addition', '+', add),
    ('Subtraction', '-', sub),
    ('Multiplication', '*', mul),
    ('Division', '/', truediv),
]

To use that data structure, we need the operations to be functions that take 2 numbers and return the needed mathematical calculation. You could easily implement those functions:

def add(a, b):
    return a + b

# Etc.

Or you could import them from the standard library and then use them in a loop:

from operator import add, sub, mul, truediv

def run_calculator(): 
    lower = -100.0
    upper = 100.0
    while True: 
        if get_yesno(f'Change the input range ({lower} through {upper})?'):
            lower = get_float('Set new lower bound.')
            upper = get_float('Set new higher bound.')
        first = get_float('First number?', lower, upper)
        second = get_float('Second number?', lower, upper)
        for label, sym, op in OPERATIONS:
            try:
                result = op(first, second)
                print(f'The {label} result is {first} {sym} {second} = {result}')
            except ZeroDivisionError:
                print('You cannot divide by zero')
        if not get_yesno('Continue using the Simple Calculator?'):
            return

# The right way to initiate the program.
if __name__ == '__main__':
    run_calculator()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.