GUI to load/save player information in database

Question

I've started programming using Python two months ago. My only prior programming experience was VBA. I'm completely self taught. I wrote the code below for a project I'm creating as a way to learn the language.

I was hoping someone experienced with Python could look over my code quickly and let me know which places could be written better. It all works but I feel(especially with the variables) there must be best practices I am missing.

Any feedback would be greatly appreciated.

The code is meant to load a window with input boxes where you can save/load player information into a database file. It's will be used for a Dungeons and Dragons game. It does what its supposed to.

I'm more concerned with the code vs functionality. Is there a more elegant way to get the same results? Specifically within the def makeVar(self) section.

from tkinter import * #import tkinter
from tkinter import messagebox as mb
import SQLclass

SQL = SQLclass.Database("Players.db")  #connect db
window = Tk() #make variable for tkinter

#overwrite the X button to exit script
def xbutt():
    exit()
window.protocol('WM_DELETE_WINDOW', xbutt)  

class PlayerWindow: 
    """
        Player input window. Used to add groups with player stats to players database.
    """
    def __init__(self, top): 
        self.makeVar()
        self.layoutWindow()
        self.buttons()
        self.populateOM()
        self.getvalues()

    def layoutWindow(self):
        window.title("Player Entry Form")

        #numbers
        Label(window,text="1").grid(row=1,column=0)
        Label(window,text="2").grid(row=2,column=0)
        Label(window,text="3").grid(row=3,column=0)
        Label(window,text="4").grid(row=4,column=0)
        Label(window,text="5").grid(row=5,column=0)
        Label(window,text="6").grid(row=6,column=0)
        Label(window,text="7").grid(row=7,column=0)
        Label(window,text="8").grid(row=8,column=0)

        #Player Names
        Label(window,text="Player Name").grid(row=0,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][1]).grid(row=1,column=1, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][1]).grid(row=2,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][1]).grid(row=3,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][1]).grid(row=4,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][1]).grid(row=5,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][1]).grid(row=6,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][1]).grid(row=7,column=1, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][1]).grid(row=8,column=1, padx=5, pady=2)        

        #Character Names
        Label(window,text="Character Name").grid(row=0,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][2]).grid(row=1,column=3, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][2]).grid(row=2,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][2]).grid(row=3,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][2]).grid(row=4,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][2]).grid(row=5,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][2]).grid(row=6,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][2]).grid(row=7,column=3, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][2]).grid(row=8,column=3, padx=5, pady=2)  

        #Class Names
        Label(window,text="Class Name").grid(row=0,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][3], width=12).grid(row=1,column=4, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][3], width=12).grid(row=2,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][3], width=12).grid(row=3,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][3], width=12).grid(row=4,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][3], width=12).grid(row=5,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][3], width=12).grid(row=6,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][3], width=12).grid(row=7,column=4, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][3], width=12).grid(row=8,column=4, padx=5, pady=2)  

        #Level
        Label(window,text="Level").grid(row=0,column=5, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][4], width=3).grid(row=1,column=5, padx=5, pady=4) 
        Entry(window, textvariable=self.PlayerValues[1][4], width=3).grid(row=2,column=5, padx=5, pady=4)
        Entry(window, textvariable=self.PlayerValues[2][4], width=3).grid(row=3,column=5, padx=5, pady=4)
        Entry(window, textvariable=self.PlayerValues[3][4], width=3).grid(row=4,column=5, padx=5, pady=4)
        Entry(window, textvariable=self.PlayerValues[4][4], width=3).grid(row=5,column=5, padx=5, pady=4)
        Entry(window, textvariable=self.PlayerValues[5][4], width=3).grid(row=6,column=5, padx=5, pady=4)
        Entry(window, textvariable=self.PlayerValues[6][4], width=3).grid(row=7,column=5, padx=5, pady=4)
        Entry(window, textvariable=self.PlayerValues[7][4], width=3).grid(row=8,column=5, padx=5, pady=4) 

        #HP
        Label(window,text="HP").grid(row=0,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][5], width=3).grid(row=1,column=6, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][5], width=3).grid(row=2,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][5], width=3).grid(row=3,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][5], width=3).grid(row=4,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][5], width=3).grid(row=5,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][5], width=3).grid(row=6,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][5], width=3).grid(row=7,column=6, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][5], width=3).grid(row=8,column=6, padx=5, pady=2)  

        #Strength Names
        Label(window,text="STR").grid(row=0,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][6], width=3).grid(row=1,column=7, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][6], width=3).grid(row=2,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][6], width=3).grid(row=3,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][6], width=3).grid(row=4,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][6], width=3).grid(row=5,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][6], width=3).grid(row=6,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][6], width=3).grid(row=7,column=7, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][6], width=3).grid(row=8,column=7, padx=5, pady=2)  

        #Dexterity Names
        Label(window,text="DEX").grid(row=0,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][7], width=3).grid(row=1,column=8, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][7], width=3).grid(row=2,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][7], width=3).grid(row=3,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][7], width=3).grid(row=4,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][7], width=3).grid(row=5,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][7], width=3).grid(row=6,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][7], width=3).grid(row=7,column=8, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][7], width=3).grid(row=8,column=8, padx=5, pady=2)  

        #Constitution
        Label(window,text="CON").grid(row=0,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][8], width=3).grid(row=1,column=9, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][8], width=3).grid(row=2,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][8], width=3).grid(row=3,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][8], width=3).grid(row=4,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][8], width=3).grid(row=5,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][8], width=3).grid(row=6,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][8], width=3).grid(row=7,column=9, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][8], width=3).grid(row=8,column=9, padx=5, pady=2)  

        #Intelligence
        Label(window,text="INT").grid(row=0,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][9], width=3).grid(row=1,column=10, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][9], width=3).grid(row=2,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][9], width=3).grid(row=3,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][9], width=3).grid(row=4,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][9], width=3).grid(row=5,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][9], width=3).grid(row=6,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][9], width=3).grid(row=7,column=10, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][9], width=3).grid(row=8,column=10, padx=5, pady=2)  

        #Wisdom
        Label(window,text="WIS").grid(row=0,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][10], width=3).grid(row=1,column=11, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][10], width=3).grid(row=2,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][10], width=3).grid(row=3,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][10], width=3).grid(row=4,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][10], width=3).grid(row=5,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][10], width=3).grid(row=6,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][10], width=3).grid(row=7,column=11, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][10], width=3).grid(row=8,column=11, padx=5, pady=2)  

        #Charisma
        Label(window,text="CHA").grid(row=0,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[0][11], width=3).grid(row=1,column=12, padx=5, pady=2) 
        Entry(window, textvariable=self.PlayerValues[1][11], width=3).grid(row=2,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[2][11], width=3).grid(row=3,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[3][11], width=3).grid(row=4,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[4][11], width=3).grid(row=5,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[5][11], width=3).grid(row=6,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[6][11], width=3).grid(row=7,column=12, padx=5, pady=2)
        Entry(window, textvariable=self.PlayerValues[7][11], width=3).grid(row=8,column=12, padx=5, pady=2)      

    def makeVar(self):
        self.Player1 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player2 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player3 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player4 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player5 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player6 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player7 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.Player8 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ]
        self.output1 = ["","","","","","","","","","","",""]
        self.output2 = ["","","","","","","","","","","",""]
        self.output3 = ["","","","","","","","","","","",""]
        self.output4 = ["","","","","","","","","","","",""]
        self.output5 = ["","","","","","","","","","","",""]
        self.output6 = ["","","","","","","","","","","",""]
        self.output7 = ["","","","","","","","","","","",""]
        self.output8 = ["","","","","","","","","","","",""]
        self.check1 = ["","","","","","","","","","","",""]
        self.check2 = ["","","","","","","","","","","",""]
        self.check3 = ["","","","","","","","","","","",""]
        self.check4 = ["","","","","","","","","","","",""]
        self.check5 = ["","","","","","","","","","","",""]
        self.check6 = ["","","","","","","","","","","",""]
        self.check7 = ["","","","","","","","","","","",""]
        self.check8 = ["","","","","","","","","","","",""]
        self.PlayerValues = [self.Player1,self.Player2,self.Player3,self.Player4,self.Player5,self.Player6,self.Player7,self.Player8]
            
    def populateOM(self):
        global existingGroups
        existingGroups = []
        existingGroups = SQL.fetchSQL('SELECT DISTINCT "Group Name" from players') #get group names
        existingGroups = [a_tuple[0] for a_tuple in existingGroups] #remove tuples from list
        existingGroups.append("New Group")
        self.groupmenu['menu'].delete(0, 'end')
        for choice in existingGroups:
            self.groupmenu['menu'].add_command(label=choice, command=lambda value=choice: self.optV.set(value))

    def buttons(self):
        #pulldown
        self.optV = StringVar()
        self.optV.trace("w", self.OptionMenu_Change) #put a trace on the variable(pulldown) to call if anything changes
        self.optV.set("New Group")
        self.groupmenu = OptionMenu(window, self.optV , "")
        self.groupmenu.grid(row=10,column=1, columnspan=2,padx=5, pady=2)
        self.optN = self.optV.get()

        #Buttons
        Button(window, text="Save and Close", command=self.SaveNClose, width=15).grid(row=10,column=3,padx=5, pady=2)#load button to points to SaveNClose function
        Button(window, text="Delete Group", command=self.DeleteGroup, width=15).grid(row=10,column=4,padx=5, pady=2,columnspan=2)#load button to points to SaveNClose function
        Button(window, text="Clear", command=self.clearvalues, width=15).grid(row=10,column=6,padx=5, pady=2,columnspan=2)#load button to points to SaveNClose function

    def getvalues(self):
        self.optN = self.optV.get() 
        self.OutputValues = [self.output1,self.output2,self.output3,self.output4,self.output5,self.output6,self.output7,self.output8]
        for r in range(0,7):
            for c in range(1,11): #11 is how many columns in the DB table minus the group name
                self.OutputValues[r][c] = self.PlayerValues[r][c].get()
 
    def checkvalues(self):
        self.optN = self.optV.get() 
        self.CheckedValues = [self.check1,self.check2,self.check3,self.check4,self.check5,self.check6,self.check7,self.check8]
        for r in range(0,7):
            for c in range(1,11): 
                self.CheckedValues[r][c] = self.PlayerValues[r][c].get()

    def LoadData(self):
        self.clearvalues()
        self.existingGroups = SQL.fetchSQL(f'SELECT * from players where "Group Name" = "{self.optV.get()}"')
        for r in range(0,len(self.existingGroups)):
            for c in range(1,11):
                self.PlayerValues[r][c].set(self.existingGroups[r][c])

    def OptionMenu_Change(self, *args):
        self.checkvalues()
        if self.optV.get() != "New Group":
            if self.OutputValues == self.CheckedValues: #check for changes made since loaded
                self.LoadData()
            else:
                x= mb.askyesno("Load Group?", f"Do you want to load {self.optV.get()}? All unsaved changes will be lost.")
                if x == True:
                    self.LoadData()
        self.getvalues()
            
    def DeleteGroup(self):
        if self.optV.get() != "New Group":
            x= mb.askyesno("Delete Group?", f"Delete the group  {self.optV.get()}  ?")
            if x == True:
                SQL.SendSQL(f'DELETE FROM Players WHERE "Group Name" = "{self.optV.get()}";')
                self.optV.set("New Group")
                self.clearvalues()
                self.populateOM()

    def clearvalues(self):
         for r in range(0,7):
            for c in range(1,11):
                self.PlayerValues[r][c].set("")
 
    def SaveNClose(self):
        self.getvalues()
        window.destroy()       

pWindow=PlayerWindow(window) #make PlayerWindow an object(variable)
window.mainloop() #keep window open until its forced closed    

class InputWindow: 
    def __init__(self, title, question): 
        self.window2 = Tk() 
        self.window2.title(title)
        self.q = StringVar() 
        Label(self.window2,text=question, wraplength=250).grid(row=0,column=1, columnspan=2) #question
        Entry(self.window2, textvariable=self.q, width =50).grid(row=1,column=1, padx=5, pady=2)
        Button(self.window2, text="Save", command=self.SaveNClose, width=10).grid(row=2,column=1,padx=5, pady=2)
        self.window2.mainloop() 
    def SaveNClose(self):
        self.answer = self.q.get()
        self.window2.destroy() 

if pWindow.optN == "New Group":
    if pWindow.OutputValues[0][1]  != "":
        inpWindow=InputWindow("Group Name", "What would you like to name this group?") 
        groupname = inpWindow.answer
else:
    groupname = pWindow.optN

#make sql value statements
theValues = ""
for x in range(0,7):
    if x > 0:
        if pWindow.OutputValues[x][1] != "":
            theValues = theValues + f",('{groupname}', '{pWindow.OutputValues[x][1]}','{pWindow.OutputValues[x][2]}','{pWindow.OutputValues[x][3]}','{pWindow.OutputValues[x][4]}','{pWindow.OutputValues[x][5]}','{pWindow.OutputValues[x][6]}','{pWindow.OutputValues[x][7]}','{pWindow.OutputValues[x][8]}','{pWindow.OutputValues[x][9]}','{pWindow.OutputValues[x][10]}','{pWindow.OutputValues[x][11]}')"
    else:
        if pWindow.OutputValues[x][1] != "":
            theValues = theValues + f"('{groupname}', '{pWindow.OutputValues[x][1]}','{pWindow.OutputValues[x][2]}','{pWindow.OutputValues[x][3]}','{pWindow.OutputValues[x][4]}','{pWindow.OutputValues[x][5]}','{pWindow.OutputValues[x][6]}','{pWindow.OutputValues[x][7]}','{pWindow.OutputValues[x][8]}','{pWindow.OutputValues[x][9]}','{pWindow.OutputValues[x][10]}','{pWindow.OutputValues[x][11]}')"

#check if the players table exists yet and if not make it
SQL.SendSQL("CREATE TABLE IF NOT EXISTS players ('Group Name','Player Name', 'Character Name', 'Class','Level', 'HP','STR', 'DEX', 'CON', 'INT', 'WIS', 'CHA')")

#add group to players db
if theValues != "":
    SQL.SendSQL(f"INSERT INTO players VALUES{theValues}")

Carcigenicate · Accepted Answer · 2020-08-31 23:25:41Z

The immediate problem with the code is the sheer repetition in the middle. That will be difficult to maintain, since any non-trivial change to how data is managed will likely need to be made across all ~100 lines.

Note these lines:

Label(window, text="1").grid(row=1, column=0)
Label(window, text="2").grid(row=2, column=0)
Label(window, text="3").grid(row=3, column=0)
Label(window, text="4").grid(row=4, column=0)
Label(window, text="5").grid(row=5, column=0)
Label(window, text="6").grid(row=6, column=0)
Label(window, text="7").grid(row=7, column=0)
Label(window, text="8").grid(row=8, column=0)

All that changes are the text and row arguments. The duplication in these lines can be reduced using a loop:

for n in range(1, 9):
    Label(window, text=str(n)).grid(row=n, column=0)

This can also be applied to more complex examples like:

Entry(window, textvariable=self.PlayerValues[0][1]).grid(row=1,column=1, padx=5, pady=2) 
Entry(window, textvariable=self.PlayerValues[1][1]).grid(row=2,column=1, padx=5, pady=2)
Entry(window, textvariable=self.PlayerValues[2][1]).grid(row=3,column=1, padx=5, pady=2)
. . .

Becomes:

for y in range(8):
    row = y + 1  # row can be calculated from y
    Entry(window, textvariable=self.PlayerValues[y][1]).grid(row=row, column=1, padx=5, pady=2)

If you apply that change to each of the chunks, you get something like:

N_PLAYERS = 8  # At the top somewhere

. . .

Label(window, text="Player Name").grid(row=0, column=1, padx=5, pady=2)
for y in range(N_PLAYERS):
    Entry(window, textvariable=self.PlayerValues[y][1]).grid(row=y+1, column=1, padx=5, pady=2)

Label(window, text="Character Name").grid(row=0,column=3, padx=5, pady=2)
for y in range(N_PLAYERS):
    Entry(window, textvariable=self.PlayerValues[y][2]).grid(row=y+1,column=3, padx=5, pady=2)

Label(window, text="Class Name").grid(row=0,column=4, padx=5, pady=2)
for y in range(N_PLAYERS):
    Entry(window, textvariable=self.PlayerValues[0][3], width=12).grid(row=1,column=4, padx=5, pady=2) 
. . .

Which has much less duplication. It's not very pretty still, but it should be much easier to change if need be.

It can be improved a bit by wrapping that duplicate code in a function, then calling the functions multiple times:

def produce_entries_chunk(label_text: str, values_x: int, entry_width: int, grid_column: int):
    Label(window, text=label_text).grid(row=0, column=grid_column, padx=5, pady=2)
    for y in range(N_PLAYERS):
        entry = Entry(window, textvariable=self.PlayerValues[y][values_x], width=entry_width)
        entry.grid(row=y+1, column=grid_column, padx=5, pady=2)

Which allows you to now write:

produce_entries_chunk("Player Name", 1, 2, 1)
produce_entries_chunk("Character Name", 2, 2, 3)
produce_entries_chunk("Class Name", 3, 12, 4)
. . .

It becomes difficult to reduce down from here though due to the varying arguments passed in to each call. You could loop over tuples of (1, 2, 1), (2, 2, 3), (3, 12, 4), . . . or something, but at some point it becomes counter-productive.

Lines like this:

self.Player1 = [StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar() ,StringVar()]

Can be reduced down to:

self.Player1 = [StringVar() for _ in range(12)]

Again, loops are super helpful when trying to reduce duplication, and list comprehensions especially are great for cases just like this. Just like before though, if you apply this change across the lines, you'll see a pattern:

self.Player1 = [StringVar() for _ in range(12)]
self.Player2 = [StringVar() for _ in range(12)]
self.Player3 = [StringVar() for _ in range(12)]
self.Player4 = [StringVar() for _ in range(12)]
self.Player5 = [StringVar() for _ in range(12)]
self.Player6 = [StringVar() for _ in range(12)]
self.Player7 = [StringVar() for _ in range(12)]
self.Player8 = [StringVar() for _ in range(12)]

self.output1 = [""] * 12
self.output2 = [""] * 12
self.output3 = [""] * 12
self.output4 = [""] * 12
self.output5 = [""] * 12
self.output6 = [""] * 12
self.output7 = [""] * 12
self.output8 = [""] * 12

You're using numbers in the variable name to enumerate players, which later necessitates lines like:

self.PlayerValues = [self.Player1,self.Player2,self.Player3,self.Player4,self.Player5,self.Player6,self.Player7,self.Player8]

self.CheckedValues = [self.check1,self.check2,self.check3,self.check4,self.check5,self.check6,self.check7,self.check8]

If you start putting numbers in a variable name, that's an indication that you should instead be using a list (or potentially a dictionary):

self.players = [[StringVar() for _ in range(12)] for _ in range(N_PLAYERS)]
self.output = [[""] * 12 for _ in range(N_PLAYERS)]
self.checks = [[""] * 12 for _ in range(N_PLAYERS)]

Also note that I lower-cased players. PEP8 says that regular variable names should be lowercase, separated by underscores. enter link description here

scnerd · Accepted Answer · 2020-08-31 21:35:51Z

Welcome to Python, I do hope you find it a pleasant language to work with! The first few suggestions are really common for newcomers to Python, and the later points are just general suggestions for writing clean code (tips which will port to any language):

Become roughly familiar with PEP8 (the official style guide for Python). The vast majority of Python coders follow the vast majority of its recommendations, resulting in most of the ecosystem being easy and consistent to read, and most people and IDE's will get upset with you if you don't follow it in general. Some particular points that are relevant to this review are
1. Using snake-case variable names (self.layout_window rather than self.layoutWindow)
2. Some spacing issues with commas ([StringVar(), StringVar(), ... rather than [StringVar() ,StringVar() ,..., and ["", "", ... instead of ["","",...)
Don't run code in the top-level of your code file. Use a main block at the end (explanation here). Note from the first link that if TKinter requires you to define some globals, that's ok for this application, but it's not a good habit to get into otherwise. The primary point of using a main block is to allow your code file to be either imported from another script to reuse its components (in which case you don't want to open a window, block the interpreter, etc.) or be run directly (in which case you do).
```
if __name__ == '__main__':
    SQL = SQLclass.Database("Players.db")  #connect db
    pWindow=PlayerWindow(window) #make PlayerWindow an object(variable)
    window.mainloop() #keep window open until its forced closed
```

Follow the DRY principle. You have huge chunks of code that are the same line repeated a bunch of times with very little changed. This makes small logical changes require large code changes, and thus require more of your time. Instead, make efficient use of loops and data structures. Rule of thumb: if you're hitting "copy/paste" for a line of code, your code would probably be cleaner if you used a different approach to re-using that logic.

Instead of:

Label(window,text="1").grid(row=1,column=0)
Label(window,text="2").grid(row=2,column=0)
Label(window,text="3").grid(row=3,column=0)
# ...

use:

for player_num in range(1, number_of_players + 1):
    Label(window, text=str(player_num)).grid(row=player_num, column=0)

You have huge chunks of logic repeated for each player. You could convert these into a loop like I show above, or you could pull Player logic out into a separate class:

class Player:
    def __init__(self, num):
        self.num = num
        self.player_name = StringVar()
        self.character_name = StringVar()
        # ...

    def insert_into_grid(self, window):
        for column, variable in [
            (1, self.player_name),
            (3, self.character_name),
            # ...
        ]:
            Entry(window, textvariable=variable).grid(row=1, column=column, padx=5, pady=2)

When combined with loops like shown above, things get a LOT simpler:

class PlayerWindow:
    def __init__(self, top):
        # You don't even need ``make_vars`` anymore
        self.num_players = 8
        self.players = [Player(i + 1) for i in range(self.num_players)]
        self.layout_window()
        self.add_buttons()
        self.populate_object_model()
        self.get_values()

    def layout_window(self):
        window.title("Player Entry Form")

        # numbers
        for player_num in range(1, self.num_players + 1):
            Label(window, text=str(player_num)).grid(row=player_num, column=0)

        for column, name in [
            (1, "Player Name"),
            (3, "Character Name"),
            # ...
        ]:
            Label(window, text=name).grid(row=0, column=column, padx=5, pady=2)

        for player in self.players:
            player.insert_into_grid(window)

        # Done!

Maintain a single authoritative source of information, particularly instead of your self.outputN and self.checkN variables. These are effectively caches of information that's actually controlled somewhere else, and every time you cache information, you're in for a world of hurt trying to ensure that cache never grows stale. If it's not a significant performance hit, it's much better to just re-compute the desired value on-demand. Think of it as creating a view into the data: the data lives in the Player objects, the view is the values of the check boxes and text boxes (self.outputN and self.checkN). Since these variables only exist to get collapsed into the self.OutputValues and self.CheckedValues list-of-lists, I'll just compute those directly:
```
  class Player:
      # ...

      @property
      def attributes(self):
          return self.player_name, self.character_name, # ...


  class PlayerWindow:
      # ...

      @property
      def output_values(self):
          return [
              [attribute.get() for attribute in player.attributes]
              for player in self.players
          ]
```
Use self-explanatory variable names. E.g., it's unclear to me without further digging what self.optV, mb, pWindow (what does the "p" mean?), and theValues are.

So in short, keep your data and code structured, make everything have a single source of truth (both for tracking data and for enforcing its structure), don't repeat yourself, try to write your code such that it's self-explaining (e.g. for player in self.players: player.roll_dice()), and, since you're in Python, follow the common PEP8 style guide.

Stack Exchange Network

GUI to load/save player information in database

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged
python
beginner
or ask your own question.

Hot Network Questions

GUI to load/save player information in database

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged pythonbeginner or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
python
beginner
or ask your own question.