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()
4 Answers 4
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/elsestatements. 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
mathtwice. 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.
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*piYou’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.141592653589793This function seems poorly named:
#Returns the square of num1 def sqrt(num1): return num1**2That 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 beelif 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.
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.
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.
Explore related questions
See similar questions with these tags.
def sqrt(num1)todef sqr(num1)as sqrt is the square root function in the math module \$\endgroup\$