6
\$\begingroup\$

Edit: New version at Python 3 Tkinter Calculator - follow-up

New status: I´ve refactored the code trying to follow the recommendations from the guys who answered this question. The new version is on the link above.

I'm a beginner developer and I chose Python as my initial language to learn. This is my very first project: a calculator using Tkinter for GUI.

I've tried to apply some OOP and modules approach, as an attempt to make it like a real job, instead of simply putting it all on a single file or procedural mode.

I need some feedback about module naming and organization, class naming and organization, PEP-8 style, and structure in general.

Module: window.py

This should be the main module, but I´m facing some circular import issue that I can't figure why yet.

import tkinter as tk
import frame_display
import frame_botoes


root = tk.Tk()
root.geometry("640x640")
visor = frame_display.DisplayContainer(root)
numeros = frame_botoes.ButtonsContainer(root)
root.mainloop()

Module: calculadora.py

I made some sort of workaround and the programs runs here:

agregator = ""
result = ""


def pressNumber(num):

    global agregator
    global result

    agregator = agregator + str(num)
    result = agregator

    window.visor.updateTextDisplay(result)


def pressEqual():

    try:
        global agregator

        total = str(eval(agregator))

        window.visor.updateTextDisplay(total)
        agregator = ""

    except ZeroDivisionError:

        window.visor.updateTextDisplay("Erro: Divisão por zero")
        agregator = ""

    except:

        window.visor.updateTextDisplay("Error")
        agregator = ""


def pressClear():

    global agregator
    agregator = ""
    window.visor.updateTextDisplay("Clear")



import window

I tried to use separate modules and classes as an attempt to use good practices.

Module: frame_display.py

import tkinter as tk
from tkinter import Frame
from tkinter import StringVar

class DisplayContainer(Frame):


    def __init__(self, root):
        Frame.__init__(self, root)
        self.parent = root
        self.configure(bg="cyan", height=5)

        self.text_display = StringVar()

        # Layout DisplayContainer
        self.grid(row=0 , column=0 , sticky="nwe")
        self.parent.columnconfigure(0, weight=1)

        # Call DisplayContainer widgets creation
        self.createWidgets()



    # Create widgets for DisplayContainer
    def createWidgets(self):

        self.label_display = tk.Label(self)
        self.label_display.configure(textvariable=self.text_display)
        self.label_display["font"] = 15
        self.label_display["bg"] = "#bebebe"
        self.label_display["relief"] = "groove"
        self.label_display["bd"] = 5
        self.label_display["height"] = 5


    # Layout widgets for DisplayContainer
        self.label_display.grid(row=0 , column=0 , sticky="nswe")
        self.columnconfigure(0, weight=1)



    def updateTextDisplay(self, text):

        self.text_display.set(text)

Module: frame_botoes.py

import tkinter as tk
from tkinter import Frame
import calculadora


class ButtonsContainer(Frame):

    def __init__(self , root):
        Frame.__init__(self, root)
        self.parent = root
        self.configure(bg="yellow")
        self.parent.bind("<Key>", self.keyHandler)
        self.parent.bind("<Return>", self.returnKeyHandler)


        # Layout ButtonsContainer
        self.grid(row=1 , column=0 , sticky ="nsew")
        self.parent.rowconfigure(1, weight=1)
        self.parent.columnconfigure(0, weight=1)

        # Call ButtonsContainer widgets creation
        self.createWidgets()


    # Create widgets for ButtonsContainer
    def createWidgets(self):

        button_padx = 15
        button_pady = 15        

        self.button_1 = tk.Button(self, text="1", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(1))
        self.button_2 = tk.Button(self, text="2", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(2))
        self.button_3 = tk.Button(self, text="3", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(3))
        self.button_4 = tk.Button(self, text="4", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(4))
        self.button_5 = tk.Button(self, text="5", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(5))
        self.button_6 = tk.Button(self, text="6", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(6))
        self.button_7 = tk.Button(self, text="7", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(7))
        self.button_8 = tk.Button(self, text="8", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(8))
        self.button_9 = tk.Button(self, text="9", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(9))
        self.button_0 = tk.Button(self, text="0", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(0))

        self.button_open_parens = tk.Button(self, text="(", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("("))
        self.button_close_parens = tk.Button(self, text=")", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(")"))


        self.button_dot = tk.Button(self, text=".", padx= button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("."))
        self.button_plus = tk.Button(self, text="+", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("+"))
        self.button_minus = tk.Button(self, text="-", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("-"))
        self.button_multiply = tk.Button(self, text="*", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("*"))
        self.button_divide = tk.Button(self, text="/", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("/"))
        self.button_equal = tk.Button(self, text="=", padx=button_padx, pady=button_pady, command=calculadora.pressEqual)
        self.button_clear = tk.Button(self, text="CLEAR", padx=button_padx, pady=button_pady, command=calculadora.pressClear)

    # Layout widgets for ButtonsContainer
        self.button_1.grid(row=0, column=0, sticky="nswe")
        self.button_2.grid(row=0, column=1, sticky="nswe")
        self.button_3.grid(row=0, column = 2, sticky="nswe")
        self.button_4.grid(row=1, column=0, sticky="nswe")
        self.button_5.grid(row=1, column=1, sticky="nswe")
        self.button_6.grid(row=1, column=2, sticky="nswe")
        self.button_7.grid(row=2, column=0, sticky="nswe")
        self.button_8.grid(row=2, column=1, sticky="nswe")
        self.button_9.grid(row=2, column=2, sticky="nswe")

        self.button_open_parens.grid(row=3, column=0, sticky="nswe")
        self.button_close_parens.grid(row=3, column=2, sticky="nswe")

        self.button_0.grid(row=3, column=1, sticky="nswe")
        self.button_dot.grid(row=4, column=2, sticky="nswe")
        self.button_plus.grid(row=0 , column=3, sticky="nswe")
        self.button_minus.grid(row=1 , column=3, sticky="nswe")
        self.button_multiply.grid(row=2 , column=3, sticky="nswe")
        self.button_divide.grid(row=3 , column=3, sticky="nswe")

        self.button_equal.grid(row=4 , column=3, sticky="nswe")
        self.button_clear.grid(row=4 , columnspan=2, sticky="nswe")

        for x in range(0,5):
            self.rowconfigure(x, weight=1)

        for i in range(0, 4):
            self.columnconfigure(i, weight=1)

    #Bind keyboard events
    def keyHandler(self, event):
        calculadora.pressNumber(event.char)

    #Bind Return key
    def returnKeyHandler(self, event):
        calculadora.pressEqual()
\$\endgroup\$
4
\$\begingroup\$

Disclaimer: you should not be using eval that said I am not going to be removing it from the code as you can work out the correct options on your own. I will be reviewing the overall code issues. Just know eval is evil! :D

Ok so quick answer to fix the main problem is to add a new argument to all functions in calculadora.py lets call this argument window because we are passing the the root window to each function.

Then you need to build the root window as a class with class attributes. This way your functions in calculadora can actually update the the fields.

Once we changed those 2 parts we need to pass that window to those functions from the frame_botoes.py buttons so we will update those buttons as well.

Updated window.py:

import tkinter as tk import frame_display import frame_botoes

class Main(tk.Tk):
    def __init__(self):
        super().__init__()
        self.geometry("640x640")
        self.visor = frame_display.DisplayContainer(self)
        self.numeros = frame_botoes.ButtonsContainer(self)

Main().mainloop()

Updated calculadora.py:

agregator = ""
result = ""


def pressNumber(num, window):
    global agregator
    global result
    agregator = agregator + str(num)
    result = agregator
    window.visor.updateTextDisplay(result)


def pressEqual(window):
    try:
        global agregator
        total = str(eval(agregator))
        window.visor.updateTextDisplay(total)
        agregator = ""
    except ZeroDivisionError:
        window.visor.updateTextDisplay("Erro: Divisão por zero")
        agregator = ""
    except:
        window.visor.updateTextDisplay("Error")
        agregator = ""

def pressClear(window):
    global agregator
    agregator = ""
    window.visor.updateTextDisplay("Clear")

Updated frame_botoes.py:

import tkinter as tk
from tkinter import Frame
import calculadora


class ButtonsContainer(Frame):

    def __init__(self , root):
        Frame.__init__(self, root)
        self.parent = root
        self.configure(bg="yellow")
        self.parent.bind("<Key>", self.keyHandler)
        self.parent.bind("<Return>", self.returnKeyHandler)


        # Layout ButtonsContainer
        self.grid(row=1 , column=0 , sticky ="nsew")
        self.parent.rowconfigure(1, weight=1)
        self.parent.columnconfigure(0, weight=1)

        # Call ButtonsContainer widgets creation
        self.createWidgets()


    # Create widgets for ButtonsContainer
    def createWidgets(self):

        button_padx = 15
        button_pady = 15

        self.button_1 = tk.Button(self, text="1", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(1, self.parent))
        self.button_2 = tk.Button(self, text="2", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(2, self.parent))
        self.button_3 = tk.Button(self, text="3", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(3, self.parent))
        self.button_4 = tk.Button(self, text="4", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(4, self.parent))
        self.button_5 = tk.Button(self, text="5", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(5, self.parent))
        self.button_6 = tk.Button(self, text="6", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(6, self.parent))
        self.button_7 = tk.Button(self, text="7", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(7, self.parent))
        self.button_8 = tk.Button(self, text="8", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(8, self.parent))
        self.button_9 = tk.Button(self, text="9", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(9, self.parent))
        self.button_0 = tk.Button(self, text="0", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(0, self.parent))

        self.button_open_parens = tk.Button(self, text="(", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("(", self.parent))
        self.button_close_parens = tk.Button(self, text=")", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(")", self.parent))


        self.button_dot = tk.Button(self, text=".", padx= button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(".", self.parent))
        self.button_plus = tk.Button(self, text="+", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("+", self.parent))
        self.button_minus = tk.Button(self, text="-", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("-", self.parent))
        self.button_multiply = tk.Button(self, text="*", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("*", self.parent))
        self.button_divide = tk.Button(self, text="/", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber("/", self.parent))
        self.button_equal = tk.Button(self, text="=", padx=button_padx, pady=button_pady, command=calculadora.pressEqual(self.parent))
        self.button_clear = tk.Button(self, text="CLEAR", padx=button_padx, pady=button_pady, command=calculadora.pressClear(self.parent))

    # Layout widgets for ButtonsContainer
        self.button_1.grid(row=0, column=0, sticky="nswe")
        self.button_2.grid(row=0, column=1, sticky="nswe")
        self.button_3.grid(row=0, column = 2, sticky="nswe")
        self.button_4.grid(row=1, column=0, sticky="nswe")
        self.button_5.grid(row=1, column=1, sticky="nswe")
        self.button_6.grid(row=1, column=2, sticky="nswe")
        self.button_7.grid(row=2, column=0, sticky="nswe")
        self.button_8.grid(row=2, column=1, sticky="nswe")
        self.button_9.grid(row=2, column=2, sticky="nswe")

        self.button_open_parens.grid(row=3, column=0, sticky="nswe")
        self.button_close_parens.grid(row=3, column=2, sticky="nswe")

        self.button_0.grid(row=3, column=1, sticky="nswe")
        self.button_dot.grid(row=4, column=2, sticky="nswe")
        self.button_plus.grid(row=0 , column=3, sticky="nswe")
        self.button_minus.grid(row=1 , column=3, sticky="nswe")
        self.button_multiply.grid(row=2 , column=3, sticky="nswe")
        self.button_divide.grid(row=3 , column=3, sticky="nswe")

        self.button_equal.grid(row=4 , column=3, sticky="nswe")
        self.button_clear.grid(row=4 , columnspan=2, sticky="nswe")

        for x in range(0,5):
            self.rowconfigure(x, weight=1)

        for i in range(0, 4):
            self.columnconfigure(i, weight=1)

    #Bind keyboard events
    def keyHandler(self, event):
        calculadora.pressNumber(event.char, self.parent)

    #Bind Return key
    def returnKeyHandler(self, event):
        calculadora.pressEqual()

Now that the quick fix is dealt with its time to go in depth as to the other formatting issues and PEP8 changes we should make.

I will keep each one of your files separate but honestly I do not think it is necessary to separate the main window file from the frame data.

1st: I would like to address is PEP8 standards. Personally I think you should use CamelCase for Class names and lowercase_with_underscores for functions/methods.

2nd: Lets look at your buttons in frame_botoes. You should probably be generating your buttons with loops so we can keep the code short and clean. I have 2 examples here. One uses simple counting for the layout and the other uses a list with grid values for placement.

3rd: We should avoid using global so lets convert your calculadora functions into a class that we use with class attribute to manage the aggregator.

4th: You only need self. prefix for a variable that will be changed later in the class outside of the method it is generated in. So for all your buttons we can remove this prefix. At the same time we don't need to name them as we are generating them from a loop. Naming doesn't help us here as the layout is simple enough and we are not changing the buttons later.

5th: We do not need from tkinter import Frame as you are already using import tkinter as tk so we can simply call tk.Frame or any other widget for that matter where it is needed.

With some general clean up and the things I mentioned above here is your modified code:

New window.py:

import tkinter as tk
import frame_display
import frame_botoes


class Main(tk.Tk):
    def __init__(self):
        super().__init__()
        self.geometry("640x640")
        self.columnconfigure(0, weight=1)
        self.rowconfigure(1, weight=1)
        self.visor = frame_display.DisplayContainer().grid(row=0, column=0, sticky="new")
        self.numeros = frame_botoes.ButtonsContainer().grid(row=1, column=0, sticky="nsew")

Main().mainloop()

New calculadora.py:

class Press:
    def __init__(self, master):
        self.master = master
        self.aggregator = ''

    def num(self, n):
        self.aggregator += str(n)
        self.master.visor.update_text_display(self.aggregator)

    def equal(self, _):
        try:
            total = str(eval(self.aggregator))
            self.aggregator = ''
            self.master.visor.text_display.set(total)
        except ZeroDivisionError:
            self.master.visor.text_display.set("Error: Divisão por zero")
        except:
            self.master.visor.text_display.set("Unexpected error")
            raise

    def clear(self):
        self.master.visor.text_display.set("Clear")

New frame_display.py:

import tkinter as tk


class DisplayContainer(tk.Frame):
    def __init__(self):
        super().__init__()
        self.configure(bg="cyan", height=5)
        self.columnconfigure(0, weight=1)
        self.txt = tk.StringVar()

        label_display = tk.Label(self, textvariable=self.txt, font=15, bg="#bebebe", relief="groove", bd=5, height=5)
        label_display.grid(row=0, column=0, sticky="nsew")

    def update_text_display(self, text):
        self.text_display.set(text)

New frame_botoes.py:

import tkinter as tk
import calculadora


class ButtonsContainer(tk.Frame):
    def __init__(self):
        super().__init__()
        self.configure(bg="yellow")
        self.screen = calculadora.Press(self.master)
        self.master.bind("<Key>", self.key_handler)
        self.master.bind("<Return>", self.screen.equal)
        for x in range(0, 5):
            self.rowconfigure(x, weight=1)
            if x < 4:
                self.columnconfigure(x, weight=1)

        pad = 15
        r = 0
        c = 0
        for i in range(10):
            if i == 0:
                tk.Button(self, text=i, padx=pad, pady=pad,
                          command=lambda n=i: self.screen.num(n)).grid(row=3, column=1, sticky="nswe")
            else:
                tk.Button(self, text=i, padx=pad, pady=pad,
                          command=lambda n=i: self.screen.num(n)).grid(row=r, column=c, sticky="nswe")
                if c == 2:
                    c = 0
                    r += 1
                else:
                    c += 1

        for i in [["-", 1, 3], ["*", 2, 3], ["/", 3, 3], ["(", 3, 0],
                  [")", 3, 2], [".", 4, 2], ["+", 0, 3], ["=", 4, 3], ["CLEAR", 4, 0]]:
            if i[0] == 'CLEAR':
                tk.Button(self, text=i[0], padx=pad, pady=pad,
                          command=self.screen.clear).grid(row=i[1], column=i[2], columnspan=2, sticky="nsew")
            elif i[0] == '=':
                tk.Button(self, text=i[0], padx=pad, pady=pad,
                          command=self.screen.equal).grid(row=i[1], column=i[2], sticky="nsew")
            else:
                tk.Button(self, text=i[0], padx=pad, pady=pad,
                          command=lambda v=i[0]: self.screen.num(v)).grid(row=i[1], column=i[2], sticky="nsew")

    def key_handler(self, event):
        self.screen.num(event.char)

If you have any questions let me know :D

Just for fun here is how I would have build this calc. Its a small enough program I think most if not all is fine in a single class. Also by placing everything in a single class we can avoid a lot of the back and forth going on and keep our code simple. By doing this we took your roughly 180+ lines of code and reduced them to around 80+ lines of code.

My example:

import tkinter as tk


class Main(tk.Tk):
    def __init__(self):
        super().__init__()
        self.geometry("640x640")
        self.columnconfigure(0, weight=1)
        self.rowconfigure(1, weight=1)
        self.aggregator = ''
        self.txt = tk.StringVar()
        self.bind("<Key>", self.key_handler)
        self.bind("<Return>", self.equal)

        dis_frame = tk.Frame(self)
        dis_frame.grid(row=0, column=0, sticky="new")
        btn_frame = tk.Frame(self)
        btn_frame.grid(row=1, column=0, sticky="nsew")

        dis_frame.configure(bg="cyan", height=5)
        dis_frame.columnconfigure(0, weight=1)

        for x in range(0, 5):
            btn_frame.rowconfigure(x, weight=1)
            if x < 4:
                btn_frame.columnconfigure(x, weight=1)

        self.display = tk.Label(dis_frame, textvariable=self.txt, font=15,
                                bg="#bebebe", relief="groove", bd=5, height=5)
        self.display.grid(row=0, column=0, sticky="nsew")

        pad = 15
        r = 0
        c = 0
        for i in range(10):
            if i == 0:
                tk.Button(btn_frame, text=i, padx=pad, pady=pad,
                          command=lambda n=i: self.num(n)).grid(row=3, column=1, sticky="nswe")
            else:
                tk.Button(btn_frame, text=i, padx=pad, pady=pad,
                          command=lambda n=i: self.num(n)).grid(row=r, column=c, sticky="nswe")
                if c == 2:
                    c = 0
                    r += 1
                else:
                    c += 1

        for i in [["-", 1, 3], ["*", 2, 3], ["/", 3, 3], ["(", 3, 0],
                  [")", 3, 2], [".", 4, 2], ["+", 0, 3], ["=", 4, 3], ["CLEAR", 4, 0]]:
            if i[0] == 'CLEAR':
                tk.Button(btn_frame, text=i[0], padx=pad, pady=pad,
                          command=self.clear).grid(row=i[1], column=i[2], columnspan=2, sticky="nsew")
            elif i[0] == '=':
                tk.Button(btn_frame, text=i[0], padx=pad, pady=pad,
                          command=self.equal).grid(row=i[1], column=i[2], sticky="nsew")
            else:
                tk.Button(btn_frame, text=i[0], padx=pad, pady=pad,
                          command=lambda v=i[0]: self.num(v)).grid(row=i[1], column=i[2], sticky="nsew")

    def key_handler(self, event):
        self.num(event.char)

    def num(self, n):
        self.aggregator += str(n)
        self.txt.set(self.aggregator)

    def equal(self, event=None):
        try:
            total = str(eval(self.aggregator))
            self.txt.set(total)
            self.aggregator = total
        except ZeroDivisionError:
            self.txt.set("Error: Divisão por zero")
        except:
            self.txt.set("Unexpected error")
            raise

    def clear(self):
        self.txt.set("Clear")
        self.aggregator = ''

Main().mainloop()
\$\endgroup\$
  • 1
    \$\begingroup\$ @BrunoLemos it is super convenient and that is why it is evil :D It can make your code vulnerable to attack. \$\endgroup\$ – Mike - SMT May 7 at 20:44
  • 1
    \$\begingroup\$ @BrunoLemos yes it is vulnerable to injection, it is also slow compared to better methods and it can make debugging issue difficult. \$\endgroup\$ – Mike - SMT May 8 at 13:54
  • 1
    \$\begingroup\$ @BrunoLemos it doesnt hurt to have the display separate. You may want to keep them separate just in case you want to make a transforming calculator. Like switching between normal and scientific mode. If you have no need to move the display or other widgets later then the extra frame is not needed but doesnt really cause any issues here. \$\endgroup\$ – Mike - SMT May 8 at 14:25
  • 1
    \$\begingroup\$ @BrunoLemos You don't want to over write your code. If there is not a good reason to build a function all you are doing is adding unwanted complexity. Just try to keep things short and readable. the PEP8 guidelines is something you should read. It will provide some good references. \$\endgroup\$ – Mike - SMT May 8 at 19:43
  • 1
    \$\begingroup\$ @BrunoLemos I would. By changing the code you end up changing what the review would look like. \$\endgroup\$ – Mike - SMT May 30 at 19:03
1
\$\begingroup\$

Welcome to CodeReview! And welcome to coding! Publishing and reviewing your code is one of the best ways to get better at coding. And we're going to make you better, no matter how much it hurts. ;-)

First, congratulations! You've written a fair amount of code in a single project, and you managed to produce a somewhat complex app, with graphics, alternative input, event handling, etc. This is a pretty ambitious first project.

I have some suggestions about the organization and structure, and the coding style.

Organization and Structure

Modules

You have too many modules. A good starting rule for breaking code into different modules is this: always put everything in one file. By the time you need to break that rule, you'll know what and how and when to break it. For now, you don't need to break it -- just put everything into calculadora.py.

On a side note, the fact that you were importing a module at the bottom of one of your files instead of at the top is a sign that you should merge the modules together if possible. Needing to do that kind of thing should set off your internal alarms that something is wrong.

Functions

There are three good reasons to create a function: (1) to standardize operations that you perform more than one time; (2) to "abstract away" low-level operations to a separate layer; (3) to isolate a valuable operation for re-use.

Reason #3 is generally rare. But you aren't doing enough of #1 and #2. Consider this:

root = tk.Tk()
root.geometry("640x640")
visor = frame_display.DisplayContainer(root)
numeros = frame_botoes.ButtonsContainer(root)
root.mainloop()

The first four lines of that block "create the application". The fifth line "runs the application". You might put that in a class, if you have learned classes yet. Otherwise, just put that into two functions:

app = create_application()
run_application(app)

Or consider this code:

self.button_1 = tk.Button(self, text="1", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(1))
self.button_2 = tk.Button(self, text="2", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(2))
self.button_3 = tk.Button(self, text="3", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(3))
self.button_4 = tk.Button(self, text="4", padx=button_padx, pady=button_pady, command=lambda: calculadora.pressNumber(4))

There are more lines of this (5..0), but these four are enough to make the point: this is a repeated operation and could be a function!

What's more, these lines appear lower down:

self.button_1.grid(row=0, column=0, sticky="nswe")
self.button_2.grid(row=0, column=1, sticky="nswe")
self.button_3.grid(row=0, column = 2, sticky="nswe")
self.button_4.grid(row=1, column=0, sticky="nswe")

These lines are "in parallel" with the button creation lines above. So they could be part of the same method. Let's try:

def make_button(self, text, row, column):
    new_button = tk.Button(self, text=text, padx=self.BUTTON_PADX, pady=self.BUTTON_PADY,
                           command=lambda: press_button(text))
    new_button.grid(row=row, column=column, sticky=self.BUTTON_STICKY)
    self.buttons.append(new_button)

Then you could replace a lot of that text with something like:

self.make_button('1', 0, 0)
self.make_button('2', 0, 1)
self.make_button('3', 0, 2)
self.make_button('4', 1, 0)
self.make_button('5', 1, 1)

Pro-tip: Visual Organization

When you're writing code, it's important to communicate to the next guy what you are trying to do. Sometimes the next guy is "future you" who will be reading this a year from now. Sometimes the next guy is another junior developer that will take over your project when you get promoted. But there's almost always going to be a "next guy" and your code is really written for him or her, more than for the compiler.

One trick you can use is to visually organize things. Then you will write code that "decodes" the visual organization. It's worth spending 15 minutes to make life easy for yourself, or for the next guy. Things like putting configuration into a docstring and parsing the string instead of putting 10 different values in separate quotation marks.

You could do something like this:

button_layout = """
    1 2 3 +
    4 5 6 -
    7 8 9 *
    ( 0 ) /
    CCC . =
""".strip('\n').splitlines()

for row, line in enumerate(button_layout):
    extra_col = 0

    for col, ch in enumerate(line.split()):
        if ch == 'CCC':
            self.make_clear_button(row, col)
            extra_col = 1
        else:
            self.make_button(ch, row, col + extra_col)

self.num_rows = row + 1
self.num_cols = col + 1

This would let you visually arrange the keys in different shapes, and the code would "figure out" where to put the buttons, and how many rows and columns were present.

Note that doing this provides absolutely no value to your program. The buttons are going to be created no matter what. But it lets you explore different shapes for the window just by moving characters around, and it lets the "next guy" see and understand how the buttons are arranged in a way that 30+ lines of row=3, col=0 ... row=4, col=2 just cannot do.

Coding Style

PEP-8

The official Python coding style document is PEP8. You may have learned a different style from reading code written in Java or some other language. But you're going to be considered "out of spec" if you deviate from PEP-8.

That said, PEP-8 contains a lot of good advice for beginning coders. It's a fairly well-reasoned document, with just a few things that are totally wrong (IMO). But I ignore those things in favor of having a common standard, and you should too. Conform!

To quickly summarize:

  • Use snake_case for all names except classes. Classes are PascalCase just like every other language.

  • Use ALL_CAPS for "constants". If a class or object has an all-caps attribute, then it's a class constant or an object constant. If a module has an all-caps variable at the top, it's a module constant. This despite the fact of math.pi and math.e and math.tau. "Do as we say, not as we do." :-)

Importing names

You can import names from a module using from module import name. Or you can import a module and refer to module.name instead. You should choose the style based on clarity and frequency of use.

For some reason, you do this:

from tkinter import Frame
from tkinter import StringVar

Then you make use of Frame and StringVar 4 + 1 times, respectively. On the other hand, you don't import Button but refer to tk.Button 25 times!

I suggest that your default should be to not import any names explicitly, and to prefer to spell out the module.name form. It's okay to abbreviate the module name, which you do (tkinter -> tk):

import tkinter as tk

class DisplayContainer(tk.Frame):
    def __init__(...):
        ...
        self.text_display = tk.StringVar()

Then if you find yourself repeating tk.Button 25 times (which you should not: see the note about functions above) you can do an additional import of that one name. Or you could just stash it in a local variable if every occurrence is within the same function:

button = tk.Button
this = button(...)
that = button(...)
another = button(...)

Comments

If your comment says in English (or Portuguese!) the same thing the code says in Python, delete the comment. Don't do this:

# Call ButtonsContainer widgets creation
self.createWidgets()

Comments should explain:

  • Details that come from the problem domain

    # Per tax code, deduction does not apply if children > 12 yrs old
    
  • Code constructs that are particularly dense or complex (particularly: nested comprehensions in Python)

    # Flatten list-of-lists, ignoring short names.
    all_planets = [planet for sublist in planets for planet in sublist if len(planet) < 6]
    
  • Things that are not obvious from the code, including side effects.

    # Note: this call flushes stdout
    
  • Things that are missing.

    # Note: NOT calling end_row() here!
    
\$\endgroup\$
  • \$\begingroup\$ thanks!!! I´ll refactor my code best as possible. Some tips are still a bit hard for me, as the button layout block you suggested. \$\endgroup\$ – Bruno Lemos May 8 at 14:41
  • 1
    \$\begingroup\$ @BrunoLemos I would say no, because separating create_widgets and arrange_widgets doesn't provide much value. Do you need to have creation and arrangement separate? I don't thing so. But doing that means you have two functions, each of which probably has 25 lines, since I think there are 25 buttons? That means 50 lines of code and the information about buttons is in two different locations (button labels and callbacks are in one function, button positioning is in a different function). \$\endgroup\$ – Austin Hastings May 8 at 22:50
  • 2
    \$\begingroup\$ @BrunoLemos (cont'd) I think it makes more sense to have one function that does "everything for a button", and call that function 25 times. Then you have 30-ish lines of code (5 lines of function plus 25 calls) and all the information about a given button is in one place - the call to the function where the parameters are all in line. \$\endgroup\$ – Austin Hastings May 8 at 22:52
  • 1
    \$\begingroup\$ @BrunoLemos Suppose you have a function that opens a file, processes the contents, and closes the file. You could call open() directly, or you could "abstract away" the low-level open call and write a function named read_data() that would return the data. Sometimes it makes sense to have a "simple" function like read_data so that you can write code like read_data(); process_data(); write_data() instead of mixing the abstraction levels like f = open(); data = read(f); process_data(data); write_data() \$\endgroup\$ – Austin Hastings May 24 at 21:26
  • 1
    \$\begingroup\$ self.num_rows (&cols) are counts, which are 1-based. So they are 1 higher than the highest value returned by enumerate, which is 0-based. Thus, if you enumerate(['a', 'b']) you will get (0, 'a') and (1, 'b'), but the num_items should be two rather than one. So I added one outside the loop. \$\endgroup\$ – Austin Hastings May 24 at 21:30

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.