Python calculator script

Question

Are there any ways to make this calculator script for Python better or simpler?

thelist = ["Add", "add", "Multiply", "multiply", "Divide", "divide","Subtract", "subtract"] def Multiply(x,y): z = x * y print(z) def Divide(x,y): x = float(x) y = float(y) z = x / y print(z) def Add(x,y): z = x + y print(z) def Subtract(x,y): z = x - y print(z) while True: operation = input("What would you like to do? Multiply/Divide/Add/Subtract ") if operation in thelist: break else: print("That was not an option..") if operation == "Multiply" or operation == "multiply": while True: try: x = int(input("First number: ")) break except ValueError: print("Make sure to enter a number.") while True: try: y = int(input("Second number: ")) break except ValueError: print("Make sure to enter a number...") Multiply(x,y) elif operation == "subtract" or operation == "Subtract": while True: try: x = int(input("First number: ")) break except ValueError: print("Make sure to enter a number.") while True: try: y = int(input("Second number: ")) break except ValueError: print("Make sure to enter a number.") Subtract(x,y) elif operation == "Add" or operation == "add": while True: try: x = int(input("First number: ")) break except ValueError: print("Make sure to enter a number..") while True: try: y = int(input("Second number: ")) break except ValueError: print("Make sure to enter a number.") Add(x,y) elif operation == "Divide" or operation == "divide": while True: try: x = int(input("First number: ")) break except ValueError: print("Make sure to enter a number.") while True: try: y = int(input("Second number: ")) break except ValueError: print("Make sure to enter a number.") Divide(x,y) else: pass 

Answer 1

A thorough review would very much depend on what you're trying to achieve.

For example, it seams mighty clumsy to have to type multiply instead of just *. Also, a case-insensitive match would be much better. For that, you might want to check method lower or how to use regular expressions in Python.

Judging from your use of print, you're aiming at Python 3. If this is the case, then - unlike in Python 2 - you don't need

x = float(x) y = float(y) 

in function Divide. Of course, if you want your code to work in both Python 2 & 3, it's O.K. to keep that code. However, even in that case, converting only one of these two variables to float is enough.

Like most (all?) interpreted and pseudocompiled languages, Python has eval function. So, you can basically do result = eval(expression), where expression is a string with any Python code. For example, if you have a string "1+2*3", and send it to eval, you'll get 7 (unless my math is off :-)). This might help you significantly, but be careful to not pass on just any (potentially junky) user's input.

One thing seems strange to me. You do

if operation == "Multiply" or operation == "multiply": 12 lines to input x and y Multiply(x,y) elif operation == "subtract" or operation == "Subtract": THE SAME 12 lines to input x and y Subtract(x,y) ... 

Why don't you first input x and y, and then do if-elif block for the actual computation, or, at least, make a function to input x and y and return them (functions in Python can return multiple values)? You have functions for simple expressions which are called only once (multiplication, subtraction, etc), which seem completely unnecessary, but you don't make one for 12 lines of code which are invoked 4 times in your code.

Regarding your naming of functions, function names should be lowercase, with words separated by underscores as necessary to improve readability, i.e., you shouldn't capitalize the names of your functions.

Answer 2

I like how you put divide, multiply, add and subtract each into their own function, but it would be even better if the function returned the result instead of printing it. That way, you can do something else with the result instead of printing it if you want to, and still use the same function.

def multiply(x, y): return x * y 

Then, further down, you need to write print(multiply(x, y)).

The while loops you use to get input look good.

One thing that bothers me is that you have the same 12 lines of code 4 times. You should reuse the same lines as much as possible, so that if you want to change something, you only have to change it in one place. This code should appear only once:

while True: try: x = int(input("First number: ")) break except ValueError: print("Make sure to enter a number.") while True: try: y = int(input("Second number: ")) break except ValueError: print("Make sure to enter a number...") 

In this case, it's easy to fix. Put it right before the line:

if operation == "Multiply" or operation == "multiply": 

Now it will be run no matter what the operation is and you can delete the repetitions.

But that piece of code above still repeats twice what is essentially the same thing. We should also do something about that. I would use a function (I'd put the function next to the other functions at the top.):

def get_int_input(prompt): while True: try: return int(input(prompt)) except ValueError: print("Make sure to enter a number...") x = get_int_input("First number: ") y = get_int_input("Second number: ") 

This will do the same thing as the old code.

Next let's do something about the fact that we're writing every operation's name twice:

if operation == "Multiply" or operation == "multiply": 

I would change that by making the content of operation lowercase as soon as possible:

thelist = ["Add", "add", "Multiply", "multiply", "Divide", "divide","Subtract", "subtract"] 

Now you don't need to care about case anymore. Note that this isn't exactly the same, though: it will also allow input such as MULtiplY.

Now we can change the first line to:

operations = ["add", "multiply", "divide", "subtract"] 

(I think operations is a better name than thelist.) And we can change our two checks for each operation to:

if operation == "multiply": print(multiply(x, y)) elif operation == "subtract": ... 

You can remove else: pass.

This post is already quite long, but one more thing: you should change your code to use 4 spaces per indent. This is because tabs can be displayed in different ways, for example, they break the indentation on here stackexchange if you're not careful. Most editors have a setting so that when you hit the tab key, 4 spaces appear. To change the code you've already written, use the editor's replace option to replace each tab character with 4 spaces.

Answer 3

There are several concerns with your code.

• You cast your numeric inputs as int. Subsequent casting to float won't recover the lost precision.
• It's very repetitive. There is a lot of copy-and-pasted code, which makes the program hard to maintain.
• Your top-level "function" is very long. In particular, nesting if-while-try blocks makes the flow of control hard to follow. Ideally, anything longer than a dozen lines should be broken up into simpler functions.
• It takes a lot of work to define a new operation. For example, to support Addition, you have to
  • Add two entries to thelist
  • Define the Add() function
  • Change the prompt for the operation
  • Add a conditional to check if the operation name matches "Add" or "add", and call Add() if it does

A key concept for consolidating all that functionality is to define a BinaryOperation class. Then, instead of writing out the instructions explicitly, let the data drive the logic.

from collections import OrderedDict class BinaryOperation: def __init__(self, op): self.op = op def go(self): x = self._prompt("First number: ") y = self._prompt("Second number: ") print(self.op(x, y)) def _prompt(self, prompt): while True: try: return float(input(prompt)) except ValueError: print("Make sure to enter a number...") def get_operation(operations): while True: op_name = input("What would you like to do? " + '/'.join(operations.keys()) + ' ').title() try: return operations[op_name] except KeyError: print("That was not an option.") operations = OrderedDict([ ('Multiply', BinaryOperation(lambda x, y: x * y)), ('Divide', BinaryOperation(lambda x, y: x / y)), ('Add', BinaryOperation(lambda x, y: x + y)), ('Subtract', BinaryOperation(lambda x, y: x - y)), ]) get_operation(operations).go() 

Edit: The OP would like an explanation of the code, so I'll give it a shot.

lambda is a way to define a very simple unnamed function. That function can then be assigned and passed around, just like any other value in Python. For example, instead of saying

def square(number): return number * number 

you could say

square = lambda number: number * number 

In Python, lambdas are limited to just one expression, to prevent programmers from trying to squeeze too much complex code inside. But that's just perfect for our simple calculator. We pass the lambda as a parameter to the BinaryOperation constructor, where it gets stored as the BinaryOperation object's op variable. Later, the go() method can call self.op(x, y) to perform the calculation.

If you're uncomfortable with passing functions around, you could do something with inheritance instead. It's more verbose, though, to have to define so many subclasses.

class BinaryOperation: def go(self): x = self._prompt("First number: ") y = self._prompt("Second number: ") print(self.op(x, y)) def _prompt(self, prompt): while True: try: return float(input(prompt)) except ValueError: print("Make sure to enter a number...") class Multiplication(BinaryOperation): @staticmethod def op(x, y): return x * y # etc. operations = OrderedDict([ ('Multiply', Multiplication()), ('Divide', Division()), ('Add', Addition()), ('Subtract', Subtraction()), ]) get_operation(operations).go() 

Edit 2: I've just learned from @Stuart about the operator module, which already defines the lambdas we want. Therefore, we could equivalently write

import operator operations = OrderedDict([ ('Multiply', BinaryOperation(operator.mul)), ('Divide', BinaryOperation(operator.truediv)), ('Add', BinaryOperation(operator.add)), ('Subtract', BinaryOperation(operator.sub)), ])