trjhtr

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()

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()