5
\$\begingroup\$

I was told to have my code reviewed here as I am a total beginner to Python.
I made a small Tkinter script that generates a 4x4 field of buttons and randomly recolors them when clicking on any button, but I was told that the code is not clean. If someone could point out a few flaws and some possible enhancements that would relly help a lot.

from tkinter import *
from random import randint

window = Tk()
window.title("Test")
window.geometry('200x200')
color = ["red","blue","green","yellow","black","purple","orange"]
RandInt = 0
j = 0
h = 0

def ButtonDef(xvar = 0,yvar = 0): 
    btn = Button(window,command =lambda:[RandomColor()])
    btn.grid()
    btn.place(x = xvar*50, y = yvar*50, width = 50, height = 50)

    def RandomColor():
        for child in window.winfo_children():
            child.configure(bg=color[randint(0,6)])

while j in range (4):
    i = 0
    j += 1
    while i in range (4):
        ButtonDef(i,h)
        i += 1
        if i == 4:
            h += 1
window.mainloop()
\$\endgroup\$
3
\$\begingroup\$
  1. One of the first useful things to take in consideration when developing in Python (whether you are a beginner or expert) is the PEP 8 that provides general rules how to write a code that can help other developers to read your code.

    • For example, from the naming conventions section you can read that variable names are written in snake_camel case. This means RandInt should be re-written as rand_int and ButtonDef() RandomColor() should be written button_def() and random_color() respectively.
    • In the imports section, you can read that we should avoid wild card imports. This means we should write import tkinter as tk instead of from tkinter import *
  2. Names should be meaningful and express their purpose. This means ButtonDef(), which is rather a misleading name, could be named create_buttons() instead because you are creating 16 button widgets with that function. I would also rename RandomColor() by something like get_random_color() or pick_random_color() because a function does something, so it is better the name gets prefixed with a verb. Also, color is actually referencing several colors, so it should be named colors instead or, why not, colors_list because it consists of a list of colors.
  3. General code formatting rules:

    • When affecting a value to a variable, we should leave one space character around both sides of the affectation symbol =. But this does not apply to tkinter when it comes to a widget's argument. Let me take an example to clarify this: btn = Button(window,command =lambda:[RandomColor()]) should be written btn = Button(window,command=lambda:[RandomColor()]) (changed ...command =lambda:... to ...command=lambda:...)
    • In this line: while j in range (4): we should remove the space in range (4): and write range(4): instead.
  4. There are two main suggestions:

    • I would write your code in an OOP approach. No wonder that OOP and GUI coincided in birth historically.
    • Ideally, whenever we create widgets, we would keep a reference to them for later reuse or code invocation. In your code, btn is overridden several 16 times so, by the end, it refers only to the last created button widget; this means you can not access easily ([winfo_children()][3] offers a way to access the child widgets of window but it is hard to have a friendly control on them)
  5. When you have to deal with several widgets and manage them through different lines and columns, grid() will make your life much easier than place().

Based on all the elements previously mentioned, and based on tkinter best practices article, I would re-rewrite your code as follows:

import tkinter as tk
import random


class ChangeButtonsColor(tk.Frame):

   def __init__(self, master):
       self.master = master
       tk.Frame.__init__(self, self.master)
       self.__colors = ["red","blue","green","yellow","black","purple","orange"]
       self.configure_gui()
       self.create_widgets()

   def configure_gui(self):
       self.master.title('Test')
       self.master.geometry('200x200')

   def create_widgets(self):
       self.create_buttons()       

   def create_buttons(self):
       self.buttons = {}     
       for i in range(4):
           for j in range(4):              
               self.buttons['button{}'.format(4*i + j)] = tk.Button(self.master, bg=self.get_random_color())
               self.buttons['button{}'.format(4*i + j)].grid(row=i, column=j)
               self.buttons['button{}'.format(4*i + j)].config(width=3, height=3)
               self.buttons['button{}'.format(4*i + j)].config(command=self.change_buttons_color)


   def get_random_color(self):
       return random.choice(self.__colors)

   def change_buttons_color(self):
       for i in range(16):
           self.buttons['button{}'.format(i)].config(bg=self.get_random_color())       



if __name__ == '__main__':
   root = tk.Tk()
   main_app =  ChangeButtonsColor(root)
   root.mainloop()

Note that in I created a dictionary to keep references to each button I created in create_buttons() function. The way to access each button is demonstrated in change_buttons_color()

\$\endgroup\$
  • \$\begingroup\$ I heard about the OOA-OOD-OOP workflow, i think i should make a clear goal and see what i should do for that goal step by step - in this case, i just wrote whatever came to my mind because im kind of uncreative when it comes to creating something from scratch - im currently looking into the init and self stuff, i think thats what im not getting the most right now \$\endgroup\$ – Flying Thunder May 15 '18 at 11:40
  • 1
    \$\begingroup\$ Storing buttons in a dict seems cumbersome at best, why not simply use a list and have change_buttons_color simply do for button in self.buttons: button.config(bg=self.get_random_color())? Same for create_buttons, why use an extra method on top of create_widgets? \$\endgroup\$ – Mathias Ettinger May 15 '18 at 12:36
  • \$\begingroup\$ Good note about the list. For the extra function you mentioned: in case the OP wants to enrich the GUI and create other areas of widgets, then he can add a corresponding suitable function name. When you have several components on the GUI and you know that you will have to configure them at least once, it is good to group related widgets in specific functions otherwise you get lost, I say this from my humble experience with tkinter @MathiasEttinger \$\endgroup\$ – Billal Begueradj May 15 '18 at 12:44

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.