9
\$\begingroup\$

I just started working on a little project to help me study for a course I'm taking.

Is there anything I can do to improve code readability / bad practices?

I had to remove most of the labels and buttons (due to stack-overflow posting rules) but I kept one of everything just so you can sus out how I programmed those features.

 import tkinter as tk
 import pandas as pd 
 import datetime as datetime
 import random
 #Global Variables
 labelH1 = ("Verdana", 20)
 labelH2 = ("Verdana", 17)
 labelParagraph = ("Verdana", 13)
 labelButton = ("Verdana", 11)
########################################################################
class GUI:
 """"""
 def __init__(self, root):
 """Constructor"""
 self.root = root # root is a passed Tk object
 #Custom Window Height
 #TODO! change to ratio
 window_height = 700
 window_width = 1000
 #Get User Screen Info
 screen_width = root.winfo_screenwidth()
 screen_height = root.winfo_screenheight()
 #Center GUI
 x_cordinate = int((screen_width/2) - (window_width/2))
 y_cordinate = int((screen_height/2) - (window_height/2))
 #Set the Window's Position and dimensions
 root.geometry("{}x{}+{}+{}".format(window_width, window_height, x_cordinate, y_cordinate))
 ######################################################################## 
 #LANDING PAGE
 def mainMenu(self):
 self.frame = tk.Frame(self.root)
 self.frame.pack()
 self.lastPage = "Main Menu"
 #TEXT/LABELS
 header = tk.Label(self.frame, text="Welcome to a quiz", font=labelH1 )
 header.grid(row=0, column = 3, ipady =20)
 SBJMath = tk.Label(self.frame, text="Math", font=labelH2 )
 SBJMath.grid(row=1, column = 3)
 #BUTTONS
 #MATHS QUIZZES
 BTN_BasicMath = tk.Button(self.frame, text="Primer Statistics", font=labelButton, command = lambda: self.quizInstructions("BasicMath"))
 BTN_BasicMath.grid(row=2,column=2, pady=10)
 #EXTRA BUTTONS
 BTNQuit = tk.Button(self.frame, text="Quit", font=labelButton, command =root.destroy)
 BTNQuit.grid(row=13, column=0)
 ######################################################################## 
 #ABOUT QUIZ PAGE 
 def quizInstructions(self, quizName):
 self.removethis()
 self.lastPage = "Quiz About"
 quizHeader = ""
 quizAbout = ""
 chooseQuiz = ""
 if quizName == "BasicMath":
 quizHeader = "Primer Statistics"
 quizAbout = """big line"""
 chooseQuiz = ""
 self.frame = tk.Frame(self.root)
 self.frame.pack()
 tk.Label(self.frame, text=quizHeader, font=labelH1 ).grid(row=0, column=3, pady = 20)
 tk.Label(self.frame, text=quizAbout, font=labelParagraph, wraplength=600,anchor="n" ).grid(row=1, column=3, pady = 30)
 tk.Button(self.frame, text="Go Back", font=labelButton, command=self.returnToLastFrame).grid(row=2, column=3, sticky = tk.W)
 tk.Button(self.frame, text="Start Quiz", font=labelButton, command=self.quiz ).grid(row=2, column=3, sticky = tk.E) 
 ########################################################################
 #QUIZ GUI
 def quiz(self):
 self.lastPage = "Quiz"
 self.removethis()
 self.frame = tk.Frame(self.root)
 self.frame.pack()
 self.quizLogic()
 ########################################################################
 #QUIZ LOGIC 
 def quizLogic(self):
 pass
 def removethis(self):
 self.frame.destroy()
 #Go back
 def returnToLastFrame(self):
 self.removethis()
 if self.lastPage == "Main Menu":
 pass
 if self.lastPage == "Quiz About":
 self.mainMenu()
 if self.lastPage == "Quiz":
 self.quizInstructions()
 else:
 pass
 #----------------------------------------------------------------------
 if __name__ == "__main__":
 root = tk.Tk()
 window = GUI(root).mainMenu()
 root.mainloop()
chicks
2,8593 gold badges18 silver badges30 bronze badges
asked Dec 28, 2019 at 13:09
\$\endgroup\$

2 Answers 2

10
\$\begingroup\$

1. Instead of:

def __init__(self, root):
 """Constructor"""
 self.root = root # root is a passed Tk object

do:

def __init__(self, root: tkinter.Tk) -> None:
 self.root = root

Python 3 has built in support for type annotations, so there's no reason to use comments to say what the type of a variable is. If you use mypy (and if you're writing Python in the modern era you should be using mypy) type annotations will help you catch the majority of bugs before you even run your code. If there's only one thing you take away from this answer, it should be use type annotations and use mypy. That on its own will instantly start to push you into habits that will make your code easier to read and maintain.

Any reader who knows Python will know that __init__ is a constructor; no need for a docstring that just says "Constructor" (although you could have one that briefly summarizes what state the freshly constructed object is in if it's not adequately evident from skimming the code).

  1. As @finally said, add line breaks between your code blocks. Think of them like paragraphs; you don't mash all your sentences together into a single blob, you add paragraph breaks so that the reader can quickly see where one thought ends and another begins before they've parsed each individual sentence.

  2. All of your member variables should be declared (even if they aren't initialized) in your __init__ function. But more importantly, variables that don't need to be member variables should not be declared as such. I'm looking particularly at self.frame, which is declared in your methods but not in your constructor -- but its value does not persist beyond any given method call as far as I can tell, so it shouldn't be self.frame, it should just be a local variable frame. You can just do a find+replace of self.frame to frame and I don't think it'd change the functioning of this code at all, but it frees the reader of having to read through every method to figure out how the self.frame set by one method call might impact another.

  3. For an example of a member variable that you can't convert to a local variable, self.lastPage is one that should be declared in the constructor. (Also, it should be snake_case rather than camelCase, since that's the Python convention for variable names.) If it doesn't have a useful default value, you can use None:

 self.last_page: Optional[str] = None
  1. If a string is only ever used internally to track changes in state, it's better to use an Enum, since it's impossible to mess those up via a typo, and there's a single source of truth that enumerates all of the possible values, so you don't have to hunt through all the places where it gets set to know what cases your code needs to handle. A Page class that enumerates all of the possible pages in your UI might look like:
 from enum import auto, Enum
 class Page(Enum):
 NONE = auto()
 MAIN_MENU = auto()
 QUIZ = auto()
 QUIZ_ABOUT = auto()

(Note that to build this list I had to read through all of your code, and I'm not 100% positive that I didn't miss something -- this is the exact sort of annoyance that's avoided if you use an enum from the get-go!)

Now in your constructor you can say:

 self.last_page = Page.NONE

and in your other functions you can use Page.MAIN_MENU, etc. Your "quiz name" seems like it also might be a good candidate for conversion into a Quiz(Enum) that enumerates all of the possible quizzes.

  1. Break up long lines. Instead of:

    tk.Label(self.frame, text=quizHeader, font=labelH1 ).grid(row=0, column=3, pady = 20)
    tk.Label(self.frame, text=quizAbout, font=labelParagraph, wraplength=600,anchor="n" ).grid(row=1, column=3, pady = 30)
    tk.Button(self.frame, text="Go Back", font=labelButton, command=self.returnToLastFrame).grid(row=2, column=3, sticky = tk.W)
    tk.Button(self.frame, text="Start Quiz", font=labelButton, command=self.quiz ).grid(row=2, column=3, sticky = tk.E) 
    

try:

 tk.Label(
 self.frame, 
 text=quizHeader, 
 font=labelH1,
 ).grid(
 row=0, 
 column=3, 
 pady=20,
 )
 tk.Label(
 self.frame, 
 text=quizAbout, 
 font=labelParagraph, 
 wraplength=600,
 anchor="n",
 ).grid(
 row=1, 
 column=3, 
 pady=30,
 )
 tk.Button(
 self.frame, 
 text="Go Back", 
 font=labelButton, 
 command=self.returnToLastFrame,
 ).grid(
 row=2, 
 column=3, 
 sticky=tk.W,
 )
 tk.Button(
 self.frame, 
 text="Start Quiz", 
 font=labelButton, 
 command=self.quiz,
 ).grid(
 row=2, 
 column=3, 
 sticky=tk.E,
 ) 

This does require more vertical scrolling to read, but now it's very easy to visually compare the parameters of each Button and grid call. Notice that I also made the whitespace consistent: no whitespace around the = in a keyword parameter call, and every function call in this block follows the same convention of one argument per line, with a trailing comma, so that the arguments are all lined up in neat columns and each has the same punctuation style.

  1. Look for opportunities to turn boilerplate code into utility functions. Taking the above example, if you end up having a lot of labels and buttons to create, put the complexity of setting up the Tk widgets into helper functions so that it's easier to read the code that actually defines the menu.

Here's a quick attempt at rewriting the entire quizInstructions function in a way that would make it easier (I think) to add other quiz types and other widgets:

from enum import auto, Enum
from typing import Callable
import tkinter as tk
class Page(Enum):
 NONE = auto()
 MAIN_MENU = auto()
 QUIZ = auto()
 QUIZ_ABOUT = auto()
class Quiz(Enum):
 BASIC_MATH = auto()
def _make_header(frame: tk.Frame, row: int, column: int, text: str) -> None:
 tk.Label(
 frame,
 text=text,
 font=labelH1,
 ).grid(
 row=row,
 column=column,
 pady=20,
 )
def _make_pgraph(frame: tk.Frame, row: int, column: int, text: str) -> None:
 tk.Label(
 frame,
 text=text,
 font=labelParagraph,
 wraplength=600,
 anchor="n"),
 ).grid(
 row=row,
 column=column,
 pady=30,
 )
def _make_button(
 frame: tk.Frame,
 row: int, 
 column: int, 
 sticky: str,
 text: str, 
 command: Callable[[], None], 
) -> None:
 tk.Button(
 frame, 
 text=text, 
 font=labelButton, 
 command=command
 ).grid(
 row=row, 
 column=column, 
 sticky=sticky
 )
def quiz_instructions(self, quiz: Quiz) -> None:
 """About Quiz page"""
 self.removethis()
 self.last_page = Page.QUIZ_ABOUT
 quiz_descriptions = {
 Quiz.BASIC_MATH: (
 "Primer Statistics", 
 """big line"""
 ),
 # Quiz.SOMETHING_ELSE: (
 # "Something Else", 
 # """another big line"""
 # ),
 # etc.
 }
 header, about = quiz_descriptions[quiz]
 frame = tk.Frame(self.root)
 frame.pack()
 _make_header(frame, 0, 3, header)
 _make_pgraph(frame, 1, 3, about)
 _make_button(frame, 2, 3, tk.W, "Go Back", self.returnToLastFrame)
 _make_button(frame, 2, 3, tk.E, "Start Quiz", self.quiz)

Note that I used a bit of artifice in my function definitions to make the calling code as easy to read as possible -- I anticipate that when I read the code that sets up a widget, I'm going to need to visualize where each widget is in the grid, so I deliberately set up row and col as the first arguments to make them easy to spot.

I also changed the code that sets up the quiz descriptions from what was presumably going to grow into a long if chain into a single dict; this dict could easily be defined elsewhere and then imported or passed into this function if you wanted to, say, separate the definition of these strings from the layout code.

answered Dec 28, 2019 at 20:28
\$\endgroup\$
5
  • \$\begingroup\$ WOW! thank you so much. I have a question with point 6 though I'm going to have about 40 or so buttons and labels so if I use the convention that you have talked about I would end up with around 400ish lines. is that okay? \$\endgroup\$ Commented Dec 29, 2019 at 1:48
  • \$\begingroup\$ If you have that much code in a single function it's a little problematic and scrunching complex statements onto single lines so it's technically fewer lines isn't the way to fix that (even if it'll trick some linters). :) My advice is still to break those complex statements up for readability, but when you have 40 buttons you might want to group them into separate functions -- presumably your menu is going to have visual groupings, so that'd be a logical way to split out the functions. \$\endgroup\$ Commented Dec 29, 2019 at 2:16
  • \$\begingroup\$ I'd also look at whether it makes sense to put some of the common code into utility functions -- anything that you find yourself copying and pasting is a good candidate. I'll add a simple example to this answer. \$\endgroup\$ Commented Dec 29, 2019 at 2:20
  • \$\begingroup\$ I have decided to start over, reusing some functionality. I will have a base class (GUI) all menus will inherit from the class GUI. I'm trying to figure out how to cut down on the use of buttons and labels because that's where most of the readability issues. \$\endgroup\$ Commented Dec 29, 2019 at 2:52
  • \$\begingroup\$ Check out the edit I just submitted; I think identifying your common usage patterns and then turning those into functions will make it a lot easier to define more complex menus in a more concise way. \$\endgroup\$ Commented Dec 29, 2019 at 3:05
4
\$\begingroup\$

If talking about code readability, I'd recommend you to read PEP-8 at first, for example:

def one():
 pass
def two():
 pass

This is how you don't want your code to look like. Instead, it should look like this:

def one(): # you can add hints for this function here
 pass
def two():
 '''
 or if it's documentation, you can leave it here
 '''
 pass

Also, it's better to have newlines between blocks of code and blocks of comments. Don't stack everything in one pile:

self.root = root # root is a passed Tk object
#Custom Window Height
#TODO! change to ratio
window_height = 700
window_width = 1000
#Get User Screen Info
screen_width = root.winfo_screenwidth()
screen_height = root.winfo_screenheight()
S.S. Anne
1,78510 silver badges27 bronze badges
answered Dec 28, 2019 at 17:34
\$\endgroup\$
1
  • \$\begingroup\$ ill check out PEP-8 thanks for telling me about it. \$\endgroup\$ Commented Dec 29, 2019 at 1:49

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.