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()
\$\endgroup\$
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.

\$\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\$ Dec 29 '19 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\$
    – Samwise
    Dec 29 '19 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\$
    – Samwise
    Dec 29 '19 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\$ Dec 29 '19 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\$
    – Samwise
    Dec 29 '19 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()

\$\endgroup\$
1
  • \$\begingroup\$ ill check out PEP-8 thanks for telling me about it. \$\endgroup\$ Dec 29 '19 at 1:49

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.