2
\$\begingroup\$

I just learned Python and Tkinter and I've just made a simple calculator using Tkinter for a small project at my school. It can do addition, subtraction, multiplication and division. I think that my code is hacky and not clean. How can I improve this?

import tkinter as tk

def button_0():
    print("button 0 clicked")
    add_character_to_display("0")

def button_1():
    print("button 1 clicked")
    add_character_to_display("1")

def button_2():
    print("button 2 clicked")
    add_character_to_display("2")

def button_3():
    print("button 3 clicked")
    add_character_to_display("3")

def button_4():
    print("button 4 clicked")
    add_character_to_display("4")

def button_5():
    print("button 5 clicked")
    add_character_to_display("5")

def button_6():
    print("button 6 clicked")
    add_character_to_display("6")

def button_7():
    print("button 7 clicked")
    add_character_to_display("7")

def button_8():
    print("button 8 clicked")
    add_character_to_display("8")

def button_9():
    print("button 9 clicked")
    add_character_to_display("9")

def button_plus():
    print("button + clicked")
    do_operator_mode("plus")

def button_minus():
    print("button - clicked")
    do_operator_mode("minus")

    
def button_mutiply():
    print("button * clicked")
    do_operator_mode("mutiply")
    
def button_divide():
    print("button / clicked")
    do_operator_mode("divide")

def button_dot():
    print("button . clicked")
    add_character_to_display(".")

def button_equal():
    print("button = clicked")
    do_equal_mode() 

def on_enter(e):
    e.widget['background'] = '#fafafa'

def on_leave(e):
    e.widget['background'] = 'SystemButtonFace'


def create_button(root, text, row, col, command=None):
    button = tk.Button(root, text = text, padx = 20, pady = 20, command = command)
    button.grid(row = row, column = col)
    button.bind("<Enter>", on_enter)
    button.bind("<Leave>", on_leave)

    return button

def delete_display():
    display.delete(0, tk.END)

def add_display(text):
    display.insert(0, text)
    
def default_display():
    delete_display()
    add_display(0)
    
def is_string_only_zero(string):
    for idx in string:
        if idx != '0':
            return False
    return True

def add_character_to_display(new_number):
    if(mode == "operator_input"): 
        return
    current_text = display.get()
    delete_display()

    if(not current_text or is_string_only_zero(current_text)):
        if new_number == '.':
            add_display(current_text + new_number)
        else:
            add_display(new_number)
            
    else:
        add_display(current_text + new_number)
        
def do_operator_mode(input_mode):
    global mode
    global first_number
    if  mode == "number_input" or mode == "operator_input":
        first_number = float(display.get())
        mode = input_mode
        default_display()
        
def do_equal_mode():
    global mode
    global first_number
    second_number = float(display.get())

    if mode == "plus":
        result = first_number + second_number
    elif mode == "minus":
        result = first_number - second_number
    elif mode == "mutiply":
        result = first_number * second_number
    elif mode == "divide":
        result = first_number / second_number
        
    delete_display()
    add_display(str(result))
    first_number = result
    mode = "operator_input"
    
root = tk.Tk()

display = tk.Entry(root, width=30)
display.grid(row = 0, column = 0, columnspan=4)


global mode
mode = "number_input"
global first_number 
first_number = 0

default_display()

buttons = [
    ('7', 1, 0, button_7), ('8', 1, 1, button_8),   ('9', 1, 2, button_9),     ('/', 1, 3, button_divide),
    ('4', 2, 0, button_4), ('5', 2, 1, button_5),   ('6', 2, 2, button_6),     ('*', 2, 3, button_mutiply),
    ('1', 3, 0, button_1), ('2', 3, 1, button_2),   ('3', 3, 2, button_3),     ('-', 3, 3, button_minus),
    ('0', 4, 0, button_0), ('.', 4, 1, button_dot), ('=', 4, 2, button_equal), ('+', 4, 3, button_plus),
]
for button_data in buttons:
    create_button(root, *button_data)

root.mainloop()
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Issues

Background colors

Traceback (most recent call last):
  File "/usr/lib/python3.11/tkinter/__init__.py", line 1948, in __call__
    return self.func(*args)
           ^^^^^^^^^^^^^^^^
  File "/home/…/calculator.py", line 72, in on_leave
    e.widget['background'] = 'SystemButtonFace'
    ~~~~~~~~^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/tkinter/__init__.py", line 1713, in __setitem__
    self.configure({key: value})
  File "/usr/lib/python3.11/tkinter/__init__.py", line 1702, in configure
    return self._configure('configure', cnf, kw)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/tkinter/__init__.py", line 1692, in _configure
    self.tk.call(_flatten((self._w, cmd)) + self._options(cnf))
_tkinter.TclError: unknown color name "SystemButtonFace"

Yeah, not a great start… Chances are that, on anything except Windows, SystemButtonFace is not a valid background color and you need to store it from a default button to be able to restore it later. One way would be to use yet another global variable to store it from your create_button calls. A better alternative would be to create a closure that will remember this value and use it as your event handler. This will also allows you to consolidate on_enter and on_leave into a single function:

def change_background_handler(color):
    def on_event(event):
        event.widget['background'] = color
    return on_event


def create_button(…):
    …
    button.bind("<Enter>", change_background_handler('#fafafa'))
    button.bind("<Leave>", change_background_handler(button['bg']))

Chaining operations

Using a calculator, it is expected that selecting an operator right after a computation is performed will reuse the result as the first operand. But:

  1. One should still be able to enter a completely new first operand after pressing the = sign, which your code forbid.
  2. It is expected that selecting a second operator performs an implicit press on the = sign just before. Instead, your code wrongfully discard the second operator input.

The fix is rather simple as you just need to use 0 and "plus" as your default first operand and mode (and reset to it after a press on =), and have your do_operator_mode call do_equal_mode first. This will allows you to get rid of all the checks of the form mode == 'xxx_input'.

In order to be able to reset the display if the user want to enter a fresh new first operand after a press on =, you may need to introduce a new variable that tells you if you need to clear the display or not.

Improvements

Operators

Instead of using strings to convey information about the operation to perform, have a look at the operator module. You can directly pass operator.add, operator.sub, operator.mul, operator.truediv around instead.

Buttons

You already know about unpacking, let's leverage that to remove all your button_X functions. Their main purpose is to transform a multi-parameter function like your do_xxx_mode into a parameter-less function suitable for the button command argument. But you can create them on the fly:

def create_button(root, text, row, col, *command):
    if not command:
        on_click = None
    else:
        command, *arguments = command
        def on_click():
            command(*arguments)
    button = tk.Button(root, text=text, padx=20, pady=20, command=on_click)
    …

Window creation

Since you already manually list the parameters to all your create_button calls, the for loop does not add much here and you would be better of just calling it with the appropriate parameters:

create_button(root, '0', 4, 0, add_character_to_display, '0')
create_button(root, '1', 3, 0, add_character_to_display, '1')
create_button(root, '2', 3, 1, add_character_to_display, '2')
create_button(root, '3', 3, 2, add_character_to_display, '3')
create_button(root, '4', 2, 0, add_character_to_display, '4')
create_button(root, '5', 2, 1, add_character_to_display, '5')
create_button(root, '6', 2, 2, add_character_to_display, '6')
create_button(root, '7', 1, 0, add_character_to_display, '7')
create_button(root, '8', 1, 1, add_character_to_display, '8')
create_button(root, '9', 1, 2, add_character_to_display, '9')
create_button(root, '.', 4, 1, add_character_to_display, '.')
create_button(root, '+', 4, 3, do_operator_mode, operator.add)
create_button(root, '-', 3, 3, do_operator_mode, operator.sub)
create_button(root, '*', 2, 3, do_operator_mode, operator.mul)
create_button(root, '/', 1, 3, do_operator_mode, operator.truediv)
create_button(root, '=', 4, 2, do_equal_mode)

Globals

Using globals like this is not ideal, consider passing the relevant variables around as parameters to functions. Consider using a list (meh) or a class to store values that are tied with the same concept and reduce the number of arguments to your functions.

Top-level code

Instead of putting blocks of code at the top-level of your code, consider using an if __name__ == '__main__' guard. It will make the code easier to deal with when importing / testing it.

Further

As with many GUI, it is advisable to regroup dependent parts of you application into a class to simplify reasoning about and ease passing of variables / context. To me, the whole Entry + Buttons you define here would be better of in a class like that.

Proposed improvements

import tkinter as tk
from operator import add, sub, mul, truediv


class Calculator:
    def __init__(self, root):
        display = tk.Entry(root, width=30)
        display.insert(tk.END, '0')
        display.grid(row=0, column=0, columnspan=4)

        self.add_button(root, '0', 4, 0, self.new_character, '0')
        self.add_button(root, '1', 3, 0, self.new_character, '1')
        self.add_button(root, '2', 3, 1, self.new_character, '2')
        self.add_button(root, '3', 3, 2, self.new_character, '3')
        self.add_button(root, '4', 2, 0, self.new_character, '4')
        self.add_button(root, '5', 2, 1, self.new_character, '5')
        self.add_button(root, '6', 2, 2, self.new_character, '6')
        self.add_button(root, '7', 1, 0, self.new_character, '7')
        self.add_button(root, '8', 1, 1, self.new_character, '8')
        self.add_button(root, '9', 1, 2, self.new_character, '9')
        self.add_button(root, '.', 4, 1, self.new_character, '.')
        self.add_button(root, '+', 4, 3, self.new_operation, add)
        self.add_button(root, '-', 3, 3, self.new_operation, sub)
        self.add_button(root, '*', 2, 3, self.new_operation, mul)
        self.add_button(root, '/', 1, 3, self.new_operation, truediv)
        self.add_button(root, '=', 4, 2, self.perform_operation)

        self.display = display
        self.operator = add
        self.previous_result = 0
        self.need_display_refresh = False

    @staticmethod
    def add_button(root, text, row, col, *command):
        if not command:
            on_click = None
        else:
            command, *arguments = command
            def on_click():
                command(*arguments)
        button = tk.Button(root, text=text, padx=20, pady=20, command=on_click)
        button.grid(row=row, column=col)
        button.bind("<Enter>", change_background_handler('#fafafa'))
        button.bind("<Leave>", change_background_handler(button['bg']))

    def parse_display(self):
        content = self.display.get()
        if '.' in content:
            return float(content)
        return int(content)

    def new_character(self, new_number):
        current_text = self.display.get()
        if self.need_display_refresh or (new_number != '.' and only_zeroes(current_text)):
            self.display.delete(0, tk.END)
        self.display.insert(tk.END, new_number)
        self.need_display_refresh = False

    def new_operation(self, operator):
        self.perform_operation()
        self.previous_result = self.parse_display()
        self.operator = operator

    def perform_operation(self):
        result = self.operator(self.previous_result, self.parse_display())
        self.display.delete(0, tk.END)
        self.display.insert(tk.END, str(result))
        self.need_display_refresh = True
        self.operator = add
        self.previous_result = 0


def change_background_handler(color):
    def on_event(event):
        event.widget['background'] = color
    return on_event


def only_zeroes(text):
    return all(char == '0' for char in text)


if __name__ == '__main__':
    root = tk.Tk()
    Calculator(root)
    root.mainloop()

TODOs

At least two buttons feels missing from your calculator: "clear" and "remove the last character".

You'll also need to improve the way numbers are retrieved from the Entry and properly handle inputs such as 3.14.159... which will otherwise leave your calculator in a broken state.

\$\endgroup\$
5
\$\begingroup\$

DRY - don't repeat yourself

All the button_X functions can be replaced by a single parametrized button(digit: int) function:

def button(digit: int):
    print("button {} clicked".format(digit))
    add_character_to_display(str(digit))

Meaningful names

Method's name should tell us what this method is doing. All your method names are fine in this regard except for default_display. set_display_to_default would explain its purpose better.

def is_string_only_zero(string):
    for idx in string:
        if idx != '0':
            return False
    return True

Name idx usually stands for "index" and is used when we iterate over indices of some iterable: for idx, dog in enumerate(my_dogs). For iterating over characters of a string use char or something similar:

def is_string_only_zero(string: str) -> bool:
    return all(char == '0' for char in string)

Don't use global names, they make code harder to test and debug, pass values around as function parameters instead.

Instead of

def do_equal_mode():
    global mode
    global first_number

do

def do_equal_mode(mode, first_number):

Since your methods change these values, you can return them from the function: return "operator_input", result and then assign on the call site: mode, first_number = do_equal_mode(mode, first_number)


Enums

Your code uses "mods" in the form of strings. Strings are not the best for this purpose as one typo can break the flow of the program in unexpected ways. Consider using enums instead.


Nitpicks:

In python named parameter values are specified without spaces around the = sign:

button = tk.Button(root, text=text, padx=20, pady=20, command=command)
button.grid(row=row, column=col)

instead of

button = tk.Button(root, text = text, padx = 20, pady = 20, command = command)
button.grid(row = row, column = col)
\$\endgroup\$
1
  • \$\begingroup\$ To be clear, In python named parameter values are specified without spaces around the = sign is due to recommendation from PEP8. \$\endgroup\$
    – Reinderien
    Sep 14, 2023 at 13:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.