8
\$\begingroup\$

I made my first Python project with the help of Tkinter. I was trying to make it works like iPhone calculator. I am a beginner and I am looking for ways to improve my code.

How it looks

I use PyCharm and got rid of all warnings, but I still think that code isn't clear enough. I know that I must comment my code, but I am going to do that later, when I will be sure that everything is ok.

GitHub project page: https://github.com/Unknown-reader/Python-Calculator

from tkinter import *
from math import *

root = Tk()
root.title("Calculator")
root.resizable(width=False, height=False)

screen = StringVar()
screen.set("0")

current = ""
power = ""

firstnum = str()
secondnum = str()
mathsign = str()

defxworking = False
percentt = False

def math_button_pressed():
    if mathsign == '+':
        button_plus.config(relief=SUNKEN)
    if mathsign == '-':
        button_minus.config(relief=SUNKEN)
    if mathsign == '*':
        button_multiply.config(relief=SUNKEN)
    if mathsign == '/':
        button_division.config(relief=SUNKEN)

def math_button_raised():
    button_plus.config(relief=RAISED)
    button_minus.config(relief=RAISED)
    button_multiply.config(relief=RAISED)
    button_division.config(relief=RAISED)

def is_int(num):
    if int(num) == float(num):
        return int(num)
    else:
        return float(num)

def number_pressed(butt):
    global current, power, firstnum, secondnum

    if mathsign == str() and defxworking == False:
        current = current + str(butt)
        screen.set(current)
        firstnum = float(current)

    elif mathsign != str() and defxworking == False:
        math_button_raised()
        current = current + str(butt)
        screen.set(current)
        secondnum = float(current)

    elif mathsign == str() and defxworking == True:
        power = power + str(butt)
        current = current + str(butt)
        screen.set(current)

    elif mathsign != str and defxworking == True:
        power = power + str(butt)
        current = current + str(butt)
        screen.set(current)
        print(power)

def math_pressed(math):
    global current, power, mathsign, firstnum, secondnum, defxworking, percentt

    if mathsign == str() and defxworking == False and percentt == False and firstnum != str():
        mathsign = str(math)
        math_button_pressed()
        current = ""

    elif mathsign != str() and defxworking == False and percentt == False:
        print(2)
        if mathsign == '+':
            firstnum = round(float(firstnum + secondnum),6)
        if mathsign == '-':
            firstnum = round(float(firstnum - secondnum),6)
        if mathsign == '*':
            firstnum = round(float(firstnum * secondnum),6)
        if mathsign == '/':
            firstnum = round(float(firstnum / secondnum),6)
        screen.set(is_int(firstnum))

        mathsign = str(math)
        math_button_pressed()
        current = ""

    elif mathsign != str() and defxworking == True and percentt == False:
        if mathsign == '+':
            firstnum = round(firstnum + secondnum ** int(power),6)
        if mathsign == '-':
            firstnum = round(firstnum - secondnum ** int(power),6)
        if mathsign == '*':
            firstnum = round(firstnum * (secondnum ** int(power)),6)
        if mathsign == '/':
            firstnum = round(firstnum / (secondnum ** int(power)),6)
        defxworking = False
        screen.set(is_int(firstnum))
        defxworking = False
        mathsign = str(math)
        math_button_pressed()
        power = ""
        current = ""

    elif defxworking and percentt == False:
        firstnum = round(firstnum ** int(power), 6)
        defxworking = False
        screen.set(is_int(firstnum))
        mathsign = str(math)
        math_button_pressed()
        power = ""
        current = ""

    elif percentt:
        if mathsign == '+':
            firstnum = round(float(firstnum + firstnum/100*secondnum),6)
        if mathsign == '-':
            firstnum = round(float(firstnum - firstnum/100*secondnum),6)
        screen.set(is_int(firstnum))
        percentt = False
        mathsign = str(math)
        math_button_pressed()
        current = ""

def squareroot():
    global firstnum, secondnum, mathsign, current

    if mathsign == str():
        firstnum = round(sqrt(firstnum),6)
        screen.set(is_int(firstnum))

    if mathsign != str():
        if mathsign == '+':
            firstnum = round(sqrt(firstnum + float(secondnum)),6)
        if mathsign == '-':
            firstnum = round(sqrt(firstnum - float(secondnum)),6)
        if mathsign == '*':
            firstnum = round(sqrt(firstnum * float(secondnum)),6)
        if mathsign == '/':
            firstnum = round(sqrt(firstnum / float(secondnum)),6)

        screen.set(is_int(firstnum))
        secondnum = str()
        mathsign = str()
        current = ""

def x():
    global firstnum, secondnum, mathsign, current, defxworking

    if mathsign == str():
        current = str(is_int(firstnum)) + '^'
        screen.set(current)
        defxworking = True

    elif mathsign != str():

        current = str(is_int(secondnum)) + '^'
        screen.set(current)
        defxworking = True

def result():
    global firstnum, secondnum, mathsign, current, power, defxworking, percentt
    if defxworking == False and percentt == False:
        if mathsign == '+':
            firstnum = round(float(firstnum + secondnum),6)
        if mathsign == '-':
            firstnum = round(float(firstnum - secondnum),6)
        if mathsign == '*':
            firstnum = round(float(firstnum * secondnum),6)
        if mathsign == '/':
            firstnum = round(float(firstnum / secondnum),6)
        screen.set(is_int(firstnum))

    if mathsign == str() and defxworking == True and percentt == False:
        firstnum = round(firstnum ** int(power),6)
        defxworking = False
        screen.set(is_int(firstnum))

    if mathsign != str() and defxworking == True and percentt == False:
        if mathsign == '+':
            firstnum = round(firstnum + secondnum ** int(power),6)
            defxworking = False
        if mathsign == '-':
            firstnum = round(firstnum - secondnum ** int(power),6)
            defxworking = False
        if mathsign == '*':
            firstnum = round(firstnum * (secondnum ** int(power)),6)
            defxworking = False
        if mathsign == '/':
            firstnum = round(firstnum / (secondnum ** int(power)),6)
            defxworking = False
        screen.set(is_int(firstnum))


    if defxworking == False and percentt == True:
        if mathsign == '+':
            firstnum = round(float(firstnum + firstnum/100*secondnum),6)
            screen.set(is_int(firstnum))
            percentt = False
        if mathsign == '-':
            firstnum = round(float(firstnum - firstnum/100*secondnum),6)
            screen.set(is_int(firstnum))
            percentt = False

    mathsign = str()
    current = ""
    power = ""

    if defxworking == False and mathsign == '*' or '/' and percentt == True:
        clear()

def clear():
    global current, firstnum, secondnum, mathsign, power, defxworking, percentt

    screen.set(0)
    current = ""
    power = ""
    firstnum = str()
    secondnum = str()
    mathsign = str()
    defxworking = False
    math_button_raised()
    percentt = False

def percent():
    global firstnum, secondnum, current, percentt

    current = str(is_int(secondnum)) + '%'
    screen.set(current)
    percentt = True



# Widgets

calculation = Entry(root, textvariable = screen, font=("Verdana", 15, ), bd = 12,
                    insertwidth=4, width=14, justify=RIGHT)
calculation.grid(columnspan=4)
#   Numbers
button1 = Button(root, text='1', command=lambda: number_pressed(1), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button1.grid(row=2, column=0, sticky=W)
button2 = Button(root, text='2', command=lambda: number_pressed(2), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button2.grid(row=2, column=1, sticky=W)
button3 = Button(root, text='3', command=lambda: number_pressed(3), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button3.grid(row=2, column=2, sticky=W)
button4 = Button(root, text='4', command=lambda: number_pressed(4), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button4.grid(row=3, column=0, sticky=W)
button5 = Button(root, text='5', command=lambda: number_pressed(5), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button5.grid(row=3, column=1, sticky=W)
button6 = Button(root, text='6', command=lambda: number_pressed(6), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button6.grid(row=3, column=2, sticky=W)
button7 = Button(root, text='7', command=lambda: number_pressed(7), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button7.grid(row=4, column=0, sticky=W)
button8 = Button(root, text='8', command=lambda: number_pressed(8), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button8.grid(row=4, column=1, sticky=W)
button9 = Button(root, text='9', command=lambda: number_pressed(9), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button9.grid(row=4, column=2, sticky=W)
button0 = Button(root, text='0', command=lambda: number_pressed(0), bg="gainsboro",
                 bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button0.grid(row=5, column=0, sticky=W)
button_float = Button(root, text='.', command=lambda: number_pressed('.'), bg="gainsboro",
                      bd=3, padx=15, pady=5, font=("Helvetica", 14, "bold"))
button_float.grid(row=5, column=1)

#   Math signs
button_plus = Button(root, text='+', command=lambda: math_pressed('+'), bg="gray70",
                     bd=3, padx=11, pady=5, font=("Helvetica", 14, "bold"))
button_plus.grid(row=2, column=3, sticky=W)
button_minus = Button(root, text='-', command=lambda: math_pressed('-'),  bg="gray70",
                      bd=3, padx=11, pady=4, font=("Verdana", 14, "bold"))
button_minus.grid(row=3, column=3, sticky=W)
button_multiply = Button(root, text='*', command=lambda: math_pressed('*'), bg="gray70",
                         bd=3, padx=13, pady=5, font=("Helvetica", 14, "bold"))
button_multiply.grid(row=4, column=3, )
button_division = Button(root, text='/', command=lambda: math_pressed('/'),  bg="gray70",
                         bd=3, padx=14, pady=5, font=("Helvetica", 14, "bold"))
button_division.grid(row=5, column=3, )
button_equal = Button(root, text='=', command=lambda: result(), bg='orange',
                      bd=3, padx=12, pady=5, font=("Arial", 14))
button_equal.grid(row=5, column=2, )

button_percent = Button(root, text='%', command=lambda: percent(),  bg="gray70",
                         bd=3, padx=8, pady=5, font=("Helvetica", 14, "bold"))
button_percent.grid(row=1, column=3, )

button_clear = Button(root, text='C', command=lambda: clear(), bg='gray70',
                      bd=3, padx=11, pady=5, font=("Helvetica", 14))
button_clear.grid(row=1, column=0)
button_sqrt = Button(root, text='√', command=lambda: squareroot(), bg="gray70",
                        bd=3, padx=12, pady=5, font=("Helvetica", 14, "bold"))
button_sqrt.grid(row=1, column=1, sticky=W)
button_x = Button(root, text='x^y', command=lambda: x(), bg="gray70",
                  bd=3, padx=6, pady=5, font=("Helvetica", 14))
button_x.grid(row=1, column=2, sticky=W)

root.mainloop()
\$\endgroup\$
5
\$\begingroup\$

Your code is organized oddly.

  • Imports
  • Variable declarations & code
  • Function definitions
  • More Variable declarations & code

You should keep the variable declarations and code together.


str() - why are you using this everywhere? It returns an empty string: "", which is 3 characters shorter to type and is much easier to understand.


elif mathsign != str and defxworking == True: does not do what you intended. It compares mathsign with the function str, not an empty string. You are missing the brackets ... mathsign != str(), but again, use mathsign != "" instead.


In several places you have code that reads ...

if mathsign == '+':
    # code
if mathsign == '-':
    # code
if mathsign == '*':
    # code
if mathsign == '/':
    # code

These last three should elif statements, not if statements.

Similarly, code like:

if mathsign == str():
   # code
if mathsign != str():
   # code

should be replaced with:

if mathsign == "":
   # code
else:
   # code

Don't Repeat Yourself (DRY). For example, in def math_pressed(math): you have every branch of your if ... elif ending in:

mathsign = str(math)
math_button_pressed()
current = ""

This can be moved out of every branch, and added as common code at the end.

Along a similar vein, I see the following over and over in the code:

if condition:
    firstnum = round( (... some operation ...), 6)
if other_condition:
    firstnum = round( (... some operation ...), 6)
if third_condition:
    firstnum = round( (... some operation ...), 6)
if fourth_condition:
    firstnum = round( (... some operation ...), 6)

How about:

if condition:
    result = ... some operation ...
elif other_condition:
    result = ... some operation ...
elif third_condition:
    result = ... some operation ...
elif fourth_condition:
    result = ... some operation ...

firstnum = round(result, 6)

Maybe you want a def precision(value) function that rounds value to the desired precision. If you want to increase the precision later, you could change this in one spot.

Again, screen.set(is_int(firstnum)) is repeated over and over, occasionally with secondnum as the argument. How about a set_screen(value) function which sets the screen variable to the int or float representation of the value?

Or a def set_firstnum(value) function which rounds the value to the desired precision, sets the firstnum variable, and updates the screen.


You have lambda functions that call functions based on which button is pressed. But you are inconsistent about what you pass to the function. Does number_pressed(butt) take an integer, such as 0 through 9, or a string such as ('.')? You pass in both types, which forces you to use str(butt) in the function itself to convert the input into a string. Instead, just always pass in a string value.

Creation of the GUI. You have written a lot of code for something that can be done in a loop.

for idx, digit in enumerate("1234567890"):
    cmd = lambda arg=digit: number_pressed(arg)
    btn = Button(root, text=digit, command=cmd, bg="gainsboro",
                 bd=3, padx=12, pady=5, font=('Helvetica', 14, bold))
    btn.grid(row = 2 + idx // 3, column = idx % 3, sticky=W)

With 5 lines, instead of 30, we create all 10 digit buttons.

Note: We needed a little bit of magic (arg=digit) to generate the lambda functions without binding the digit variable itself to the lambda function body. Without that magic, the lambda functions for all buttons would use the last value assigned to the digit variable when the lambda function gets invoked ... which would mean every button would call number_pressed('0').

You can generate the remaining buttons in a similar manner. The main difficulty is ensuring the right padx, bg, and font values, which are different for some buttons.

Also, you'll want to store the buttons for the math operators, so you can implement math_button_raised(). I'd recommend adding them to a list, so you can still generate them in a loop.


Global variables are horrible. Don't use them. A class object would make a nice container for you calculator GUI:

from tkinter import *

class Calculator:

    def __init__(self, root):
        self.current = ""
        self.firstnum = 0
        self.screen = StringVar()

        display = Entry(root, textvariable=self.screen, justify=RIGHT)
        display.grid(columnspan=4)

        for idx, digit in enumerate('1234567890.'):
           cmd = lambda arg=digit: self.number_pressed(arg)
           btn = Button(root, text=digit, command=cmd)
           btn.grid(row=2+idx//3, column=idx%3)

        # ... etc ...

    def number_pressed(self, digit):
        self.current += digit
        self.screen.set(self.current)
        self.firstnum = float(self.current)

    # ... etc ...

root = Tk()
Calculator(root)
root.mainloop()
\$\endgroup\$
3
\$\begingroup\$

The answer of AJNeufeld already has a lot of absolutely great advice you should definitely follow.

Another thing that I previously commented on under his answer is that you should not use if bool_value == True: or if bool_value == False:. Instead use if bool_value: and if not bool_value:.

If you follow this advice, and your if statements don't "read" well, maybe think about how a better names for your variables can improve clarity and readability. I would say a prime example for this would be defxworking. Even after staring at your code for some time I'm still not sure why the variable is named like this. Another instance would be percentt. At first glance I thought about a typo, but later realized that it's probably because of def percent():. I think both, the function (soon to be method if you follow the other review) and the variable could have more telling names.

Since you have already mentioned that you plan to document your code, I invite you to have a read into the "official" Style Guide for Python Code - section "Documentation Strings" (or this for further reading).
To sum it up shortly: you're encouraged to document every function with a short docstring, wrapped in """...""", immediately following the def method_name(your, args). For example:

def math_pressed(math):
    """Handle presses of math operator buttons"""

Following this convention will help Python's built-in help(...) function and also Python IDEs like PyCharm to pick-up your documentation. Future-you will also be greatful, especially if code gets more complex and spread out over multiple files.

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