1
\$\begingroup\$

I would like to critical advice on my code:

#-----import modules-----#
from tkinter import *
from turtle import *
from random import sample , seed

#-----Setup-----#
win = Tk() # ...generates a window...
win.title("Lotto Number Generator") 
#win.geometry("500x500")
win.resizable(False,False)

#-----Widget Declaration-----# ...With Labels and Buttons
Labl1 = Label(win , relief = 'groove' , width = 2)
Labl2 = Label(win , relief = 'groove' , width = 2)
Labl3 = Label(win , relief = 'groove' , width = 2)
Labl4 = Label(win , relief = 'groove' , width = 2)
Labl5 = Label(win , relief = 'groove' , width = 2)
Labl6 = Label(win , relief = 'groove' , width = 2)
LablX = Label(win , relief = 'groove' , width = 2) #This is a lucky number. 
ResetBtn = Button(win , text = "Reset")
PickBtn = Button(win , text = "Pick My Lucky Numbers")

#-----Position Widgets-----#
Labl1.grid(row = 1 , column = 1 , padx = 10)
Labl2.grid(row = 1 , column = 2 , padx = 10)
Labl3.grid(row = 1 , column = 3 , padx = 10)
Labl4.grid(row = 1 , column = 4 , padx = 10)
Labl5.grid(row = 1 , column = 5 , padx = 10)
Labl6.grid(row = 1 , column = 6 , padx = 10)
LablX.grid(row = 1 , column = 7 , padx = 20)

ResetBtn.grid(row = 2 , column = 6 , columnspan = 2)
PickBtn.grid(row = 2 , column = 1 , columnspan = 5)

#-----Functions-----#
def reset(): ...That resets the labels...
    Labl1.configure(text='...')
    Labl2.configure(text='...')
    Labl3.configure(text='...')
    Labl4.configure(text='...')
    Labl5.configure(text='...')
    Labl6.configure(text='...')
    LablX.configure(text='...')
    PickBtn.configure(state = NORMAL)
    ResetBtn.configure(state = DISABLED)

def pick(): #...and can pick random numbers WITH a bonus number
    picks = sample(range(1,49) , 7)
    LablX.configure(text=picks[6])
    del picks[6]
    picks.sort()
    Labl1.configure(text=picks[0])
    Labl2.configure(text=picks[1])
    Labl3.configure(text=picks[2])
    Labl4.configure(text=picks[3])
    Labl5.configure(text=picks[4])
    Labl6.configure(text=picks[5])
    PickBtn.configure(state = DISABLED)
    ResetBtn.configure(state = NORMAL)

#-----Assign Functions-----#
ResetBtn.configure(command = reset)
PickBtn.configure(command = pick)

#-----Initialise-----#
reset()



win.mainloop()
\$\endgroup\$
  • \$\begingroup\$ Why not holding these Labels in an array and do all those initializations in a loop? \$\endgroup\$ – πάντα ῥεῖ Jan 6 '18 at 12:14
  • \$\begingroup\$ On asking questions: there's at least the help center and How to get the best value out of Code Review - Asking Questions. \$\endgroup\$ – Mast Jan 6 '18 at 12:14
  • \$\begingroup\$ Why is the 7th labeled LablX? \$\endgroup\$ – Mast Jan 6 '18 at 12:15
  • \$\begingroup\$ @πάνταῥεῖ I don't know how to do that? Could you lead me to a link to show me how please? \$\endgroup\$ – VortexYT Jan 6 '18 at 12:17
  • \$\begingroup\$ @Mast It's a Lucky Number \$\endgroup\$ – VortexYT Jan 6 '18 at 12:18
1
\$\begingroup\$

Don't use wildcard imports

Instead of doing from tkinter import *, use import tkinter as tk and then prefix your use of tkinter objects with tk. (eg: tk.Tk(), tk.Label(...), etc)

Use an array to keep track of multiple identical widgets.

Whenever you have a number as part of a variable name, that's a code smell telling you there's probably a better way. Instead of Lab1, Lab2, etc, put the labels in a list:

labels = []
for column in range(7):
    label = tk.Label(win , relief = 'groove' , width = 2)
    label.grid(row = 1 , column = column+1 , padx = 10
    labels.append(label)

Later, when you need to reconfigure the widget you can use the global labels like so:

for i in range(5):
    labels[i].configure(text=picks[i])

Follow PEP8

You should follow the naming conventions of PEP8. Specifically, don't use an uppercase character as the first letter of a variable name.

Don't turn off the ability of the user to resize the window

Don't do win.resizable(False,False). Users should be able to control their window size.

Use sticky appropriately

When using grid, you should almost always explicitly set the sticky attribute. It's rate to need to put something in a grid cell and not have it expand or contract to fit that cell.

Set row and column weights

When using grid, you should always give at least one row and one column a non-zero weight. If you don't want your rows and columns to grow and shrink when the user resizes the window, give the weight to an empty column to the right of all other columns, and to the row below all other rows. This will explicitly tell tkinter to allocate extra space to the edges.

Don't include unnecessary imports

It doesn't look like you're using the turtle module, so remove the import.

\$\endgroup\$
  • \$\begingroup\$ Okay thanks, about the turtle module, I am using it. Just, not yet... \$\endgroup\$ – VortexYT Jan 6 '18 at 22:53

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.