10
\$\begingroup\$

I have made a flashcard program. It opens a small window and displays a series of questions.

Since I am new to programming, how can my code be improved?

from tkinter import *

# This opening code is to create a window and menu options. I'm going for a
# basic framework here in which everything else will fall under.


def center_window(width=300, height=200):
    # get screen width and height
    screen_width = root.winfo_screenwidth()
    screen_height = root.winfo_screenheight()

    # calculate position x and y coordinates
    x = (screen_width/2) - (width/2)
    y = (screen_height/2) - (height/2)
    root.geometry('%dx%d+%d+%d' % (width, height, x, y))


def finished():
    root.destroy()


# Variables used throughout the code
top_color = 'royalblue'
bottom_color = 'lightsteelblue'
background_color = 'navy'
button_color = 'powderblue'

card_num = 0
questions = [
    'Who is the first Pokemon?',
    'Who is the first President?',
    'What did MLK have?',
    'Are you already dead?'
]
answers = [
    'Mew',
    'G. Washington',
    'A dream',
    'Yes'
]
fanswers1 = [
    'Pikachu',
    'T. Biggums',
    'A thought',
    'No'
]
fanswers2 = [
    'Agumon',
    'Big John',
    'An idea',
    'Perhaps'
]
fanswers3 = [
    'Exodia',
    'M. Robbins',
    'A concept',
    'Maybe'
]


root = Tk()
root.title('Flashcard Application')
root.resizable(width=0, height=0)
center_window(600, 350)
root.configure(bg=background_color)
# Makes sure the items in the root grid are stretched to capacity
root.grid_rowconfigure(0, weight=3)
root.grid_columnconfigure(0, weight=1)
root.grid_rowconfigure(1, weight=1)


def clicked_correct():
    try:
        global card_num
        card_num += 1
        question_label.configure(text=questions[card_num])
        btn1.configure(text=answers[card_num])
        btn2.configure(text=fanswers1[card_num])
        btn3.configure(text=fanswers2[card_num])
        btn4.configure(text=fanswers3[card_num])
    except IndexError:
        finished()


def clicked_incorrect():
    question_label.configure(text='Incorrect. Try Again.')


# Menu functions
def res():
    global card_num
    card_num = 0
    question_label.configure(text=questions[0])
    btn1.configure(text=answers[0])
    btn2.configure(text=fanswers1[0])
    btn3.configure(text=fanswers2[0])
    btn4.configure(text=fanswers3[0])


# Menu bar options
menu = Menu(root)
reset = Menu(menu, tearoff=0)
reset.add_command(label='Reset', command=res)
menu.add_cascade(label='File', menu=reset)

menu.add_command(label='Edit')
root.config(menu=menu)


# Creates root frame containers
top_frame = Frame(root, bg=top_color, width=600, height=225)
bottom_frame = Frame(root, bg=bottom_color, width=600, height=125)
# Places root frame containers
top_frame.grid(row=0, sticky='wens', padx=5, pady=(5, 0))
bottom_frame.grid(row=1, sticky='wens', padx=5, pady=5)

# Makes sure items in the top frame grid are stretched to capacity
top_frame.grid_rowconfigure(0, weight=1)
top_frame.grid_columnconfigure(0, weight=1)
# Creates top frame widgets
question_label = Label(top_frame, bg=top_color, text=questions[card_num], font=('Arial Bold', 30))

# Place top frame widgets
question_label.grid(row=0, column=0)

# Makes sure items in bottom frame grid are stretched to capacity
bottom_frame.grid_rowconfigure(0, weight=1)
bottom_frame.grid_rowconfigure(1, weight=1)
bottom_frame.grid_columnconfigure(0, weight=1)
bottom_frame.grid_columnconfigure(1, weight=1)


# Creates bottom widgets
btn1 = Button(bottom_frame, text=answers[card_num], bg=button_color, command=clicked_correct)
btn2 = Button(bottom_frame, text=fanswers1[card_num], bg=button_color, command=clicked_incorrect)
btn3 = Button(bottom_frame, text=fanswers2[card_num], bg=button_color, command=clicked_incorrect)
btn4 = Button(bottom_frame, text=fanswers3[card_num], bg=button_color, command=clicked_incorrect)

# Place top frame widgets
btn1.grid(row=0, column=0)
btn2.grid(row=0, column=1)
btn3.grid(row=1, column=0)
btn4.grid(row=1, column=1)

root.mainloop()
\$\endgroup\$
3
  • 2
    \$\begingroup\$ This has accumulated a close vote for having an incomplete description. Personally I think it's fine, however you may want to flesh out what flash cards and Anki are. At first I thought you were building a deck builder game. Best of luck. \$\endgroup\$
    – Peilonrayz
    Commented Apr 7, 2020 at 16:00
  • 1
    \$\begingroup\$ Your edit 23 hours ago didn't add information about what your code is doing, it removed most of the explanation. Please don't fill up questions with the fact you want a review, we know that since you've posted a question. \$\endgroup\$
    – Peilonrayz
    Commented Apr 8, 2020 at 15:53
  • \$\begingroup\$ If this is a learning/student project that's fine. If you're expecting a future user base, have you considered building on Anki instead of reinventing the wheel? en.wikipedia.org/wiki/Anki_(software) \$\endgroup\$
    – Galen
    Commented Apr 8, 2020 at 16:27

3 Answers 3

12
\$\begingroup\$

First off, it's fantastic that you're grouping your widgets together when you create them, and then grouping your calls to grid together. Most people who are starting to learn tkinter don't do that. The way you've done it makes the code much easier to understand than if you mixed it all together.

Don't use wildcard imports

Wildcard imports are discouraged by PEP8. Instead, I recommend doing imports like this:

import tkinter as tk

You will then have to change all of the code that references classes, functions, and constants to include the prefix tk. (eg: root = tk.Tk())

This has two advantages: it helps keep the global namespace from getting too large (which can lead to difficult-to-spot problems), and it makes the fact that you're using tk explicit. This is especially important if you also import ttk, as both tk and ttk have classes with the same name (Button, Label, etc).

Use a data structure that links questions with answers

Imagine if you had 100 questions, and you needed to change the answer or a false answer to question 77. You would have to manually count down 77 lines in one of the lists and hope you didn't miscount. Even worse, what if you decide to re-order all of your questions and answers? It could take you all day to reorganize the data.

Instead, a simple technique is to use a dictionary where the keys are the questions and the values are a list of answers. You could always put the correct answer as the first item in the list.

Dictionaries have a way to iterate over the values by key, so you can easily step over the flash cards in the order they are defined, or in sorted order, or in random order, or in any other way you see fit.

For example:

flashcards = {
    "Who is the first pokemon?": [
        "Mew", "Pikachu", "Agumon", "Exodia",
    ],
    "Who is the first president?": [
        "G. Washington", "T.Biggums", "Big John", "M.Robbins",
    ],
    "What did MLK have?": [
        "A dream", "A thought", "An idea", "A concept",
    ],
    "Are you already dead?": [
        "Yes", "No", "Perhaps", "Maybe",
    ]
}
...

Another alternative would be to use a list of lists, if you want to keep your current logic that depends on the question index.

For example,

flashcards = [
    ["Who is the first pokemon?", "Mew", "Pikachu", "Agumon", "Exodia"],
    ["Who is the first president?", ...],
    ...
]

Even better would be to define a Flashcard class, but I don't know if you're familar with classes yet.

Create a main function

I recommend putting the main application inside a function. Call it main (or anything else, but main will be instantly recognizable by most programmers). Then, call main() as the last step in the file.

For example:

def main():
    global root

    root = Tk()
    root.title('Flashcard Application')
    root.resizable(width=0, height=0)
    center_window(600, 350)
    root.configure(bg=background_color)
    # Makes sure the items in the root grid are stretched to capacity
    root.grid_rowconfigure(0, weight=3)
    root.grid_columnconfigure(0, weight=1)
    root.grid_rowconfigure(1, weight=1)

... the rest of your function definitions ...

if __name__ == "__main__":
    main()
# end of the file

This last condition (if __name__ == "__main__") is a common python trick which lets you import this file as a library, or use it as a script. Why is that important? It makes your code more portable, but more importantly, it makes the code more testable. You more easily write unit tests that can load up all of your functions and test them independently of the main program.

While testability isn't terribly important in such a small program, this is a good habit to form.

For more information see https://stackoverflow.com/q/419163/7432

Move all of the other code into functions too

A good rule of thumb is to have almost no code running in the global scope. While that's not overly important for such a small script, it's good to get into the habit of organizing your code into functions.

So, perhaps create a function called create_ui with all of the other tkinter code. Or, create a couple of functions such as create_menubar and create_main_ui or something like that.

This will help you to be more explicit about your use of global variables, and to think about how code logically groups together.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much! I appreciate your effort. This is exactly the feedback I was looking for and I will begin implementing these better habits immediately and in the future. \$\endgroup\$
    – Shinox
    Commented Apr 7, 2020 at 18:14
  • 2
    \$\begingroup\$ The other advantage to organizing your questions and answers into data objects is you can then randomize the question order. Which from a pedagogy perspective will keep people from memorizing the click through order. But also gives you a challenge you can move forward with to learn more and improve your code. \$\endgroup\$
    – Robby1212
    Commented Apr 8, 2020 at 3:59
5
\$\begingroup\$

Namespace pollution

It's usually a bad idea to do this:

from tkinter import *

This forces every bell and whistle from tkinter to be imported. Instead, either

  • import tkinter as tk, or
  • from tkinter import Tk, ... if you aren't importing a lot of symbols.

Order of operations

The parens here:

x = (screen_width/2) - (width/2)
y = (screen_height/2) - (height/2)

can all be dropped.

String interpolation

root.geometry('%dx%d+%d+%d' % (width, height, x, y))

can be

root.geometry(f'{width}x{height}+{x}+{y}')

Constants

These are good to remain in global scope but should be capitalized:

top_color = 'royalblue'
bottom_color = 'lightsteelblue'
background_color = 'navy'
button_color = 'powderblue'

Global code

These statements should be pulled into one or more functions:

root = Tk()
root.title('Flashcard Application')
root.resizable(width=0, height=0)
center_window(600, 350)
root.configure(bg=background_color)
# Makes sure the items in the root grid are stretched to capacity
root.grid_rowconfigure(0, weight=3)
root.grid_columnconfigure(0, weight=1)
root.grid_rowconfigure(1, weight=1)

# ...and everything after
# Menu bar options

Immutable sequences

Your questions, etc. are immutable, so make them tuples, not lists.

Computers are good at loops

# Creates bottom widgets
btn1 = Button(bottom_frame, text=answers[card_num], bg=button_color, command=clicked_correct)
btn2 = Button(bottom_frame, text=fanswers1[card_num], bg=button_color, command=clicked_incorrect)
btn3 = Button(bottom_frame, text=fanswers2[card_num], bg=button_color, command=clicked_incorrect)
btn4 = Button(bottom_frame, text=fanswers3[card_num], bg=button_color, command=clicked_incorrect)

# Place top frame widgets
btn1.grid(row=0, column=0)
btn2.grid(row=0, column=1)
btn3.grid(row=1, column=0)
btn4.grid(row=1, column=1)

can be

for ans, command, row, col in (
  (answer,    clicked_correct,   0, 0),
  (fanswers1, clicked_incorrect, 0, 1),
  (fanswers2, clicked_incorrect, 1, 0),
  (fanswers3, clicked_incorrect, 1, 1),
):
    btn = Button(bottom_frame, text=ans[card_num], bg=button_color, command=command)
    btn.grid(row=row, column=col)
\$\endgroup\$
5
  • \$\begingroup\$ I appreciate your help. This will definitely save me time in the future and I'm looking forward to developing better habits with your advice. \$\endgroup\$
    – Shinox
    Commented Apr 7, 2020 at 18:17
  • 6
    \$\begingroup\$ While the parens can be dropped, I don't think they should be dropped. Parens add clarity. This is especially important for people just now learning the language as it helps them to show what they intend. As a reviewer, if I see an expression without parenthesis I usually have to slow down and read it more closely to make sure the expression is correct. When I see parenthesis I don't have to guess as to what the author intended. \$\endgroup\$ Commented Apr 8, 2020 at 4:14
  • 2
    \$\begingroup\$ Your "computers are good at loops" advice is good, but in this case you've swapped 8 exceptionally clear lines of code with 8 lines of code that require a bit more time to understand. \$\endgroup\$ Commented Apr 8, 2020 at 4:18
  • \$\begingroup\$ Re. parens: I'll agree to disagree. Proper use of whitespace (as has already been done) along with elementary-school math makes the author's intent more than clear. \$\endgroup\$
    – Reinderien
    Commented Apr 8, 2020 at 12:57
  • \$\begingroup\$ You seem to assume people remember "elementary-school math". I've reviewed enough code in my career to know that's not always the case. \$\endgroup\$ Commented Apr 8, 2020 at 16:57
1
\$\begingroup\$

This is my revision using the advice found here. The flashcards themselves are not as easily displayed yet, but I'm going to figure that out later.

# Begin coding
from tkinter import Tk, Menu, Button, Label, Frame

card_num = 0


def main():
    # Use this function to create root window
    background_color = 'navy'
    root = Tk()
    root.title('Flashyr')
    root.resizable(0, 0)
    root.configure(bg=background_color)
    # Makes sure the items in the root grid are stretched to capacity
    root.grid_rowconfigure(0, weight=3)
    root.grid_columnconfigure(0, weight=1)
    root.grid_rowconfigure(1, weight=1)

    def center_window(width=300, height=200):
        # get screen width and height
        screen_width = root.winfo_screenwidth()
        screen_height = root.winfo_screenheight()

        # calculate position x and y coordinates
        x = (screen_width / 2) - (width / 2)
        y = (screen_height / 2) - (height / 2)
        root.geometry('%dx%d+%d+%d' % (width, height, x, y))

    def clicked_incorrect():
        question_label.configure(text='Incorrect. Try Again.')

    def clicked_correct():
        try:
            global card_num
            card_num += 1
            question_label.configure(text=c_l[card_num][card_num])
            btn2.configure(text=c_l[card_num][card_num])
            btn1.configure(text=c_l[card_num][card_num])
            btn3.configure(text=c_l[card_num][card_num])
            btn4.configure(text=c_l[card_num][card_num])
        except IndexError:
            root.destroy()

    def menu_bar():
        # Menu bar options
        menu = Menu(root)
        reset = Menu(menu, tearoff=0)

        # Menu functions
        def res():
            global card_num, btn1, btn2, btn3, btn4, question_label
            card_num = 0
            question_label.configure(text=c_l[0][0])
            btn1.configure(text=c_l[0][0])
            btn2.configure(text=c_l[0][0])
            btn3.configure(text=c_l[0][0])
            btn4.configure(text=c_l[0][0])

        reset.add_command(label='Reset', command=res)
        menu.add_cascade(label='File', menu=reset)

        menu.add_command(label='Edit')
        root.config(menu=menu)

    def top():
        global question_label
        top_color = 'royalblue'
        # Creates/place root container
        top_frame = Frame(root, bg=top_color, width=600, height=225)
        top_frame.grid(row=0, sticky='wens', padx=5, pady=(5, 0))
        top_frame.grid_rowconfigure(0, weight=1)
        top_frame.grid_columnconfigure(0, weight=1)

        # Place top frame widgets
        question_label = Label(top_frame, bg=top_color, text='Hello World', font=('Arial Bold', 30))
        question_label.grid(row=0, column=0)

    def bottom():
        bottom_color = 'lightsteelblue'
        bottom_frame = Frame(root, bg=bottom_color, width=600, height=125)
        bottom_frame.grid(row=1, sticky='wens', padx=5, pady=5)
        bottom_frame.grid_rowconfigure(0, weight=1)
        bottom_frame.grid_rowconfigure(1, weight=1)
        bottom_frame.grid_columnconfigure(0, weight=1)
        bottom_frame.grid_columnconfigure(1, weight=1)

        def buttons():
            global btn1, btn2, btn3, btn4
            button_color = 'blue'
            button_color_active = 'light blue'

            # Creates bottom widgets
            btn1 = Button(bottom_frame, text=c_l[card_num][card_num], bg=button_color,
                          activebackground=button_color_active, command=clicked_correct)
            btn2 = Button(bottom_frame, text=c_l[0][card_num], bg=button_color,
                          activebackground=button_color_active, command=clicked_incorrect)
            btn3 = Button(bottom_frame, text=c_l[0][card_num], bg=button_color,
                          activebackground=button_color_active, command=clicked_incorrect)
            btn4 = Button(bottom_frame, text=c_l[0][card_num], bg=button_color,
                          activebackground=button_color_active, command=clicked_incorrect)

            # Place top frame widgets
            btn1.grid(row=0, column=0)
            btn2.grid(row=0, column=1)
            btn3.grid(row=1, column=0)
            btn4.grid(row=1, column=1)
        buttons()

    top()
    bottom()
    center_window(600, 350)
    menu_bar()
    root.mainloop()


class Flashcard:
    def __init__(self, q, a, x, y, z, num):
        self.q = q
        self.a = a
        self.x = x
        self.y = y
        self.z = z
        self.num = num


c_l = [
    ['What is my first name?', 'Robert', 'Bobert', 'Trebor', 'Bortre'],
    ['What is my middle name?', 'James', 'Ames', 'Semaj', 'Majse'],
    ['What is my last name?', 'Johnson', 'Sonjohn', 'Johnston', 'Smith'],
    ['What is my suffix?', 'III', 'IV', 'PhD', 'Esq']
]

card1 = Flashcard(q=c_l[0][0], a=c_l[0][1], x=c_l[0][2], y=c_l[0][3], z=c_l[0][4], num=1)
card2 = Flashcard(q=c_l[1][0], a=c_l[1][1], x=c_l[1][2], y=c_l[1][3], z=c_l[1][4], num=2)

if __name__ == '__main__':
    main()
# end of file
\$\endgroup\$

Your Answer

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

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