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

    3
    \$\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\$CommentedSep 14, 2023 at 13:10

      Start asking to get answers

      Find the answer to your question by asking.

      Ask question

      Explore related questions

      See similar questions with these tags.