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

For example, it seams mighty clumsy to have to typemultiply instead of just*. Also, a case-insensitive match would be much better. For that, you might want to checkmethodlower orhow to use regular expressions in Python.

Judging from your use ofprint, 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 functionDivide. 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 haseval function. So, you can basically doresult = eval(expression), whereexpression is a string with any Python code. For example, if you have a string"1+2*3", and send it toeval, you'll get7 (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 inputx andy, and then doif-elif block for the actual computation, or, at least, make a function to inputx andy 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.

• \$\begingroup\$z isn't global.\$\endgroup\$

  flornquake

  – flornquake

  2013-09-15 00:40:13 +00:00

  CommentedSep 15, 2013 at 0:40
• \$\begingroup\$You're right. Sorry for that, I'll remove it. I had a different agenda in my head (computing in functions and printing outside of them, just like you suggested in your answer). Thank you.\$\endgroup\$

  Vedran Šego

  – Vedran Šego

  2013-09-15 00:41:35 +00:00

  CommentedSep 15, 2013 at 0:41

I like how you putdivide,multiply,add andsubtract 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 writeprint(multiply(x, y)).

Thewhile 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 ofoperation 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 asMULtiplY.

Now we can change the first line to:

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

(I thinkoperations is a better name thanthelist.) And we can change our two checks for each operation to:

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

You can removeelse: 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 thetab key, 4 spaces appear. To change the code you've already written, use the editor's replace option to replace eachtab character with 4 spaces.

There are several concerns with your code.

• You cast your numeric inputs asint. Subsequent casting tofloat 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, nestingif-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 tothelist
  • Define theAdd() function
  • Change the prompt for the operation
  • Add a conditional to check if the operation name matches"Add" or"add", and callAdd() if it does

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

from collections import OrderedDictclass 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 theBinaryOperation constructor, where it gets stored as theBinaryOperation object'sop variable. Later, thego() method can callself.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 theoperator module, which already defines the lambdas we want. Therefore, we could equivalently write

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

• \$\begingroup\$I wish I was that good at writing code, but I'm still learning basic things. I have no clue what some of those words in this code are. For example, "self.op" and "lambda". I just mainly know how to do the really basic things in Python. Thanks for the feedback though, I will definitely try learning some of these things.\$\endgroup\$

  TheSuds13

  – TheSuds13

  2013-09-15 04:36:47 +00:00

  CommentedSep 15, 2013 at 4:36
• \$\begingroup\$int has unlimited precision. :)\$\endgroup\$

  flornquake

  – flornquake

  2013-09-15 05:14:59 +00:00

  CommentedSep 15, 2013 at 5:14
• \$\begingroup\$Thanks @200_success I appreciate you explaining this more to me. What is the best way to learn Python by myself? It feels like I can't get past the basics, and I'm practically teaching myself. Any suggestions?\$\endgroup\$

  TheSuds13

  – TheSuds13

  2013-09-15 05:22:03 +00:00

  CommentedSep 15, 2013 at 5:22
• \$\begingroup\$@flornquake Yes, but 1.5 + 1.5 = 2 would be lossier than the calculator in MS Windows. =) Thedecimal module would be ideal.\$\endgroup\$

  200_success

  – 200_success

  2013-09-15 05:38:45 +00:00

  CommentedSep 15, 2013 at 5:38
• \$\begingroup\$@TheSuds13 You're already on the right track! Learning to program is like learning any foreign language — the most effective way is immersion. Practice. Learn from your bugs. Study others' code. Ask for code reviews. Follow the official Python tutorial and documentation; skip the parts you don't understand, and come back to them years later, after dabbling in other languages. This was probably not the advice you were looking for, but it's the truth!\$\endgroup\$

  200_success

  – 200_success

  2013-09-15 05:49:45 +00:00

  CommentedSep 15, 2013 at 5:49

• python
• python-3.x
• calculator