I am a beginner playing around with Python and Tkinter. I wrote this program to draw random names given the students in my classes. I tried to follow PEP 8, but I was wondering if there are any stylistic problems here and also if there is any way to condense the code without affecting the working of the program (as I've finally gotten it to work like I want it to.)

from random import *
from Tkinter import *

class App:

    def __init__(self, master):
        self.frame = Frame(master, height=500, width=500)
        self.Results = Message(self.frame, text="", width=400, font="Courier, 32")
        self.Results.grid(row=1, column=1)                           
        self.b1 = Button (self.frame, text = "Get Random Name: Science 7", width=50,
                          command = self.s7_Names, bg="#6ddd58")
        self.b1.grid(row=2, column=1)
        self.b2 = Button (self.frame, text = "Get Random Name: Science 8",width=50,
                       command = self.s8_Names, bg="#75c0d2")
        self.b2.grid(row=3, column=1)
        self.b3 = Button (self.frame, text = "Get Random Name: Math 10",width=50,
                          command = self.m10_Names, bg="#ffb9ec")
        self.b3.grid(row=4, column=1)
        self.Quit = Button (self.frame, text = "Quit", command=self.frame.quit)
        self.Quit.grid (row=5, column=1)

    def get_names(self, roster, i):
        words = roster[i]

    def draw_names(self, roster, count):
        if count == 0:
            self.mixed = sample(roster, len(roster))
            self.get_names(self.mixed, count)
            count +=1

        elif 0 < count < len(roster):
            self.get_names(roster, count)
            count +=1

            count = 0
            self.mixed = sample(roster, len(roster))
            self.get_names(self.mixed, self.count)
            self.count_s7 +=1

    def s7_Names(self):
        s7 = ['Name1', 'Name2', 'Name3', 'Name4', 'Name5', 'Name6','Name7',
             'Name 8', 'Name9','Name10', 'Name11','Name12','Naome13']
        self.draw_names(s7, self.count_s7)

    def s8_Names(self):
        s8 = ['Kid1','Kid2','Kid3','Kid4','Kid5','Kid6','Another kid']
        self.draw_names(s8, self.count_s8)

    def m10_Names(self):
        m10 = ['Student1', 'Student2', 'Student3', 'Student4', 'Student5'
              'Student6', 'Student7', 'Student8', 'Student9']
        self.draw_names(m10, self.count_m10)

root.title("Random Names")
app = App(root)

1 Answer 1

from random import *
from Tkinter import *

Wildcard imports are against PEP 8. I realize that from Tkinter import * is the usual form expressed in the documentation, but it is still not recommended by PEP 8.

I see s7, s8, and m10 used in many places. I know what they are only because I can see what the texts of your buttons are. Ideally, every line of code should be clear without any context. I would use names that are more clear. Also, self.m is not a very descriptive name. I would suggest that you use a better one, but I see that it isn't used anywhere except its definition. Therefore, it should be taken out completely.

Your equals signs are a little strange. I give you two rules from PEP 8:

Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=,-= etc), comparisons (==,<,>,!=,<>,<=,>=,in,not in,is,is not), Booleans (and,or,not).


Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.

You seem to have those two rules switched up. You have, for example, self.count_s7=0, but self.b1 = Button(..., text = ...). Mind you, I mention = specifically, but you also break the rule with +=.

Your capitalization is a little messed up. From PEP 8 on Method Names and Instance Variables:

Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

You break that rule with self.Results and self.Quit.

get_names doesn't seem to be a fitting name. It doesn't return anything; it sets the text of self.Results. Perhaps set_names would be more appropriate?

I don't like that you define the class names within the methods. What you could do, actually, is make your __init__ method take a dictionary as an argument with the class names for keys and the student names for values. Then, for each key-value pair, define a button and a random color and give command as just a single method. It's fairly hard to explain without code, so I'll give an example:

def __init__(self, master, students):
    i = 2
    for class_name, student_names in students.iteritems():
        students[class_name] = [student_names, 0]
        button = Button(self.frame,
            text='Get Random Name: {}'.format(class_name), width=50,
            command=lambda name=class_name: self.draw_from_class(name),
            bg='#' + hex(randint(16777215))[2:] # 16777215 is FFFFFF
        button.grid(row=i, column=1)
        i += 1

def draw_from_class(self, name):
    class_info = self.students[name]

You could then do:

app = App(root, {
    'Science 7': ['Name1', 'Name2', ...],
    'Science 8': ['Kid1', 'Kid2', ...],
    'Math 10': ['Student1', 'Student2', ...],

That way, it is very easy to add more classes.

It is generally a good idea to use if __name__ == '__main__' for your module-level code if you don't want it to run when this file is imported.


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.