5
\$\begingroup\$

From my previous question, I got great feedback which helped improve my code which I submitted. I went back and built upon that and came back to see if my code is an improvement.

try:
    from tkinter import *

except ImportError:
    from Tkinter import *

import time
import random

with open("NAME_FILE.txt") as f:
    name = "".join(f.readlines())

with open("HIGH_SCORE.txt") as f:
    high_score = "".join(f.readlines())

with open("WORDS_FILE.txt") as f:
    WORDS = list(map(lambda s: s.strip('\n'), f.readlines()))

time_score = 0 
word_count = 0
skips = 0

def shuffle():
    global word, time_score, start_time
    go_btn.pack_forget()
    start_time = time.time()
    while word_count < 12:

        word = random.choice(WORDS)

        label.config(text=str(word))
        time_score_label.config(text="Time: " + str(time_score) + "s")      
        return

    if word_count == 12:
        end()

def restart(event):
    global word_count, skips, time_score
    start_time = time.time()

    word_count = 0
    skips = 0
    time_score = 0

    label.config(text="")
    time_score_label.config(text="Time: " + str(time_score) + "s")
    word_counter.config(text="Words: " + str(word_count))
    skip_counter.config(text="Skips: " + str(skips))
    wrong_label.config(text="")

    high_score_label.pack_forget()
    enter.pack_forget()
    time_score_label.pack_forget()
    word_counter.pack_forget()
    skip_counter.pack_forget()
    wrong_label.pack_forget()
    entry.pack_forget()

    time_score_label.pack()
    word_counter.pack()
    skip_counter.pack()
    wrong_label.pack()
    entry.pack()

    entry.focus_set()
    root.bind("<Return>", check)
    shuffle()

def no(event):
    pass

def new_score():
    global start_time, time_score, high_score
    if float(time_score) < float(high_score):
        high_score_label.pack()
        high_score = time_score
        enter.pack()

    root.bind("<Return>", no)

def write():
    with open("HIGH_SCORE.txt", 'w') as f:
        f.write(str(time_score))
    with open("NAME_FILE.txt", 'w') as a:
        a.write(str(name))    

def end():
    global time_score, start_time, high_score, name

    entry.delete(0, END)
    time_score_label.pack_forget()
    label.config(text="Finished")
    word_counter.config(text="Well done you did it in " + str(time_score) +"s")
    new_score()

    root.bind("<Return>", no)

def reset(event):
    with open("NAME_FILE.txt", 'w') as f:
        f.write("A really slow typer")

    with open("HIGH_SCORE.txt") as a:
        a.write("60.0")

def update():
    global time_score, start_time, high_score, name
    name = entry.get()
    body.config(text="The aim of the game is to type whatever you see on the"
                     " screen as quickly as possible!" + '\n'"You will be"
                     " shown 12 words at random and you have to type them all"
                     " correctly as shown" + '\n' + "The current world record"
                     " is " + str(high_score) + "s, by " + name + '\n' +
                     "When you are ready, press go to begin!" + '\n' + "Press"
                     " [SHIFT] to reset the scores.")
    write()
    entry.delete(0, END)

def check(event):
    global time_score, high_score, name, word_count, skips, word, start_time

    if entry.get().lower() == word.lower():
        update_right()

    elif entry.get().lower() != word.lower():
        update_wrong()

    if len(entry.get()) < 1:
        update_skip()

    update_time()    
    shuffle()
    entry.delete(0, END)

def update_time():
    global time_score, start_time
    score_score = time.time() - start_time
    time_score += score_score
    time_score = round(time_score,2)

def update_skip():
    global time_score, high_score, name, word_count, skips, word, start_time

    skips += 1
    skip_counter.config(text="Skips: " + str(skips))
    wrong_label.config(text="SKIPPED!", fg='red')
    time_score_label.config(text="Time: " + str(time_score) + "s")

def update_right():
    global time_score, high_score, name, word_count, skips, word, start_time

    word_count += 1
    WORDS.remove(word)

    time_score_label.config(text="Time: " + str(time_score) + "s")
    word_counter.config(text="Words: " + str(word_count))
    wrong_label.config(text="")

def update_wrong():
    global time_score, high_score, name, word_count, skips, word, start_time
    wrong_label.config(text="WRONG!", fg='red')
    time_score_label.config(text="Time: " + str(time_score) + "s")

root = Tk()          
root.title("SpeedTypr")
root.geometry = ("750x600")

header = Label(root, text="SpeedTypr", font=('Titilium Web', 32)
                       , bg='Red')

seperator = Frame(root, height=2, width=2, bg='black')

body = Label(root, text="The aim of the game is to type whatever you see on"
                          " the screen as quickly as possible!" + '\n'
                          "You will be shown 12 words at random and you have"
                          " to type them all correctly as shown" + '\n' + 
                          "The current world record is: " + str(high_score) +
                          "s, by: " + name + '\n' + "When you are ready, press"
                          " go to begin!" + '\n' + "Press [SHIFT] to reset"
                          " the scores")

label = Label(root, font=("Helvetica", 60))

time_score_label = Label(root, text="Time: " + str(time_score) +
                                "s", font=('Helvetica', 14))

word_counter = Label(root, text="Words: " + str(word_count),
                            font =("Helvetica", 14))

skip_counter = Label(root, text="Skips: " + str(skips),
                            font =("Helvetica", 14))

wrong_label = Label(root, text="", font =("Helvetica, 14"))                                      

go_btn = Button(root, text="GO!", command=shuffle)

high_score_label = Label(root, text="Well done, you got the high score!"
                        + '\n' + "Enter your name above" + '\n' + "Press [TAB]"
                         " to restart!")

enter = Button(root, text="Enter", command=update)
entry = Entry(root)

root.bind("<Return>", check)
root.bind("<Tab>", restart)
root.bind("<Shift-L>", reset)

header.pack(fill=X)
seperator.pack(fill=X) 
body.pack()
label.pack()
time_score_label.pack()
word_counter.pack()
skip_counter.pack()
wrong_label.pack()
go_btn.pack()
entry.pack()

entry.focus_set()

root.mainloop()

if __name__ == "__main__":
    print("Functional")

I've tried to group my code into chunks and increase the amount of functions which now have (or should have) a single purpose and I now use text files to store the data.

HIGH_SCORE.txt

60.0

NAME_FILE.txt

A really slow typer

WORDS_FILE.txt

Games
Development
Keyboard
Speed
Typer
Anything
Alpha
Zealous
Accurate
Basics
Shortcut
Purpose
Window
Counter
Fortress
Modification
Computer
Science
History
Football
Basketball
Solid
Phantom
Battlefield
Advanced
Warfare
Download
Upload
Antidisestablishmentarianism
Supercalifragilisticexpialidocious
Discombobulation
Liberated
Assassin
Brotherhood
Revelation
Unity
Syndicate
Victory

Also, while testing my program before, I realised that the timer wouldn't reset after the tab key was pressed. That is now fixed. The program should now work in Python 2 aswell (correct me if I am wrong).

\$\endgroup\$
5
\$\begingroup\$

This is not a good way to read in data. You have a .txt file purely just to read in a name. You're also reliant on data being in the right form. If someone tried to put in multiple names

A really slow typer
A really fast typer
I don't type

Then you mangle them all together in name:

with open("NAME_FILE.txt") as f:
    name = "".join(f.readlines())
print(name)
# A really slow typerA really fast typerI don't type

Instead you should use JSON. Python has a builtin JSON library you can import and use. You could easily save and load data from a single .json file with it. You would just need to store them all in a dictionary, then use json.dump to write the data to a file.

import json

data = {
        'name': name,
        'high_score': high_score,
        'words': WORDS,
       }
with open(filename, 'w') as f:
    json.dump(data, f, indent=4)

(note indent is an optional parameter that will indent the levels of nesting for your json file. I used it here for readability, especially as you have relatively little data to format)

You can then easily load the data in a similar way:

import json

with open(filename) as f:
    data = json.load(f)

name = data['name']
high_score = data['high_score']
WORDS = data['words']

This will save you a lot of fixing data since you're saving it from Python in the form you'll want ot read it again. No need to reformat strings every time you open the program.

\$\endgroup\$
  • \$\begingroup\$ In my code I use WORDS.remove(word) to remove the word from the list if the user gets it right. It was fine using a text file but now using a JSON file, it permanently removes the word from the file and not the list. Any ideas? \$\endgroup\$ – Inkblot Dec 22 '15 at 12:22
4
\$\begingroup\$

I will try to give you as much feedback as I can, but I make no claim to be complete.

General

  • Add the encoding of your source code on top of your file. PEP263 recommends something like # -*- coding=utf-8 -*-.
  • You have loads of global variables. Consider refactoring your code, so that you can manage them in a central place. A class would be ideal, but a dictionary might also do the trick.
  • 12 (words?) and 60 (default highscore?) are more or less magic numbers. It would be good practice to replace them by constants with meaningful names.
  • Rethink your use of the if __name__ == '__main__': construct. Usually this little piece of code is used to get different behaviour if your code is executed as script (__name__ is __main__ in that case), from when your code is imported from other programs (__name__ is name_of_your_file).

Imports

I would not recommend to use * imports in general. It is quite hard to see which functions/objects/... come from a certain package. This is an even greater problem if someone, who maybe does not even know that package, reviews your code. In your case you could do something like

try:
    import tkinter as 
except ImportError:
    import Tkinter as tk

This allows you to use the functionality of the package simply by prepending tk. to your calls, without the need to care which module really got imported.

Functions (general)

  • Consider choosing more meaningfull names, e.g. no is not a good function name. empty_callback or something similar would be more self describing
  • Add a short documentation for your functions. PEP8 and PEP257 will give you very good guidelines and examples how to design such "docstrings", as they are called in Python.
  • Use global only if you assign to a global variable (see SO for a good example). If you just want to access them, they are in your scope with or without global. The only difference is, that with global assignments to these variables will cause the global value to change, otherwise the change will be lost after the function returns.

Functions (one by one)

If a function does not appear in the list below, I did not spot any special issue(s). The statements from the previous sections apply to them and all items in the list below likewise.

shuffle

  • As I already mentioned, consider to rename some functions. As far as I understand the code of shuffle, you are not really shuffling anything, but instead pick a random word of your list of words.
  • The use of while is not really appropriate here. The loop is only run once (at most) and can easily be replaced with an if statement. The following code should have the exact same behaviour:

    if word_count < 12:
        # your while-code here, without return
    else:
        end()
    
  • Loops are usually left with a break. return will not just leave the loop, but the whole function. In your case that will not make any difference, but you should stick with break if you want to leave loops.

restart

  • First problem with global: global word_count, skips, time_score will will change this variables at global scope, but then there is start_time = time.time(). That assignement will never leave the function.

no

See my example in Functions (general)

write

  • You can add the values to write as parameters, which allows you to use them again later, e.g. in reset.
  • Maybe the two values written to two files should go to one file as they belong together. As SuperBiasedMan already suggested in his answer, JSON could be a good way to go.
  • You should consider writing a function read as wrapper for the lines at the beginnig that read your information from file.

end

Mostly another issue with global. You do not assign anything to the variables you introced as global. start_time, high_score and name are not even used in this function.

reset

Consider a rename. The name is quite missleading, as it does not tell anything about what is actually reset.

update

  • Next inconsistency with global. You only use high_score and name anywhere in the function and just assign a new value to name.
  • There is a quite large part of code which is somehow redudant to the initalization code later.

check

  • global again.
  • Consider a more meaningful name as well.
  • Is there a specific reason the checks only include all lowercase comparisons? The words in your list all have uppercase letters in the first position.

update_time

  • global again.
  • You could save the temporary assignement simply by using

    time_score += time.time() - start_time
    

update_right

  • globals again.
  • Small typo in the functions name.

All the rest

After the definition of your functions there is a large chunk to initialize the GUI. A few comments would be very nice, so that a reviewer can spot errors, where your intention differs from your actual code, easier.

There are several redundant blocks with lots of calls to .pack(). These blocks might be pushed into a function as well.

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking your time to write some feedback and I will try to build upon it. The reason I use lowercase letters is so the user doesn't have to waste time entered a mixture of uppercase and lowercase. \$\endgroup\$ – Inkblot Dec 21 '15 at 23:18
  • 1
    \$\begingroup\$ It might be a good idea to communicate that fact to potential users. \$\endgroup\$ – AlexV Dec 21 '15 at 23:24
3
\$\begingroup\$

Looping to reduce repetition

body.pack()
label.pack()
time_score_label.pack()
word_counter.pack()
skip_counter.pack()
wrong_label.pack()
go_btn.pack()
entry.pack()

Can be written as a loop to avoid repeating .pack()

for widget in (body, label...):
    widget.pack()
\$\endgroup\$

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.