Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This is a Python program that requires the user to type the colour of the text and not the name of the colour. They have a set time limit and a scoring system. If you have any improvements or suggestions I would greatly appreciate them.

#import the modules we need, for creating a GUI...
import tkinter
#...and for creating random numbers.
import random

#the list of possible colour.
colours =     ['Red','Blue','Green','Pink','Black','Yellow','Orange','White','Purple','Brown']
#the player's score, initially 0.
score=0
#the game time left, initially 30 seconds.
timeleft=30

#a function that will start the game.
def startGame(event):

    #if there's still time left...
    if timeleft == 30:
        #start the countdown timer.
        countdown()

    #run the function to choose the next colour.
    nextColour()

#function to choose and display the next colour.
def nextColour():

    #use the globally declared 'score' and 'play' variables above.
    global score
    global timeleft

    #if a game is currently in play...
    if timeleft > 0:

        #...make the text entry box active.
        e.focus_set()

        #if the colour typed is equal to the colour of the text...
        if e.get().lower() == colours[1].lower():
            #...add one to the score.
            score += 1

        #clear the text entry box.
        e.delete(0, tkinter.END)
        #shuffle the list of colours.
        random.shuffle(colours)
        #change the colour to type, by changing the text _and_ the colour to     a random colour value
        label.config(fg=str(colours[1]), text=str(colours[0]))
        #update the score.
        scoreLabel.config(text="Score: " + str(score))

#a countdown timer function. 
def countdown():

    #use the globally declared 'play' variable above.
    global timeleft

    #if a game is in play...
    if timeleft > 0:

        #decrement the timer.
        timeleft -= 1
        #update the time left label.
        timeLabel.config(text="Time left: " + str(timeleft))
        #run the function again after 1 second.
    timeLabel.after(1000, countdown)

#create a GUI window.
root = tkinter.Tk()
#set the title.
root.title("TTCANTW")
#set the size.
root.geometry("375x200")

#add an instructions label.
instructions = tkinter.Label(root, text="Type in the colour of the words,     and not the word text!", font=('Helvetica', 12))
instructions.pack()

#add a score label.
scoreLabel = tkinter.Label(root, text="Press enter to start", font=    ('Helvetica', 12))
scoreLabel.pack()

#add a time left label.
timeLabel = tkinter.Label(root, text="Time left: " + str(timeleft), font=    ('Helvetica', 12))
timeLabel.pack()

#add a label for displaying the colours.
label = tkinter.Label(root, font=('Helvetica', 60))
label.pack()

#add a text entry box for typing in colours.
e = tkinter.Entry(root)
#run the 'startGame' function when the enter key is pressed.
root.bind('<Return>', startGame)
e.pack()
#set focus on the entry box.
e.focus_set()

#start the GUI
root.mainloop()
share|improve this question
4  
I have rolled back the last edit. Please see what you may and may not do after receiving answers. You could also come talk to us in chat if you want to learn more about editing in general. – Pimgd Feb 19 at 16:22
2  
you only gave us two hours to submit a review before selecting an answer. I think you should wait a little longer, to give more people a chance to notice your question. – Bryan Oakley Feb 19 at 16:26
up vote 8 down vote accepted

Now, I can't code in python or very well read it. But you have a bunch of comments, like these

#the list of possible colour.
colours =     ['Red','Blue','Green','Pink','Black','Yellow','Orange','White','Purple','Brown']

#a function that will start the game.
def startGame(event):

#add an instructions label.
instructions = tkinter.Label(root, text="Type in the colour of the words,     and not the word text!", font=('Helvetica', 12))

#add a score label.
scoreLabel = tkinter.Label(root, text="Press enter to start", font=    ('Helvetica', 12))

Even being someone that can't read the code, telling me that you're adding an instructions label isn't needed - I see you adding instructions.

My point being that a lot of your code speaks for itself (yay!) and you don't need those comments. Comments should explain why you do something, rather than what you do or how you do it. It will become easier to read and feel less cluttered.

share|improve this answer
    
Thank you for your feedback, however, the reason for the comments, is that I am currently doing GCSE computing, and we are expected to comment on every line, just to make it clear and show that we know exactly what we are doing and not plucking bits of code here and there off the internet... – D.C Feb 19 at 16:21
    
Fair enough, well then - your code is pretty well readable. – Raystafarian Feb 19 at 16:22

Text widget indexes

e.delete(0, tkinter.END)

The canonical first character of a text widget is the index "1.0", not the integer zero. Your code still works, but only by accident.


Layout out your widgets

instructions = tkinter.Label(...)
instructions.pack()
...
scoreLabel = tkinter.Label(...)
scoreLabel.pack()

I personally think it would make your code easier to read and maintain if you separate your widget creation from widget layout. Also, I think it's a best practice to give explicit options to pack The zen of python says that explicit is better than implicit.

instructions = tkinter.Label(...)
scoreLabel = tkinter.Label(...)
...
instructions.pack(side="top", fill="x")
scoreLabel.pack(side="top", fill="x")
...

Comments

#import the modules we need, for creating a GUI...
...
#...and for creating random numbers.
...
#create a GUI window.
...
#set the title.
...
#set the size.

Comments like that don't tell me anything that the code isn't already telling me. Comments should explain why you're doing something, and only if it's not obvious. Otherwise they just add noise.


Repeated code for fonts

instructions = tkinter.Label(..., font=('Helvetica', 12))
scoreLabel = tkinter.Label(..., font=    ('Helvetica', 12))
timeLabel = tkinter.Label(..., font=    ('Helvetica', 12))

You hare hard-coding a font in three different places. There are a couple solutions. The simplest is to set a global variable, so you set it once at the start of the program and then use the variable everywhere you want a custom font.

Tkinter has a better mechanism, however. You can create a custom font by creating an instance of tkinter.font.Font. For example:

import tkinter.font
...
app_font = tkinter.font.Font(family="helvetica", size=32)
...
label = tkinter.Label(..., font=app_font, ...)

The really awesome thing about these font objects is that if you change the font while the program is running (eg: app_font.configure(size=48)`), every widget that uses that font will automatically update to use the new size.


Use classes

My final recommendation is to make use of classes. For an example of how to structure your tkinter programs, see http://stackoverflow.com/questions/17466561/best-way-to-structure-a-tkinter-application/17470842#17470842

share|improve this answer

Good code is easy to change because it is DRY, each important information is stated once and only once.

Trying to make the game last more (or less)

I changed the timeleft at the start:

timeleft=15

But it did not work because you repeated that information afterwards in startGame:

if timeleft == 30:

So the time was not running out, it was remaining at \$15\$.

Trying to change the update frequency of the timer

To give more precise information to the player, i wanted to make the label change one time each 0.1 seconds pass:

I changed

timeleft -= 1

to

timeleft -= 0.1

But it did not work, because the label was being decreased by 0.1 each second, because the information was repeated at:

timeLabel.after(1000, countdown)

Trying to change to bigger (or smaller) fonts

To allow the user to see the text without much squinting I wanted to have bigger writing.

I changed:

instructions = tkinter.Label(root, text="..snip..", font=('Helvetica', 12))

to:

instructions = tkinter.Label(root, text="..snip..", font=('Helvetica', 20))

But only one of the labels changed as you rewrote the exact same font information 3 times, once for each small label.

To allow easy functionality changes, use CONSTANTS

For example:

TOTAL_TIME = 30

And then you use TOTAL_TIME two times. (The same can be done for the other values).

This will allow to tweak the code functionality much faster.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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