10
\$\begingroup\$

Hi I am new to python and I have written a simple calculator, I would like to ask if the code is written well or if anyone has any tips on how to write it better.


def addidtion():
    if choose == 1:
        print("Your result:", firstNum + secondNum)
def subtraction():
    if choose == 2:
        print("Your result:", firstNum - secondNum)
def multiplication():
    if choose == 3:
        print("Your result:", firstNum * secondNum)
def division():
    if choose == 4:
        print("Your result:", firstNum / secondNum)

print("Welcome to the calculator,")

firstNum = float(input("Give me first number: "))
secondNum = float(input("Give me second number: "))

choose = int(input("Choose one option:\n"
      "1.Addition\n"
      "2.Subtraction\n"
      "3.Multiplication\n"
      "4.Division\n"))

addidtion()
subtraction()
multiplication()
division()
\$\endgroup\$
1
  • \$\begingroup\$ I see a typo in line 1. "addidtion" \$\endgroup\$
    – Roland
    Dec 7 '21 at 11:39
18
\$\begingroup\$
  1. If I want to use your functions; it'd be very hard. I'd have to modify the global scope of the program and have to know the quirks around choose.

    Always pass variables to functions as arguments.

    def addition(choose, firstNum, secondNum):
        if choose == 1:
            print("Your result:", firstNum + secondNum)
    
  2. Don't include choose in your addition and such functions. It simply doesn't belong there and means all calls to addition have to be addition(1, lhs, rhs).

  3. You've written the same print four times, you should really return the result of the operation and then write the print once.

def addition(lhs, rhs):
    return lhs + rhs


def subtraction(lhs, rhs):
    return lhs - rhs


def multiplication(lhs, rhs):
    return lhs * rhs


def division(lhs, rhs):
    return lhs / rhs


print("Welcome to the calculator,")
lhs = float(input("Give me first number: "))
rhs = float(input("Give me second number: "))
choose = int(input(
    "Choose one option:\n"
    "1.Addition\n"
    "2.Subtraction\n"
    "3.Multiplication\n"
    "4.Division\n"
))
if choose == 1:
    result = addition(lhs, rhs)
if choose == 2:
    result = subtraction(lhs, rhs)
if choose == 3:
    result = multiplication(lhs, rhs)
if choose == 4:
    result = division(lhs, rhs)

print("Your result:", result)

We can further simplify the code by storing the options in a list or a dictionary. A list is simpler to learn, but requires the options start at 0.

We can store a list ([``]) of options in options, seperating each option by a ,. We can then get an item from the list (1st being 0, 2nd being 1, ...) using [0].

>>> options = [addition, subtraction, ...]
>>> options[0](1, 2)
3
def addition(lhs, rhs):
    return lhs + rhs


def subtraction(lhs, rhs):
    return lhs - rhs


def multiplication(lhs, rhs):
    return lhs * rhs


def division(lhs, rhs):
    return lhs / rhs


options = [
    addition,
    subtraction,
    multiplication,
    division,
]

print("Welcome to the calculator,")
lhs = float(input("Give me first number: "))
rhs = float(input("Give me second number: "))
choose = int(input(
    "Choose one option:\n"
    "0.Addition\n"
    "1.Subtraction\n"
    "2.Multiplication\n"
    "3.Division\n"
))
print("Your result:", options[choose](lhs, rhs))

Advanced changes

  • We can change the functions to lambdas. These are function's without names.

    options = [
        lambda lhs, rhs: lhs + rhs,
        lambda lhs, rhs: lhs - rhs,
        lambda lhs, rhs: lhs * rhs,
        lambda lhs, rhs: lhs / rhs,
    ]
    
  • By providing a name with each lambda we can build the options table. We can by:

    1. Change options to a tuple with a name and a function.

      options = [
          ("addition", lambda lhs, rhs: lhs + rhs),
          ...
      ]
      
    2. Use enumerate to go through options getting the index at the same time as the name (and function).

      for index, (name, _) in enumerate(options):
          pass
      
    3. Use an f-string to format the index and name into a string.

      option = f"{index}.{name}\n"
      
    4. Join all the available options together with str.join.

options = [
    ("addition", lambda lhs, rhs: lhs + rhs),
    ("subtraction", lambda lhs, rhs: lhs - rhs),
    ("multiplication", lambda lhs, rhs: lhs * rhs),
    ("division", lambda lhs, rhs: lhs / rhs),
]

print("Welcome to the calculator,")
lhs = float(input("Give me first number: "))
rhs = float(input("Give me second number: "))
choose = int(input(
    "Choose one option:\n"
    + "".join(
        f"{index}.{name}\n"
        for index, (name, _) in enumerate(options)
    )
))
print("Your result:", options[choose][1](lhs, rhs))
\$\endgroup\$
5
  • \$\begingroup\$ Good answer. Reminds me of another recent question: Basic calculator using break statements to control the logic with some similarities in the approach and differences too (using the operator library). \$\endgroup\$
    – Kate
    Dec 5 '21 at 19:34
  • \$\begingroup\$ @Anonymous Thanks. Yeah once you've seen one "calculator" answer, you've really seen them all :) There's different flavors each person will have... but they're all basically the same. \$\endgroup\$
    – Peilonrayz
    Dec 5 '21 at 20:31
  • \$\begingroup\$ If you're doing [ ("addition", lambda lhs, rhs: lhs + rhs), ... ] <-- this list, with a name and a lambda, why not make it into a dictionary? \$\endgroup\$ Dec 6 '21 at 9:15
  • 1
    \$\begingroup\$ @IsmaelMiguel OP's initial code selected the operation by index. You can argue that it'd be more intuitive for the user to select an operation by typing a string, but IMO that's too much of a change from OP's initial intent. If that list were a dict, it would be much harder to look up the element of the dictionary that corresponds to a particular index, and wouldn't even be guaranteed to work correctly on versions of Python before 3.7 \$\endgroup\$
    – ymbirtt
    Dec 6 '21 at 9:18
  • \$\begingroup\$ @ymbirtt That's a good point. I'm not familiar with anything below Python 3.6, and I'm not that good at Python 3.7, so, I take your word on it. \$\endgroup\$ Dec 6 '21 at 9:24
9
\$\begingroup\$

Don't rely on global variables. It's good that you are experimenting with functions; however, you aren't really taking advantage of them, because they operate on global variables rather than taking arguments. Global variables are almost never needed.

Code repetition is a warning sign. The functions are repetitive: they all contain the same conditional logic and printing behavior. One of the key purposes of functions is to reduce code duplication.

Validate inputs. The program will raise an exception if the user makes a mistake and enters an invalid number.

Simplify the user interface. The program's user-interface is tedious. Each number has to be typed and entered separately, followed by selecting the mathematical operation by typing a number from a list. That feels awkward considering that Python makes it so easy for a user to perform all of the data entry in one shot. Also, programs should have a straightforward way to quit.

Unlimited calculations. Most calculators don't limit you to performing a single calculation. There's no good reason for that limitation in Python.

Better to learn the language than reinvent simple things. Programming languages are very good a math. In fact, they often have built-in functions for addition, subtraction, and so forth. So there's no good reason to write such functions at all. Much better is to learn about the built-in powers of the language.

Use data structures to eliminate logic. A lot of your program's logic is oriented around handling the user's choice and taking the correct action in response. Quite often, tedious code like that can be eliminated through the use of appropriate data structures. In this case, we will use a dict mapping the user-entered arithmetic operators to the corresponding mathematical functions.

# Get arithmetic functions for free, from the Python standard library.

import operator

operations = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.truediv,
}

print('Welcome to the calculator.')

# Support multiple calculations.
while True:
    # Support an easy way to quit.
    inp = input('=> ').strip()
    if not inp:
        break
    # Handle invalid input. Enhance as needed.
    try:
        a, op, b = inp.split()
        # No conditional logic, thanks to our data structure.
        result = operations[op](float(a), float(b))
        print(result)
    except Exception:
        print('Invalid input.')
\$\endgroup\$
1
\$\begingroup\$

It seems nice. A good code. Simple calculator.

I think it would be better if you write the 'ifs' outside functions. Then only the choose one will be called.

\$\endgroup\$
0
\$\begingroup\$

Consider this:

from typing import Union
def calculator(choice: int, first_num: int, second_num: int) -> Union[int, float]:
    result = 0
    if choice == 1:
        result = first_num + second_num
    elif choice == 2:
        result = first_num - second_num
    elif choice == 3:
        result = first_num * second_num
    elif choice == 4:
        if second_num != 0:
            result = first_num / second_num
    return result

print("Welcome to the calculator.")

first_num = float(input("Give me first number: "))
second_num = float(input("Give me second number: "))

choice = int(input("Choose one option:\n"
      "1.Addition\n"
      "2.Subtraction\n"
      "3.Multiplication\n"
      "4.Division\n"))

result = calculator(choice, first_num, second_num)
print(f'Your result is: {result}.')

One function to do it all.

Obviously could add some while loop or a try statement to make sure input is always right (especially choice) or could add else: raise error, in the function, but that is just an extra (for such a simple thing at this stage).

Do yourself a favour, try to type-hint from the beginning. Helps a lot, you will say thank you in future ;) learn on others mistakes not yours ;)

\$\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.