5
\$\begingroup\$

Here I am making a calculator for a robotics assignment. I need the program to be able to take two inputs and convert to float. It also needs to display previous calculations when exiting and also stop when trying to divide by 0. It works I just want to make sure its clean and im not using any bad practices.

operators = ["+", "-", "*", "/"]
answers = []
def run_calculator():
    def instructions():
        try: chosen_operator = int(input("Choose an Operator:\n1) Addition\n2) Subtraction\n3) Multiplication\n4) Division\n\n5) Exit\nInput (1/2/3/4/5):"))
        except:
            print('Invalid Input: Choose A Valid Operation')
            return
        if chosen_operator > 4 or chosen_operator < 1:
            if chosen_operator == 5: return
            print('Invalid Input: Choose a Valid Operation')
            return
        operator = operators[chosen_operator - 1]
        try: firstnum = float(input('First Number:'))
        except: 
            print('Invalid Input: Not a valid number')
            return
        try: secondnum = float(input('Second Number:'))
        except: 
            print('Invalid Input: Not a valid number')
            return
        return operator, firstnum, secondnum
    while (True):
        try: operator, firstnum, secondnum = instructions()
        except: break
        if operator == "/" and secondnum == 0.0:
            print("Cannot divide by 0")
            break
        calc_function = float.__add__ if operator == "+" else float.__sub__ if operator == "-" else float.__mul__ if operator == "*" else float.__truediv__
        answers.append(f"{firstnum} {operator} {secondnum} = {calc_function(firstnum, secondnum)}")
        restart = input((f"{firstnum} {operator} {secondnum} = {calc_function(firstnum, secondnum)}\nWould you like to do another calculation? (Y/N)"))
        if restart.lower() == 'y':
            continue
        else:
            print("Results:")
            for x in range(len(answers)):
                print(f"{answers[x]}")
            break
run_calculator()
\$\endgroup\$
2
\$\begingroup\$

Immutable sequences

operators should be a tuple (i.e. ('+', ...)), since you don't change it.

Nested functions

instructions should be moved to the global level - you shouldn't need a closure, and should be passing information between instructions and run_calculator with parameters and return values, not closure variables.

Index-based selection

It's a little awkward to ask that the user select an operator based on numerical index. Make operators a dictionary instead, and have the user input the operator character. Of course, this is only possible if the parameters of your homework assignment permit.

Never except:

Seriously. except ValueError: instead. Otherwise, you'll catch exceptions you don't want to, including user-break Ctrl+C.

Parens

Python doesn't need parens here:

while (True):

so delete them.

Logic inversion

    if restart.lower() == 'y':
        continue
    else:
        print("Results:")
        for x in range(len(answers)):
            print(f"{answers[x]}")
        break

is more easily expressed as

    if restart.lower() != 'y':
        print("Results:")
        for x in range(len(answers)):
            print(f"{answers[x]}")
        break
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ From the last suggestion.What is the use of using index range for the loop when you can do in answers? Also, what is the point of using f-strings when you have only one value? Lastly, It might be better to do [print x for x in answers] for the last suggestion. \$\endgroup\$ – Aryan Parekh Oct 13 at 17:37
  • \$\begingroup\$ @AryanParekh Those comments belong in your own answer, since they're about the original code and not my suggestions. \$\endgroup\$ – Reinderien Oct 13 at 17:43
  • \$\begingroup\$ @AryanParekh Also I discourage discarded comprehension-loops like [print x for x in answers]. You're throwing the list away, so why make it at all? Just use a regular loop. \$\endgroup\$ – Reinderien Oct 13 at 17:44
  • \$\begingroup\$ Those were in your improved version, I was suggesting you correct a bad implementation in your version, I was trying to improve your answer more, I could've ignored it/downvoted but now I will just let it be. \$\endgroup\$ – Aryan Parekh Oct 13 at 17:48
  • \$\begingroup\$ Yeah I dont like using a numerical value to choose the operator but its what my assignment required. \$\endgroup\$ – Jack W Oct 14 at 1:36
2
\$\begingroup\$

Positives

  • Your code adopts better practices than before.
  • You have used meaningfully names for variables and functions

Negatives

  • Code looks a little clunky as one whole chunk of code that does everything
  • Can improve readability by using eval.
  • Program terminates on invalid input, rather than asking vaild input

Let's see how we can do Negatives = []


Split work into multiple functions

In your program all operations are done by a single function. That is

  1. Input
  2. Processing
  3. Output
  4. Starting over again

Instead what if you split the work into functions with meaningful names, your loop() function could look like

def calculator_loop():
    while True:
        clear_screen()
        show_instructions()
        args = calculator_input()
        result = process_input(args)
        print_result(result)
        if input("Do you want to calculate again(y/n) ?\n").lower() ==  'n':
            break
        

Show instructions

We can move printing the instructions into a separate function, making it easy to show the instruction whenever we might want to.

def show_instructions():
    print( # I/O instructions for calculator ) 

Taking input

With our current loop() function in mind, out calculator_function() keeping the same logic should look like, note that this comes after showing the instructions (show_instructions)

def calculator_input():
    operators = ['+','-','*','/']
    try:
        chosen_operator = int(input("Enter operator: "))
    except:
        print('Invalid Input: Choose A Valid Operation')
        return
    if chosen_operator > 4 or chosen_operator < 1:
        if chosen_operator == 5: return
        print('Invalid Input: Choose a Valid Operation')
        return
    operator = operators[chosen_operator - 1]
    try:
        firstnum = float(input('First Number:'))
    except:
        print('Invalid Input: Not a valid number')
        return
    try: 
        secondnum = float(input('Second Number:'))
    except:
        print('Invalid Input: Not a valid number')
        return
    return operator, firstnum, secondnum
  • Notice these error messages
    'Invalid Input: Choose a Valid Operation' 'Invalid Input: Not a valid number'
    Instead of copy-pasting these messages everywhere, a better idea would be to use operator_err_message and number_err_message and print them.

  • As I mentioned before, the program shouldn't terminate if the user entered invalidly, instead it should ask the user for valid input. Does command prompt in Windows close if you made a mistake? Or does it show you Unknown x command?

  • You can handle list index error using IndexError. Ultimately it is shorter and clear compared to if chosen_operator > 4 or....

With these improvements

def calculator_input():
    operators = ['+','-','*','/']
    operator_err_message = 'Invalid Input: Choose A Valid Operation'
    number_err_message = 'Invalid Input: Not a valid number'

    try:
        chosen_operator = int(input("Enter operator: "))
    except Exception:
        print(operator_err_message)
        calculator_input()

    if operator == 5:
        return 

    try:
        operator = operators[chosen_operator - 1]
    except IndexError:
        print(operator_err_message)
        calculator_input()
    try:
        firstnum = float(input('First Number:'))
    except Exception:
        print(number_err_message)
        calculator_input()
    try:
        secondnum = float(input('Second Number:'))
    except Exception:
        print(number_err_message)
        calculator_input()
        
    return operator, firstnum, secondnum

Edit: As suggested by @Reinderien, except: fill catch if Cntrl-C is pressed, hence it is better to use except Exception:

Processing input

Again, if you split the work into a new function without any changes, it would look like

def process_input(num1,num2,operator):
    calc_function = float.__add__ if operator == "+" else float.__sub__ if operator == "-" else float.__mul__ if operator == "*" else float.__truediv__
    return calc_function(num1,num2)

With eval

def process_input(num1,num2,operator):
    return eval(f"{num1}{operator}{num2}")

Printing result

Your current method is just fine, and I believe that it doesn't need improvements, you can decide to move it into a function or not.


Final

def show_instructions():
    instructions = 
"""
Operators:
    1) Addition
    2) Subtraction
    3) Multiplication
    4) Division
    5) Exit
"""
    print(instructions)

def calculator_input():
    operators = ['+','-','*','/']
    operator_err_message = 'Invalid Input: Choose A Valid Operation'
    number_err_message = 'Invalid Input: Not a valid number'

    try:
        chosen_operator = int(input("Enter operator: "))
    except Exception:
        print(operator_err_message)
        calculator_input()
    if operator == 5:
        return
    try:
        operator = operators[chosen_operator - 1]
    except IndexError:
        print(operator_err_message)
        calculator_input()
    try:
        firstnum = float(input('First Number:'))
    except Exception:
        print(number_err_message)
        calculator_input()
    try:
        secondnum = float(input('Second Number:'))
    except Exception:
        print(number_err_message)
        calculator_input()

    return operator, firstnum, secondnum

def process_input(num1,num2,operator):
    return eval(f"{num1}{operator}{num2}")

def clear_screen():
    print(chr(27) + "[2J")

def calc_loop():
    while True:
        clear_screen()
        show_instructions()
        operator,num1,num2 = calculator_input()
        result = process_input(num1,num2,operator)
        print(f"{num1} {operator} {num2} = {result}\n")
        if input("Would you like to play again?(y/n): ").lower() == 'n':
            break


calc_loop()

The results are stored in answers

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Having repeated newline-escape continuations in a literal is a poor idea for a handful of reasons, not the least being: that literal is going to inherit all of the indentation from its enclosing code block. You're best to use a triple-quote literal, or perhaps one string per line joined by implicit concatenation, instead. \$\endgroup\$ – Reinderien Oct 13 at 17:54
  • \$\begingroup\$ except operator == 5: makes no sense and I'm frankly surprised that it's accepted as valid syntax. It certainly doesn't do what you likely want, which is to enter the except block when the condition is true. That's what if is for. \$\endgroup\$ – Reinderien Oct 13 at 17:56
  • 1
    \$\begingroup\$ @Reinderien Triple quoted literal sounds good, thank you for suggesting! I will add it \$\endgroup\$ – Aryan Parekh Oct 13 at 17:56
  • 1
    \$\begingroup\$ Also: don't except:. I encourage you to try cancelling the program with Ctrl+C during a try-protected input, to see how this affects you. The solution is to except Exception:. \$\endgroup\$ – Reinderien Oct 13 at 18:00
  • 2
    \$\begingroup\$ @Reinderien I have fixed all errors and added Exception, thank you for the correction \$\endgroup\$ – Aryan Parekh Oct 13 at 18:05

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.