22
\$\begingroup\$

I've just wrote my first code, which is basically a calculator. Please rate it and suggest improvements (keeping in mind that I'm a beginner).

import math

#Returns the sum of num1 and num2
def add(num1, num2):
    return num1 + num2

import math

#Returns the difference between num1 and num2
def sub(num1, num2):
    return num1 - num2

#Returns product of num1 and num2
def mul(num1, num2):
    return num1 * num2

#Returns the quotient of num1 and num2
def div(num1, num2):
    return num1 / num2

#Returns the result of num1 to the power of num2    
def exp(num1, num2):
    return num1 ** num2

#Returns the square of num1
def sqrt(num1):
    return num1**2

#Returns the value of pi
def pi():
    pi = 3.14159265
    return pi

#Returns the value of num1 multiplied with pi
def mulpi(num1):
    pi = 3.14159265
    return num1*pi


def main():
    operation = input("What do you want to do? (+,-,*,/,exp,pi,_pi,sqrt)")
    if(operation != '+' and operation != '-' and operation != '*' and operation != '/' and operation != 'exp' and operation != '_pi' and operation != 'sqrt' and operation != 'pi'):
        #Invalid Operation
        print("YOU MUST ENTER A VALID OPERATION")
    elif(operation == 'pi'):
        print(pi()) 
    elif(operation == '_pi' or 'sqrt'):
        var1 = int(input("enter number: "))
        if(operation == '_pi'):
            print(mulpi(var1))
        elif(operation == 'sqrt'):
            print(sqrt(var1))
    else:
        var1 = int(input("enter num1: "))
        var2 = int(input("enter num2: "))
        if(operation == '+'):
            print (add(var1, var2))
        elif(operation == '*'):
            print(mul(var1, var2))
        elif(operation == '-'):
            print(sub(var1, var2))
        elif(operation == 'exp'):
            print(exp(var1, var2))
        else:
            print(div(var1, var2))

main()
\$\endgroup\$
  • 4
    \$\begingroup\$ While it's helpful in your situation (beginner level, getting warm with datatypes and so), comments don't generally tell what is done in a piece of code, but why you have written it that way. Usually, experienced coders will figure out what happens at a specific block of code, but they can't figure out why you have done it that way (as there are always multiple ways and some obviouse ways are wrong at the second look). \$\endgroup\$ – Sempie Sep 14 '15 at 9:32
  • 2
    \$\begingroup\$ However, those comments look like docstrings. (Which should be writen inside the function and inside triple quotes). Docstrings do say what code does as they are API reference \$\endgroup\$ – Lyndon White Sep 14 '15 at 11:17
  • \$\begingroup\$ You should probably change def sqrt(num1) to def sqr(num1) as sqrt is the square root function in the math module \$\endgroup\$ – SirParselot Sep 14 '15 at 15:26
24
\$\begingroup\$

Input validation

You do absolutely no input validation when the user enters an integer. If the user enters a non-integer value, like foo, or bar, your code will crash with a ValueError. The correct way to obtain user input is to use a try/except block, like this:

try:
    var1 = int(input( ... ))
    var2 = int(input( ... ))

    ...
except ValueError:
    print("Invalid input!")
    ... 

By doing this, you can be sure that your program won't crash on invalid input.


Input mappings

Right now, you're chaining if/elif/else statements to check user input. This is not an optimal way to do this. Rather, you can create a dictionary mapping, like the below, for possible inputs:

OPERATIONS = {
    "+": add,
    "-": sub,
    ...
}

Then, all you have to do to get, and run user input is something like this:

operation = input( ... )
if operation in OPERATIONS:
    OPERATIONS[operation](var1, var2)
else:
    print("Invalid operation!")

This makes your code much clearer, and easier to read, rather than a chain of if/elif/else statements.


Use math and operator

There is absolutely no reason to define your own mathematical functions, just so you can use them in a dictionary mapping, you should be using the math and operator modules instead. These all provide built-in functions, like add, and sub, or values, like \$\pi\$ or \$e\$.


General nitpicks

  • There's no need for parentheses around if/elif/else statements. They can be written like this:

    if condition:
        ...
    
  • You should be using an if __name__ == "__main__" guard. This ensures that if your code is imported to another file, certain code will not be run. This means that this function call right here:

    main()
    

    Should become this:

    if __name__ == "__main__":
        main()
    
  • There's no need to import math twice. Once it's imported, it's functions and variables already populate the file's namespace.

  • Finally, if you're going to write comments for your functions (in this case your comments are fairly useless), you should be using docstrings instead. A typical docstring looks something like this:

    def my_func( ... ):
        """Brief description.
    
        More detailed description.
    
        Keyword arguments:
        argument -- Argument description.
        """
        ...
    

    Keep in mind though, if your code is self-commenting, and clearly written, the comments probably aren't needed.

\$\endgroup\$
12
\$\begingroup\$

Here are a few comments:

  • Read PEP 257, which explains docstrings. Docstrings are the preferred way to document functions in Python, not comments above the definition.

  • You’re importing the math module twice. You only need to do it once, and per PEP 8, the Python style guide, they should go at the top of the file.

    Having said that, you aren’t using it at all.

  • In these two functions:

    #Returns the value of pi
    def pi():
        pi = 3.14159265
        return pi
    
    #Returns the value of num1 multiplied with pi
    def mulpi(num1):
        pi = 3.14159265
        return num1*pi
    

    You’re hard-coding the value of pi, and ignoring a function you defined specifically for looking up pi. If you’re going to do this, it would be better to declare pi as a constant at the top of the file. But better would be to use the version of pi from the math module:

    >>> import math
    >>> math.pi
    3.141592653589793
    
  • This function seems poorly named:

    #Returns the square of num1
    def sqrt(num1):
        return num1**2
    

    That name is usually associated with the square root of a number, not the square.

  • This line is long an unwieldy, and will just get longer as you add more operations:

    if(operation != '+' and operation != '-' and operation != '*' and operation != '/' and operation != 'exp' and operation != '_pi' and operation != 'sqrt' and operation != 'pi'):
    

    It would be better to define an operations list, and then check whether it falls within that list. As a bonus, you can tie this to the help line:

    operations = ['+', '-', '*', '/', 'exp', 'pi', '_pi', 'sqrt']
    operation = input("What do you want to do? ({ops})".format(','.join(operations)))
    if operation not in operations:
        ...
    

    I’m not a fan of the '_pi' operation, which is not at all intuitive to me.

  • There’s no input validation. I could enter non-numbers, or weird numbers, or values that don’t make sense. If I try to divide by zero, for example, what happens?

  • Your logic for choosing the operation doesn’t work quite like you think it does. In particular, this line:

     elif(operation == '_pi' or 'sqrt'):
    

    will always be executed. The reason is that Python breaks this down as (parentheses mine):

     elif (operation == '_pi') or 'sqrt':
    

    and implicitly coerces the string 'sqrt' to True. A better approach would be

     elif operation in ['_pi', 'sqrt']:
    
  • Rather than choosing an operation based on a series of nested if statements, it would be better to use a dict that makes operation strings to operator functions. That will make it easier to add new operations later.

\$\endgroup\$
8
\$\begingroup\$

Main

First off it's good that you used a main. However if I were to accidentally import your program, I would probably not want it to run.
To safe guard this you can wrap the call in if __name__ == '__main__':

If your program was called 'calc.py' the following would run your program.

import calc

With if __name__ == '__main__':, you code will not run.
I could still however do calc.mul(1, 2).


Dictionaries and functions as first class citizens.

Python is friendly with functions. It allows you to pass them around as if they were variables.

def my_func():
    return 1

my_new_func = my_func
print(my_new_func) # 1

This may seem pointless at first, however combined with dictionaries and lists, you can do some cool stuff.

A dictionary is like a list, however it can have strings as well as integers as it's keys.

my_dict = {'foo': 'bar'}
print(my_dict['foo']) # bar

Now onto the magic!

If we were to combine these two things together, how many if statements could we remove?

my_functions = {
    '+': add,
    '-': sub,
    # etc
}

print(my_functions['+'](2, 3)) # 5

But how would you use this?

You can change the else of main to contain this.

var1 = int(input("enter num1: "))
var2 = int(input("enter num2: "))
print(my_functions[operation](var1, var2))

It's now that simple.


User input errors

Your main's else statement looks nice, but there is still a problem.

enter num1: 43
enter num2: 51q
# program exits.

This is quite simple to amend actually.

First if you don't know what the try except statements are.

try:
    1 / 0
except:
    print("you can't divide by zero.")

This will, without fail, always print 'you can't divide by zero.'.

How can we use it in your program to fix the above?

try:
    return int(input("enter num2: "))
except:
    print('Enter valid numbers.')
    return 1

This is a very rudimentary version. You would want to change it to be safer, and loop.

def ask(question, fn=int):
    while True:
        answer = input(question)
        try:
            return fn(answer)
        except ValueError:
            print('\r\b\r')

You can then use the function in place for int(input())

Now the else of the main is simpler.

var1 = ask("enter num1: ")
var2 = ask("enter num2: ")
print(my_functions[operation](var1, var2))

Removing the if

I think that having such a massive initial if is not a good idea.

If I enter 'pi' it will check if it's '+', '-', '*', '/', 'exp', '_pi', 'sqrt' and 'pi'. Then it will go on to check if it's 'pi'.

That is a bit too much in my opinion. And I would make it the else.

To make the transition easy you could make more dictionaries!
But before that, you should know about in with dictionaries.

my_dict = {'foo': 'bar'}
print('foo' in my_dict) # True
print('bar' in my_dict) # False
print('baz' in my_dict) # False

How would I implement the dictionary(s) and the if statements?
You can have a dictionary of 'constants', 'singletons' and 'duets'. Then the 'singletons' will be a dictionary of '_pi' and 'sqrt'.

It is easy to implement.

functions = {
    'constants': {
        'pi': pi
    },
    'singletons': {
        '_pi': mulpi,
        # etc
    },
    # etc
}

Now we can use the in dict to check if they contain the function.

if operation in functions['constants']:
    print(functions['constants'][operation]())
elif operation in functions['singletons']:
# etc
else:
    print("You must enter a valid operation")

Lambda

Finally, if you were to stick with making the functions such as mul, I would recommend doing it in a different way.

You use it once, and it's only used on one line.

And so I would use lambda.

mul = lambda a, b: a * b
print(mul(2, 5)) # 10

Please do not do the above. It's to display its functionality, use def instead.

This is good as then we can create all the functions in the dictionary.

my_functions = {
    '*': lambda a, b: a * b,
    '/': lambda a, b: a / b,
    # etc
}

To summarize, for a first program this is very good.

However, your end user is your worst enemy, and will try to break your program. And you can make it way smaller.

def ask(question, fn=int):
    while True:
        answer = input(question)
        try:
            return fn(answer)
        except ValueError:
            print('\r\b\r')

def main():
    functions = {
        'constants': {
            'pi': lambda: math.pi
        },
        'singletons': {
            '_pi': lambda a: a * math.pi,
            # etc
        },
        # etc
    }

    operation = input("What do you want to do? (+,-,*,/,exp,pi,_pi,sqrt)")
    if operation in functions['duets']:
        var1 = ask("enter num1: ")
        var2 = ask("enter num2: ")
        print(functions['duets'][operation](var1, var2))
    elif operation in functions['singletons']:
        var1 = ask("enter num1: ")
        print(functions['singletons'][operation](var1))
    elif operation in functions['constants']:
        print(functions['constants'][operation]())
    else:
        print('You must enter valid operators.')

if __name__ == '__main__':
    main()

Finally, it will still break on 1 / 0, But I'm sure you can now sort that out.

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

The others have already made a lot of great points, but I think one key simple feature you're missing is to preserve values. If you have a persistent num variable then you can just keep looping and chain operations in your calculator. This makes it more useful as well as removing repetition from your code.

def main():
    num = int(input("What's your first number?")
    while True:
        operation = input("What do you want to do? (+,-,*,/,exp,pi,_pi,sqrt)")

        ...

        elif(operation == '_pi' or 'sqrt'):
            if(operation == '_pi'):
                num = mulpi(num)
            elif(operation == 'sqrt'):
                num = sqrt(num)
        else:
            num2 = int(input("enter num2: "))
            if(operation == '+'):
                num = add(num, num2)

        ...

        print (num)

As you can see, this instead lets you keep num persistently as well as having print(num) at the bottom, for all cases. You could now change how the result is displayed much easier without editing it in multiple places.

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