4
\$\begingroup\$

After a bit of a break away from any coding, I decided to return with something simple. As I haven't done much of GUI programming with Python, and I would like to do more, I decided to make a simple calculator. If anyone is so kind, I'd like some feedback on anything that might be improved. If it makes a difference in how you'd give feedback, I consider myself an intermediate Python coder (whether that's actually true remains to be seen). Some things I'm curious about in particular:

  • Logic - Am I using any convoluted chains of logic that can be simplified?
  • General Style - I've attempted to adhere to PEP 8
  • Key bindings - Is there a more effective way to bind so many keys?
  • Bugs - Did I add in some unnoticed "features"? How can I avoid them in the future?
  • (Opinion based) Tkinter: Even a good choice/something to pursue?
import tkinter as tk

from decimal import Decimal
from tkinter import ttk


class Calculator(ttk.Frame):
  def __init__(self, master=tk.Tk()):
    super().__init__(master)
    self.master = master
    self.master.title("tKalculator")
    self.operator = None
    self.prev_num = 0
    self.completed_calculation = False
    self.grid()

    self.create_widgets()
    self.bind_keys()
    self.arrange_widgets()

    self.mainloop()

  def create_widgets(self):
    """
    Create all calculator buttons
    and widgets
    """

    self.number_buttons = []

    # Number display
    self.top_display_space = ttk.Label(self, text=" ")
    self.display = tk.Text(self, height=1, width=30, pady=4)
    self.display.insert(1.0, "0")
    self.display["state"] = "disabled"
    self.bottom_display_space = ttk.Label(self, text=" ")

    # Number buttons
    for num in range(0, 10):
      num = str(num)
      self.number_buttons.append(
        # note to self: The num=num is necessary here
        ttk.Button(self, text=num, command=lambda num=num: self.update(num))
                                 )

    self.decimal_button = ttk.Button(self, text=".",
                    command=lambda: self.update("."))

    # Special Buttons
    self.clearall_button = ttk.Button(self, text="A/C", command=self.all_clear)
    self.clear_button = ttk.Button(self, text="C", command=self.clear)

    # Math operators
    self.add_button  = ttk.Button(self, text="+", command=lambda: self.math("+"))
    self.sub_button  = ttk.Button(self, text="-", command=lambda: self.math("-"))
    self.mult_button = ttk.Button(self, text="X", command=lambda: self.math("x"))
    self.div_button  = ttk.Button(self, text="/", command=lambda: self.math("/"))
    self.eql_button  = ttk.Button(self, text="=", command=lambda: self.math("="))

  def arrange_widgets(self):
    """
    Arrange all calculator widgets.
    """

    # Display
    self.top_display_space.grid(row=0, column=1)
    self.display.grid(row=1, column=0, columnspan=5)
    self.bottom_display_space.grid(row=2, column=2)

    # Number buttons
    row = 3
    column = 1
    for i in range(1, 10):
      self.number_buttons[i].grid(row=row, column=column)
      column += 1
      if column > 3:
        column = 1
        row += 1

    self.number_buttons[0].grid(row=6, column=2)
    self.decimal_button.grid(row=6, column=1)

    # Special Buttons
    self.clearall_button.grid(row=7, column=1)
    self.clear_button.grid(row=6, column=3)

    # Math operator buttons
    self.add_button.grid(row=7, column=2)
    self.sub_button.grid(row=7, column=3)
    self.mult_button.grid(row=8, column=2)
    self.div_button.grid(row=8, column=3)
    self.eql_button.grid(row=8, column=1)

  def bind_keys(self):
  """
  Binds events to keyboard button presses.
  """

    # Numbers
    self.master.bind("1", self.keypress_handler)
    self.master.bind("<KP_1>", self.keypress_handler)
    self.master.bind("2", self.keypress_handler)
    self.master.bind("<KP_2>", self.keypress_handler)
    self.master.bind("3", self.keypress_handler)
    self.master.bind("<KP_3>", self.keypress_handler)
    self.master.bind("4", self.keypress_handler)
    self.master.bind("<KP_4>", self.keypress_handler)
    self.master.bind("5", self.keypress_handler)
    self.master.bind("<KP_5>", self.keypress_handler)
    self.master.bind("6", self.keypress_handler)
    self.master.bind("<KP_6>", self.keypress_handler)
    self.master.bind("7", self.keypress_handler)
    self.master.bind("<KP_7>", self.keypress_handler)
    self.master.bind("8", self.keypress_handler)
    self.master.bind("<KP_8>", self.keypress_handler)
    self.master.bind("9", self.keypress_handler)
    self.master.bind("<KP_9>", self.keypress_handler)
    self.master.bind("0", self.keypress_handler)
    self.master.bind("<KP_0>", self.keypress_handler)
    self.master.bind(".", self.keypress_handler)
    self.master.bind("<KP_Decimal>", self.keypress_handler)

    # Special buttons
    self.master.bind("c", self.keypress_handler)
    self.master.bind("a", self.keypress_handler)
    self.master.bind("C", self.keypress_handler)
    self.master.bind("A", self.keypress_handler)
    self.master.bind("<BackSpace>", self.backspace)

    # Math operator buttons
    self.master.bind("+", self.keypress_handler)
    self.master.bind("-", self.keypress_handler)
    self.master.bind("*", self.keypress_handler)
    self.master.bind("x", self.keypress_handler)
    self.master.bind("/", self.keypress_handler)
    self.master.bind("<KP_Add>", self.keypress_handler)
    self.master.bind("<KP_Subtract>", self.keypress_handler)
    self.master.bind("KP_Multiply>", self.keypress_handler)
    self.master.bind("<KP_Divide>", self.keypress_handler)

    # Attempt to math
    self.master.bind("<KP_Enter>", self.keypress_handler)
    self.master.bind("<Return>", self.keypress_handler)
    self.master.bind("=", self.keypress_handler)

    # Escape to close the calculator
    self.master.bind("<Escape>", self.keypress_handler)

  def backspace(self, event):
  """
  Remove one character from the display.
  """

    self.display["state"] = "normal"
    current = self.display.get(1.0, tk.END)
    self.display.delete(1.0, tk.END)
    current = current[:-2]

    # Make sure that the display is never empty
    if current == "":
      current = "0"

    self.display.insert(1.0, current)
    self.display["state"] = "disabled"

  def keypress_handler(self, event):
  """
  Handles any bound keyboard presses.
  """

    char_keycode = '01234567890.'
    char_operator = "+-x*/"

    if (event.char in char_keycode):
      self.update(event.char)

    elif event.char in char_operator:
      self.math(event.char)

    elif event.char == "\r" or event.char == "=":
      self.math("=")

    elif event.char == "\x1b":
      self.master.destroy()

    elif event.char == "c" or event.char == "C":
      self.clear()

    elif event.char == "a" or event.char == "A":
      self.all_clear()

  def update(self, character):
  """
  Handles all updating of the number display.
  """
    # Allow editing of the display
    self.display["state"] = "normal"

    # Get the current number
    num = self.display.get(1.0, tk.END)
    # clear the display
    self.display.delete(1.0, tk.END)

    # Remove "\n"
    num = num.strip()

    # Clear num provided we're not putting a 
    # decimal after a zero
    if num == "0" and not character == ".":
      num = ""

    num = f"{num}{character}"

    self.display.insert(1.0, f"{num}")
    self.display["state"] = "disabled"

  def all_clear(self):
  """
  Resets everything for starting a
  new calculation.
  """
    self.clear()
    self.prev_num = 0
    self.operator = None

  def clear(self):
  """
  Clears the display by removing
  any current text and setting the
  display to 0
  """
    self.display["state"] = "normal"
    self.display.delete(1.0, tk.END)
    self.display.insert(1.0, "0")
    self.display["state"] = "disabled"

  def math(self, operator):
  """
  Handle any actual math.
  """
    if not self.operator:
      # If an operator doesn't exist, the
      # calculator is waiting for a new 
      # input.
      self.operator = operator
      self.prev_num = self.display.get(1.0, tk.END)
      self.clear()

    else:
      # The calculator is ready to do some math.
      self.prev_num = Decimal(self.prev_num)
      curr_num = self.display.get(1.0, tk.END)
      curr_num = Decimal(curr_num)

      if self.operator == "+":
        self.prev_num += curr_num
      elif self.operator == "-":
        self.prev_num -= curr_num
      elif self.operator == "x":
        self.prev_num *= curr_num
      elif self.operator == "/":
        self.prev_num /= curr_num
        
      self.operator = operator

      if self.operator == "=":
        # It's now time to show the current result
        # of all calculations.
        self.display["state"] = "normal"
        self.display.delete(1.0, tk.END)
        self.display.insert(1.0, str(self.prev_num))
        self.display["state"] = "disabled"
        self.completed_calculation = True
      else:
        # We're ready for another number to
        # perform calculations on
        self.clear()

if __name__ == "__main__":
  calc = Calculator()

I realized a while after posting that there is a better way to bind the keys: A for loop (duh). While I've implemented that in my updated code, I've kept the original here.

\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

It's a small note, but let's take this:

    self.number_buttons = []

And this:

    for num in range(0, 10):
      num = str(num)
      self.number_buttons.append(
         # note to self: The num=num is necessary here
        ttk.Button(self, text=num, command=lambda num=num: self.update(num))
                                 )

And use a list comprehension.

    self.number_buttons = [
      ttk.Button(self, text=num, command=lambda num=num: self.update(num))
      for n in range(10)
      for num in [str(n)]
    ]

This building a list pattern is rather common in Python, so hopefully this small tweak will help you as you move forward.

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

Portability

I get syntax errors due to illegal indentation:

def bind_keys(self):
"""
Binds events to keyboard button presses.
"""

The docstring needs to be indented one level:

def bind_keys(self):
  """
  Binds events to keyboard button presses.
  """

Perhaps your version of Python is more forgiving.

Layout

The code does not use a large enough indentation; 4 spaces is recommended. The black program can be used to automatically reformat the code.

Divide

In the math function, this code does not check if curr_num is 0 before doing the division:

self.prev_num /= curr_num

I see an error message in my shell (not in the Tk GUI) when I try to divide a number by 0. It would be cleaner to handle that case in the code.

UX

It would be nice to add information to the GUI to explain what the "C" and "A/C" buttons do.

DRY

The event.char comparison is duplicated in this line:

elif event.char == "c" or event.char == "C":

It can be simplified to check case insensitivity using lower:

elif event.char.lower() == "c":

Simpler

This line:

self.display.insert(1.0, f"{num}")

is simpler as:

self.display.insert(1.0, num)

When I see an if/else with a not in it:

if not self.operator:
else:

I prefer to invert the polarity to make it easier to understand:

    if self.operator:
    else:

This means you need to swap the lines in each branch as well.

Comments

This comment should be removed:

# note to self: The num=num is necessary here

It doesn't convey much information, and essentially every line of code could have a similar comment. Alternately, you could add more of an explanation in the comment describing why the code is necessary.

Or, you could re-write the code in a style that is easier for you to understand.

\$\endgroup\$
1
  • \$\begingroup\$ Totally opinion territory, but while I'd generally agree with you on if not ... the one exception I'd make is if it puts the simpler branch first. As can be seen in the if operator == "=": conditional in the OP's code. \$\endgroup\$ Commented yesterday

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.