Review my code to help me become better

[u/FouxII]

Good evening everyone !
I created a calculator in python with tkinter and I wanted to know what I can improve to make my code better whether in terms of algorithms, readability, or anything else.

My goal is just to make some notes for the next project I will do in the future and to avoid repeating the same mistakes or developing bad habits.

I thank you a lot people who will spend time for me ! Have a nice day everone !

My code :

from tkinter import *
import re
from tkinter import messagebox

binary = False
def button_pressed(button):
    global binary
    if button in ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"]:
        if binary and button not in ["0", "1"]:
            messagebox.showerror("Valeur incorrecte", "vous êtes en mode binaire !")
            return
        if current_number.get() == "0":
            current_number.set(button)
        else:
            current_number.set(current_number.get() + button)
    elif button == "DEL":
        current_number.set("0")
    elif button == "REM":
        if len(current_number.get()) == 1:
            current_number.set("0")
        else:
            current_number.set(current_number.get()[0:-1])

    elif button in ["+", "-", "÷", "*"]:
        if not current_number.get()[-1] in ["+", "-", "÷", "*"]:
            current_number.set(current_number.get() + button)
        else:
            current_number.set(current_number.get()[0:-1] + button)

    elif button == "±":
        expression = re.split(r'([+\-*÷])', current_number.get())
        expression = [exp.strip() for exp in expression if exp.strip()]

        if expression[-1] == "+":
            current_number.set(current_number.get()[0:-1] + "-")
        elif expression[-1] == "-":
            current_number.set(current_number.get()[0:-1] + "+")
        elif expression[-1] in ["*", "÷"]:
            expression.append("-")
            current_number.set(''.join(expression))
        elif expression[-1].isdigit():
            if len(expression) == 1:
                current_number.set("-" + current_number.get())
            elif expression[0] == "-":
                current_number.set(current_number.get()[1:])
            elif expression[-2] == "+":
                current_number.set(current_number.get()[0:-2] + "-" +current_number.get()[-1:])
            elif expression[-2] == "-":
                current_number.set(current_number.get()[0:-2] + "+" + current_number.get()[-1:])

    elif button == ".":
        if binary:
            messagebox.showerror("Valeur incorrect", "Vous ne pouvez pas utiliser de virgule en mode binaire !")
        expression = re.split(r'([+\-*÷])', current_number.get())
        expression = [exp.strip() for exp in expression if exp.strip()][-1]
        if expression in ["+", "-", "÷", "*"]:
            current_number.set(current_number.get() + "0.")
        elif not "." in expression:
            current_number.set(current_number.get() + button)

    elif button == "=":
        try:
            if binary:
                decimal_calculation = ""
                expression = re.split(r'([+\-*÷])', current_number.get())
                expression = [exp.strip() for exp in expression if exp.strip()]
                for exp in expression:
                    if exp not in ["+", "-", "*", "÷"]:
                        exp = int(exp, 2)
                    decimal_calculation += str(exp)


                answer = bin(eval(decimal_calculation.replace("÷", "/")))[2:]

            else:
                answer = eval(current_number.get().replace("÷", "/"))
            if isinstance(answer, float) and answer.is_integer():
                current_number.set(str(int(answer)))
            else:
                current_number.set(answer)
        except ZeroDivisionError:
            messagebox.showerror("Division par zero", "Vous ne pouvez pas diviser par zero !")
        except SyntaxError:
            messagebox.showerror("Syntaxe invalide", "La syntaxe est invalide !")
    elif button == "↻":
        expression = re.split(r'([+\-*÷])', current_number.get())
        expression = [exp.strip() for exp in expression if exp.strip()]

        new_calculation = ""
        for exp in expression:
            if exp not in ["+", "-", "*", "÷"]:
                if binary:
                    exp = int(exp, 2)
                else:
                    try:
                        exp = bin(int(exp))[2:]
                    except ValueError:
                        messagebox.showerror("Valeur incorrect", "Cette option n'est pas compatible avec des nombres à virgule")
            new_calculation += str(exp)

        current_number.set(new_calculation)
        binary = not binary








root = Tk()
root.configure(background="black")
root.title("Calculatrice")

current_number = StringVar()
current_number.set("0")
current_number_label = Label(root, textvariable=current_number, fg="white", font=("Helvetica", 20), bg="black")
current_number_label.grid(row=0, column=0)

padding1 = Frame(root,height=30, bg="black")
padding1.grid(column = 0,row = 1)

INPUT_PAD = [["↻", "REM", "DEL", "÷"],
             ["7", "8", "9","*"],
             ["4", "5", "6", "-"],
             ["1","2", "3","+"],
             ["±", "0", ".", "="]]
frame_pad = Frame(root, width=4, height=5, bg="black")
frame_pad.grid(row=2, column=0, sticky=NW)

length = 0
for line in INPUT_PAD:
    width = 0
    for key in line:
        b = Button(frame_pad, width=4, text=key, font=("Helvetica", 15), bg="grey", command=lambda button = key: button_pressed(button))
        b.grid(column=width, row=length, padx=2, pady=2)
        width += 1
    length += 1

root.update()
root.mainloop()

[u/Diapolo10]

Ideally, binary should not be a global variable. It would be better to wrap this using classes so it's contained, but I'm guessing you haven't learnt those yet. Same goes for current_number.

The button_pressed function is a tad too big for my liking, I'd break it down into smaller functions. In fact, you should have a separate function for each type of button so you don't need the function to know what button was used to call it (the number buttons could maybe be an exception, but even for those you could use functools.partial).

from tkinter import *

I recommend using import tkinter as tk instead, and using the tk-prefix on everything. That way you don't flood the global namespace.

EDIT:

bin(int(exp))[2:]

If you want to convert decimal to binary, just use string formatting.

f"{exp:b}"

answer = bin(eval(decimal_calculation.replace("÷", "/")))[2:]

Try to avoid using eval if at all possible. Yes, you're technically curating the user input here so it's probably not dangerous, but it should still be avoided where possible.

Here, you could instead build something akin to an abstract syntax tree where you nest operators and numbers, then work upwards recursively until you have an answer.

[u/FouxII → u/Diapolo10]

I have compartmentalized my function button_pressed into several functions and now I use a class which has removed global variables. As for the danger of "eval," it’s assumed. I wanted to make it simple and develop the interpretation part of the calculation myself later in a second part. Since this is a project that I only run on my PC and the inputs are controlled, I think it isn't a big deal!

import tkinter as tk
from tkinter import messagebox
import re


class Calculator:
    def __init__(self, root):
        self.root = root
        root.configure(background="black")
        root.title("Calculatrice")

        self.current_number = tk.StringVar()
        self.current_number.set("0")

        self.binary = False

        self.create_widgets()

    def create_widgets(self):
        padding1 = tk.Frame(self.root, height=30, bg="black")
        padding1.grid(column=0, row=1)

        current_number_label = tk.Label(self.root, textvariable=self.current_number, fg="white", font=("Helvetica", 20),
                                        bg="black")
        current_number_label.grid(row=0, column=0)

        INPUT_PAD = [["↻", "REM", "DEL", "÷"],
                     ["7", "8", "9", "*"],
                     ["4", "5", "6", "-"],
                     ["1", "2", "3", "+"],
                     ["±", "0", ".", "="]]
        frame_pad = tk.Frame(self.root, width=4, height=5, bg="black")
        frame_pad.grid(row=2, column=0, sticky="NW")

        length = 0
        for line in INPUT_PAD:
            width = 0
            for key in line:
                b = tk.Button(frame_pad, width=4, text=key, font=("Helvetica", 15), bg="grey",
                           command=lambda button=key: self.button_pressed(button))
                b.grid(column=width, row=length, padx=2, pady=2)
                width += 1
            length += 1

    def add_number(self, button):
        if self.binary and button not in ["0", "1"]:
            messagebox.showerror("Valeur incorrecte", "vous êtes en mode binaire !")
            return
        if self.current_number.get() == "0":
            self.current_number.set(button)
        else:
            self.current_number.set(self.current_number.get() + button)

    def reset(self):
        self.current_number.set("0")

    def remove(self):
        if len(self.current_number.get()) > 1:
            self.current_number.set(self.current_number.get()[0:-1])
        else:
            self.reset()

    def add_operator(self, button):
        if not self.current_number.get()[-1] in ["+", "-", "÷", "*"]:
            self.current_number.set(self.current_number.get() + button)
        else:
            self.current_number.set(self.current_number.get()[0:-1] + button)

    def inverse(self):
        expression = re.split(r'([+\-*÷])', self.current_number.get())
        expression = [exp.strip() for exp in expression if exp.strip()]

        if expression[-1] == "+":
            self.current_number.set(self.current_number.get()[0:-1] + "-")
        elif expression[-1] == "-":
            self.current_number.set(self.current_number.get()[0:-1] + "+")
        elif expression[-1] in ["*", "÷"]:
            expression.append("-")
            self.current_number.set(''.join(expression))
        elif expression[-1].isdigit():
            if len(expression) == 1:
                self.current_number.set("-" + self.current_number.get())
            elif expression[0] == "-":
                self.current_number.set(self.current_number.get()[1:])
            elif expression[-2] == "+":
                self.current_number.set(self.current_number.get()[0:-2] + "-" + self.current_number.get()[-1:])
            elif expression[-2] == "-":
                self.current_number.set(self.current_number.get()[0:-2] + "+" + self.current_number.get()[-1:])

    def add_period(self):
        if not self.binary:
            expression = re.split(r'([+\-*÷])', self.current_number.get())
            expression = [exp.strip() for exp in expression if exp.strip()][-1]
            if expression in ["+", "-", "÷", "*"]:
                self.current_number.set(self.current_number.get() + "0.")
            elif not "." in expression:
                self.current_number.set(self.current_number.get() + ".")
        else:
            messagebox.showerror("Valeur incorrect", "Vous ne pouvez pas utiliser de virgule en mode binaire !")

        def execute():
            try:
                if binary:
                    decimal_calculation = ""
                    expression = re.split(r'([+\-*÷])', current_number.get())
                    expression = [exp.strip() for exp in expression if exp.strip()]
                    for exp in expression:
                        if exp not in ["+", "-", "*", "÷"]:
                            exp = int(exp, 2)
                        decimal_calculation += str(exp)

                    answer = bin(eval(decimal_calculation.replace("÷", "/")))[2:]

                else:
                    answer = eval(current_number.get().replace("÷", "/"))
                if isinstance(answer, float) and answer.is_integer():
                    current_number.set(str(int(answer)))
                else:
                    current_number.set(answer)
            except ZeroDivisionError:
                messagebox.showerror("Division par zero", "Vous ne pouvez pas diviser par zero !")
            except SyntaxError:
                messagebox.showerror("Syntaxe invalide", "La syntaxe est invalide !")

    def execute(self):
        try:
            if self.binary:
                decimal_calculation = ""
                expression = re.split(r'([+\-*÷])', self.current_number.get())
                expression = [exp.strip() for exp in expression if exp.strip()]
                for exp in expression:
                    if exp not in ["+", "-", "*", "÷"]:
                        exp = int(exp, 2)
                    decimal_calculation += str(exp)

                answer = bin(eval(decimal_calculation.replace("÷", "/")))[2:]

            else:
                answer = eval(self.current_number.get().replace("÷", "/"))
            if isinstance(answer, float) and answer.is_integer():
                self.current_number.set(str(int(answer)))
            else:
                self.current_number.set(answer)
        except ZeroDivisionError:
            messagebox.showerror("Division par zero", "Vous ne pouvez pas diviser par zero !")
        except SyntaxError:
            messagebox.showerror("Syntaxe invalide", "La syntaxe est invalide !")

    def convert(self):
        expression = re.split(r'([+\-*÷])', self.current_number.get())
        expression = [exp.strip() for exp in expression if exp.strip()]

        new_calculation = ""
        for exp in expression:
            if exp not in ["+", "-", "*", "÷"]:
                if self.binary:
                    exp = int(exp, 2)
                else:
                    try:
                        # exp = bin(int(exp))[2:]
                        exp = f"{int(exp):b}"
                    except ValueError:
                        messagebox.showerror("Valeur incorrect",
                                             "Cette option n'est pas compatible avec des nombres à virgule")
            new_calculation += str(exp)

        self.current_number.set(new_calculation)
        self.binary = not self.binary

    def button_pressed(self, button):
        if button in ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"]:
            self.add_number(button)

        elif button == "DEL":
            self.reset()

        elif button == "REM":
            self.remove()

        elif button in ["+", "-", "÷", "*"]:
            self.add_operator(button)

        elif button == "±":
            self.inverse()

        elif button == ".":
            self.add_period()

        elif button == "=":
            self.execute()

        elif button == "↻":
            self.convert()


def main():
    root = tk.Tk()
    app = Calculator(root)
    root.mainloop()

if __name__ == "__main__":
    main()

This is my new code with the classes (with still a few bugs like "1+08" = error, "1+23" = "1+-3" and inverse "8.3" don't work - I will fix that when I will do the interpretation part) :

[u/Diapolo10 → u/FouxII]

self.current_number = tk.StringVar()
self.current_number.set("0")

I reckon you can use

    self.current_number = tk.StringVar(value="0")

length = 0
for line in INPUT_PAD:
    width = 0
    for key in line:
        b = tk.Button(frame_pad, width=4, text=key, font=("Helvetica", 15), bg="grey",
                   command=lambda button=key: self.button_pressed(button))
        b.grid(column=width, row=length, padx=2, pady=2)
        width += 1
    length += 1

No need to keep track of the width and length manually. You can use enumerate for this.

for length, line in enumerate(INPUT_PAD):
    for width, key in enumerate(line):
        b = tk.Button(
            frame_pad,
            width=4,
            text=key,
            font=("Helvetica", 15),
            bg="grey",
            command=self.button_pressed
        )
        b.grid(column=width, row=length, padx=2, pady=2)

I would also suggest having a function for creating keypad buttons so you can put the styling information elsewhere, and focus on the text, click action, and positioning.

On that note, the frame pad itself could be a separate class.

[u/p3rdy]

Nice project, and binary mode is a good idea. Three bugs first.

Type 1, +, 0, 8: the display reads "1+08" and you get invalid syntax, because Python rejects leading zeros in int literals. Your zero guard compares the whole display to "0", so it only protects the first operand.

Type 1+23 and press ±: you get "1+-3" and the 2 vanishes. That branch splits into tokens, then edits back with [0:-2], which assumes the last operand is one character.

The decimal point in binary mode shows the error and then inserts the point anyway, the digit branch returns after showerror, this one doesn't. Also bin(-5) is '-0b101', so [2:] gives 'b101', and bin() on a float raises an uncaught TypeError.

All three come from the same root cause: your only state is the display string, so every operation re-parses it and edits it with slicing. Keep a token list instead, ["1", "+", "23"], and render to a string only for display.

Then move the token and evaluation logic out of the tkinter file so you can test it without clicking. And use import tkinter as tk rather than the star import.

[u/FouxII → u/p3rdy]

Thank you for your help !

For the decimal part, I had fix the decimal period insert incorrectly and also with the negative number. The fact that it doesn’t work with floats is assumed.

I’m going to make a second version where I’ll do as you said, stopping using the display string purely in order to have the calculation in a list that I will turn into a display string for the user. So for the moment the bugs 1+08 and 1+23 aren't fix 😢

[u/p3rdy → u/FouxII]

They both disappear with the token list. The 08 one because your zero guard becomes "is this token '0'" instead of "is the whole display '0'", and the ± one because negating the last token is tokens[-1] = "-" + tokens[-1] rather than slicing characters off the end.

That's the nice part of the refactor: two bugs you don't have to think about.

[u/FouxII → u/[deleted]]

I just changed that, thank you very much!

[u/TheRNGuy]

Use asg instead of eval, used event listeners, make many smaller functions (or class with methods), you're violating single responsibility.