3
\$\begingroup\$

I've recently finished building a calculator in Python using Tkinter, and would appreciate any and all feedback on it. Please ignore the use of the eval function, I know this is advised against using but my teacher insists that we use it in our development.

I'd really like to condense and simplify my code for validation which can be found below, but I can't think of any other way to do it. This code validates text being entered into the Tkinter entry field.

def validate_input(self, entered_value, modify_type, index):#used to validate input entered into entry field.

    current_input = str(self.text_box.get())
    index_pos = int(index)

    #Cheecks if the attempted modification type is insertion or deletion.
    if modify_type == '1': #insert

        if entered_value== ".":
            if current_input == "":
                return True#allows a decimal point to be inserted if the field is blank.
            elif index_pos == len(current_input):
                if current_input[-1] == ".":
                    return False#doesn't allow a decimal to be inserted if the last character is a decimal.
                else:
                    return True
            elif current_input[index_pos-1] == "." or current_input[index_pos] =="." :
                return False#doesn't allow a decimal to be inserted if there is a decimal point on either side of the insert position.
            else:
                return True

        if entered_value in "*/+-":

            if current_input == "" and entered_value in "*/":
                return False#doesn't allow a multiplication or division operator to be entered into an empty field.
            elif current_input == "" and entered_value in "+-":
                return True#allows an addition or subtraction operator to be entered for negative and positive numbers

            if index_pos == len(current_input):#if character is being inserted at the end of the string
                if current_input[-1] in "+-" and entered_value in "+-":
                    return True#allows two addition or subtraction signs in a row.
                elif current_input[-1] == "." and entered_value in "+-":
                    return False#doesn't allow the insertion of + or - after a decimal point.
                elif current_input[-1] in "*/+-." and entered_value in "*/":
                    return False#doesn't allow * or / to be entered after */+-
                else:
                    return True

            if entered_value in "+-":
                if current_input[index_pos-1] in "*/+-" or current_input[index_pos] in "*/+-" :
                    return True#allows a + or a - to be inserted after or before another operator.
                elif current_input[index_pos-1] == ".":
                    return False#doesn't allow a + or a - to be entered after a decimal point.
            elif entered_value in "*/":
                if current_input[index_pos-1] in "*/+-." or current_input[index_pos] in "*/+-" :
                    return False#doesn't allow a * or / to be entered if there is an operator or decimal before, or an operator after.
            else:
                return True

        #Checks if entered value is in list of accepted values, stored in setup of CalculatorGUI class.
        if entered_value in self.accepted_values:
            return True

    #Accepts all attempts to remove text from the entryfield.
    elif modify_type == "0":#delete
        return True
    return False

Full code can be found below, thanks for your help! :)

from tkinter import *
from tkinter import messagebox
import re

class CalculatorFunctions:
    def __init__(self, root):
        self.root = root

    def num_press(self, num):#function that runs if a number button is pressed
        new_input = num
        cursor_position = self.text_box.index(INSERT)#gets position of where number is trying to be inserted
        self.text_box.insert(cursor_position, new_input)#inserts number at the cursor's position in the entry field

    #Creates a message-box popup to display relevant author information.
    def show_info_popup(self):
        messagebox.showinfo("Author", "NAME", \nLast Edited: September 2018\nPython Version: 3.7.0")

    #Command that clears everything in the calculator's entrybox.
    def clear_screen(self):
        self.text_box.delete(0, END)

    #Removes the last character in the entry field.
    def backspace(self):
        current = str(self.text_box.get())
        cursor_position = self.text_box.index(INSERT)
        if cursor_position == 0:#if the insert position is at the beginning of the entry field (far left), don't backspace anything.
            pass
        else:
            #deletes the text one index position before the insert position.
            cursor_position -= 1
            self.text_box.delete(cursor_position)

    #Uses the eval function to calculate entered string in calculator.
    def calculate_answer(self):
        try:
            #regex string that removes leading zeroes from the start of numbers and replaces the matched pattern with the numbers found in group 2.
            answer = eval(re.sub(r"((?<=^)|(?<=[^\.\d]))0+(\d+)", r"\2", self.text_box.get()))
            self.accepted_values.append(str(answer)) #appends answer to list of accepted values so that it is able to be inserted into the entry field through the validation algorithm.
            self.text_box.delete(0, END) #deletes contents of entry field.
            self.text_box.insert(0, answer)#inserts answer into entry field.
        except (SyntaxError, ZeroDivisionError):#runs if a syntax error or zero division error is caught when calculating an answer.
            messagebox.showwarning("Error", "Please edit your entered input and calculate again.\nCommon errors include:\n\n -Dividing by 0\n -Including too many decimal points in one number\n -Incorrect operator usage\n- Pressing equals when the screen is empty")

    def validate_input(self, entered_value, modify_type, index):#used to validate input entered into entry field.

        current_input = str(self.text_box.get())
        index_pos = int(index)

        #Cheecks if the attempted modification type is insertion or deletion.
        if modify_type == '1': #insert

            if entered_value== ".":
                if current_input == "":
                    return True#allows a decimal point to be inserted if the field is blank.
                elif index_pos == len(current_input):
                    if current_input[-1] == ".":
                        return False#doesn't allow a decimal to be inserted if the last character is a decimal.
                    else:
                        return True
                elif current_input[index_pos-1] == "." or current_input[index_pos] =="." :
                    return False#doesn't allow a decimal to be inserted if there is a decimal point on either side of the insert position.
                else:
                    return True

            if entered_value in "*/+-":

                if current_input == "" and entered_value in "*/":
                    return False#doesn't allow a multiplication or division operator to be entered into an empty field.
                elif current_input == "" and entered_value in "+-":
                    return True#allows an addition or subtraction operator to be entered for negative and positive numbers

                if index_pos == len(current_input):#if character is being inserted at the end of the string
                    if current_input[-1] in "+-" and entered_value in "+-":
                        return True#allows two addition or subtraction signs in a row.
                    elif current_input[-1] == "." and entered_value in "+-":
                        return False#doesn't allow the insertion of + or - after a decimal point.
                    elif current_input[-1] in "*/+-." and entered_value in "*/":
                        return False#doesn't allow * or / to be entered after */+-
                    else:
                        return True

                if entered_value in "+-":
                    if current_input[index_pos-1] in "*/+-" or current_input[index_pos] in "*/+-" :
                        return True#allows a + or a - to be inserted after or before another operator.
                    elif current_input[index_pos-1] == ".":
                        return False#doesn't allow a + or a - to be entered after a decimal point.
                elif entered_value in "*/":
                    if current_input[index_pos-1] in "*/+-." or current_input[index_pos] in "*/+-" :
                        return False#doesn't allow a * or / to be entered if there is an operator or decimal before, or an operator after.
                else:
                    return True

            #Checks if entered value is in list of accepted values, stored in setup of CalculatorGUI class.
            if entered_value in self.accepted_values:
                return True

        #Accepts all attempts to remove text from the entryfield.
        elif modify_type == "0":#delete
            return True
        return False

class CalculatorGUI(CalculatorFunctions):

    def __init__(self, root):
        self.root = root
        #binds equals key to calculate an answer when pressed
        root.bind("=", lambda event: self.calculate_answer())
        #binds enter key to calculate an answer when pressed
        root.bind('<Return>', lambda event: self.calculate_answer())
        #list of values allowed to be inserted into entry field
        self.accepted_values = ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "+", "-", "*", "/", "."]
        self.create_number_buttons()
        self.create_text_box()
        self.create_special_buttons()

    def create_number_buttons(self):
        button_characters = "789*456/123-0.=+"
        #Variable that is used to iterate through button characters.
        i = 0
        #Empty list to store created buttons in.
        self.button_list = []
        #Row starts at row 2 as I will have the entry field on row 0 and AC on row 1.
        for row_counter in range(2,6):
            #I want to have a 4x4 grid of buttons, so will use column in range 4 (0,1,2,3) for each row.
            for column_counter in range(4):
                #Appends each button to a list as it is created so that individual buttons are able to be referenced at later stages of program development.
                self.button_list.append(Button(root, bg="#11708e", fg="white", pady=25, padx=35, text=button_characters[i], font=("Helvetica", 20, 'bold')))
                self.button_list[i].grid(row=row_counter, column=column_counter, sticky="NSEW")
                self.button_list[i].configure(command = lambda character=button_characters[i]: self.num_press(character))
                i += 1

                self.reconfigure_operator_buttons()

    #Reconfigures operators ro have a red background.
    def reconfigure_operator_buttons(self):
        i = 0
        for button in self.button_list:
            #Cget gets the current value of the specified button attribute, in this case being "text".
            if self.button_list[i].cget("text") in "-+/*.=":
                self.button_list[i].configure(bg="#d14302")

                if self.button_list[i].cget("text") == "=":
                    self.button_list[i].configure(command=self.calculate_answer)
            i +=1

    def create_text_box(self):
        self.text_box = Entry(root, justify=RIGHT, validate="key", font=("Helvetica", 20, 'bold'), borderwidth=15)
        self.text_box['validatecommand'] = (self.text_box.register(self.validate_input),'%S','%d', '%i')
        #Places the entry field in row 0, column 0, adds internal padding to increase width of field.
        self.text_box.grid(row=0, column=0, columnspan=4, ipady=10, sticky="WE")

    def create_special_buttons(self):

        clear_button = Button(root, bg="#302e2e", fg="white", text="AC", font=("Helvetica", 14, 'bold'), pady=10,command=self.clear_screen)
        clear_button.grid(row=1, columnspan=2, sticky="WE")

        backspace_button = Button(root, bg="#302e2e", fg="white", text="Backspace", font=("Helvetica", 14, 'bold'), pady=10, command=self.backspace)
        backspace_button.grid(row=1, column=3, sticky="NSEW")

        author_button = Button(root, bg="#302e2e", fg="white", font=("Helvetica", 14, 'bold'), text="Info", pady=10, command=self.show_info_popup)
        author_button.grid(row=1, column=2, sticky="NSEW")


root = Tk()
root.title("Calculator")
root.resizable(0, 0)
calc = CalculatorGUI(root)
root.mainloop()
\$\endgroup\$
  • \$\begingroup\$ Please do not update or remove the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Sep 24 '18 at 7:13
  • \$\begingroup\$ Welcome to CodeReview, since you already got an answer: Enjoy your stay! For Python questions you should also annotate it with the version (2/3) since it will reviewers to give better advice. \$\endgroup\$ – ferada Sep 30 '18 at 14:31
3
\$\begingroup\$

Remove redundant comments

Your code is heavy with comments, quite some which are just stating the obvious. If you have a function called clear_screen(), then you don't need a comment saying it clears the screen. Just rely on descriptive function names (which you have!) to convey the meaning. Less comments means less clutter, and makes the code itself easier to read.

Of course, not all comments are redundant, for example the one explaining the regex in calculate_answer() is very good to have.

Use appropriate names for functions and variables

You have a function num_press(self, num) that is called not only when a number is pressed, but also when operator buttons are pressed. The name of this function is therefore misleading. In this case, button_press() would be too generic perhaps, unless you would make that a function that handles all buttons. Maybe add_input(self, character) is better?

Don't unnecessarily use array indices in for-loops

If you are iterating over an array in a for-loop, you often don't need to know the actual index. For example, you can rewrite reconfigure_operator_buttons() to get rid of the variable i and make the code more compact:

def reconfigure_operator_buttons(self):
    for button in self.button_list:
        if button.cget("text") in "-+/*.=":
            button.configure(bg="#d14302")

            if button.cget("text") == "=":
                button.configure(command=self.calculate_answer)

In case you actually need a numeric index while iterating over a container, you can use the enumerate() function, like so:

    for i, button in enumerate(self.button_list):
        ...

Configure elements completely before adding to a container

In create_number_buttons(), you are using an iterator because you are modifying an element that you just added to a list. Try to first complete all operations on the element, only then add it to the container. For example:

def create_number_buttons(self):
    button_characters = "789*456/123-0.=+"
    self.button_list = []
    i = 0

    #Row starts at row 2 as I will have the entry field on row 0 and AC on row 1.
    for row_counter in range(2,6):
        for column_counter in range(4):
            button = Button(root, bg="#11708e", fg="white", pady=25, padx=35, text=button_characters[i], font=("Helvetica", 20, 'bold'))
            button.grid(row=row_counter, column=column_counter, sticky="NSEW")
            button.configure(command = lambda character=button_characters[i]: self.num_press(character))
            self.button_list.append(button)
            i += 1

Only call reconfigure_operator_buttons() once

You are calling it for every button you add, you only need to call it once. Or maybe this is just an indentation typo?

Give names to magic constants

In your code, you give colors to elements of the GUI. While a color like "white" is quite self-explanatory, something like "#302e2e" is not. Assign the color value to a variable that you can reuse. In this case, don't name the variable after the color itself, but give it a more meaningful name, like number_button_color. An advantage is that you can later easily change the actual color used by changing only a single line.

Reduce indentation if possible

In validate_input(), you have a structure that looks like:

if modify_type == '1':
    ...long piece of code...
else:
    return True

You can instead rewrite this like so:

if modify_type != '1':
    return True

...long piece of code...

Consider using a more data-driven approach for creating the button layout

Instead of having nested for-loops and if-statements to arrange your buttons and to assign functions to them, you could instead define a list with the parameters for all the buttons, and go through that list only once. For example:

buttons = [
    #chr. x  y  color                  command
    ("7", 0, 0, number_button_color,   lambda: self.num_press("7")),
    ("8", 1, 0, number_button_color,   lambda: self.num_press("8")),
    ("9", 2, 0, number_button_color,   lambda: self.num_press("9")),
    ("*", 3, 0, operator_button_color, lambda: self.num_press("*")),
    ...
]

for (character, x, y, color, command) in buttons:
    button = Button(character, bg=color, ...)
    button.grid(x, y, ...)
    button.configure(command=command)
    ...

Improve separation between presentation and logic

While you have created two classes, one for the GUI and one for the actual logic behind it, your code freely calls GUI functions in CalculatorFunctions. Try to avoid changing the GUI at all in that class. It would be better to have the logic class update its own state, and have the GUI class read and present this state.

One way to do this is to not have buttons directly call functions in the CalculatorFunctions class, but instead have it call a function in the CalculatorGUI class which will in turn call the required function in CalculatorFunctions, but also read the state and update the text_box. For example:

class CalculatorFunctions:
    def init(self, ...):
        self.current_text = ""
        ...

    def num_press(self, num):
        self.current_text.append(num)
    ...

class CalculatorGUI(CalculatorFunctions):
    def gui_num_press(self, num):
        self.num_press(num)
        self.text_box.set(self.current_text)
    ...
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for putting the time in to help me out! :) Very grateful \$\endgroup\$ – Brian Sep 23 '18 at 22:57
  • \$\begingroup\$ @G.Sliepen, in my code, is the following algorithm at the start of the "CalculatorFunctions" class redundant? def __init__(self, root): self.root = root \$\endgroup\$ – Brian Sep 24 '18 at 7:15
  • \$\begingroup\$ @D.Austerus: it is. Just try removing self.root = root in CalculatorFunctions' initializer. \$\endgroup\$ – G. Sliepen Sep 24 '18 at 17:06
  • \$\begingroup\$ Hi! Sorry for bothering you again. Would it be okay to just have the add_input in my GUI class, or does it need to be handled in the function class as technically it's functionality? \$\endgroup\$ – Brian Sep 30 '18 at 22:21
  • \$\begingroup\$ I ask this because the only thing that the buttons do is update the tkinter entry field in the GUI class, so no real functionality behind them is required. \$\endgroup\$ – Brian Sep 30 '18 at 22:29

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.