3
\$\begingroup\$

Hello everyone

Last week I've started creating a little project for fun - TapType Game, I know there is a lot of things to make better, but what about my solution, can you give me some general tips, how to make it better? Thanks in advance, and have a nice night.

import tkinter as tk
from tkinter import ttk
from typing import List, TextIO
import time
import pygame
import random

class FastTypeGame:
    """A class for the Fast Type Game GUI and its game logic."""
    
    def __init__(self) -> None:
        """Initialize the game window, set up the UI and start the game."""
        self.window = tk.Tk()
        self.window.title("Fast Type Game")
        self.setup_window_size()
        self.set_background_photo("background.png")
        self.words: List[str] = []
        self.word_index: int = 0
        self.time_limit: int = 0
        self.start_time: float = 0
        self.elapsed_time: float = 0
        self.wpm: float = 0
        self.accuracy: float = 0
        self.typed_words: int = 0
        self.correct_words: int = 0
        self.incorrect_words: int = 0
        self.typed_characters: int = 0
        self.correct_characters: int = 0
        self.incorrect_characters: int = 0
        self.word_length: int = 0
        self.game_over: bool = False
        self.load_words()
        self.init_ui()
        self.window.mainloop()

    def setup_window_size(self, ) -> None:
        """Set up the window size."""
        window_width = 500
        window_height = 1000
        self.window.geometry(f"{window_width}x{window_height}")

    def load_words(self, file: TextIO = "words.txt") -> None:
        """Load words from words.txt file and store them in a list."""
        with open(file, "r") as f:
            self.words = f.read().splitlines()
            # shuffle the words
            random.shuffle(self.words)
            
    def set_time_limit(self, time_limit: int) -> None:
        """Set the time limit for the game."""
        self.time_limit = time_limit
        
    def set_background_photo(self, photo: str = "backgrounds\\background.png") -> None:
        """Set the background photo for the game."""
        try:
            self.background_photo = tk.PhotoImage(file=photo)
            self.background_label = tk.Label(self.window, image=self.background_photo)
            self.background_label.place(x=0, y=0, relwidth=1, relheight=1)
        except tk.TclError:
            print("Unable to load background photo. Setting white background instead.")
            self.background_label = tk.Label(self.window, bg="white")
            self.background_label.place(x=0, y=0, relwidth=1, relheight=1)

    def init_ui(self) -> None:
        """Set up the UI elements."""
        # Setup UI
        self.game_name_label = tk.Label(self.window, text="💻 TapType Game 💻", font=("Helvetica", 37))
        self.game_name_label.pack(pady=30)

        self.separator = ttk.Separator(self.window, orient='horizontal')
        self.separator.pack(fill='x')

        self.word_label = tk.Label(self.window, text="", font=("Helvetica", 35))
        self.word_label.pack(pady=30)

        self.input_field = tk.Entry(self.window, font=("Helvetica", 25))
        self.input_field.pack(pady=30)
        self.input_field.bind("<Key>", self.on_key_pressed), self.input_field.focus()

        self.timer_label = tk.Label(self.window, text="", font=("Helvetica", 20))
        self.timer_label.pack(pady=30)

        self.wpm_label = tk.Label(self.window, text="WPM: 0", font=("Helvetica", 25))
        self.wpm_label.pack(pady=30)

        self.accuracy_label = tk.Label(self.window, text="Accuracy: 0%", font=("Helvetica", 25))
        self.accuracy_label.pack(pady=30)

        self.typed_characters_label = tk.Label(self.window, text="Typed Characters: 0", font=("Helvetica", 20))
        self.typed_characters_label.pack(pady=30)

        self.correct_characters_label = tk.Label(self.window, text="Correct Characters: 0", font=("Helvetica", 20))
        self.correct_characters_label.pack(pady=30)

        self.incorrect_characters_label = tk.Label(self.window, text="Incorrect Characters: 0", font=("Helvetica", 20))
        self.incorrect_characters_label.pack(pady=30)

        # Start game
        self.start_game()

    def start_game(self) -> None:
        """Reset game variables and start the game."""
        self.start_time = time.time()
        self.elapsed_time = 0
        self.wpm = 0
        self.accuracy = 0
        self.typed_words = 0
        self.correct_words = 0
        self.incorrect_words = 0
        self.correct_characters = 0
        self.incorrect_characters = 0
        self.word_index = 0
        self.time_limit = 5
        self.max_len = len(self.words)
        self.new_word()
        self.word_length = len(self.words[self.word_index])
        self.update_ui()

    def new_word(self) -> None:
        """Get the next word from the list and set up the UI for it."""
        self.word_label.configure(text=self.words[self.word_index])
        self.input_field.delete(0, tk.END)
        self.word_length = len(self.words[self.word_index])

    def check_word(self) -> None:
        """Check if the typed word is correct and update game variables accordingly."""
        self.typed_words += 1
        if self.input_field.get() == self.words[self.word_index]:
            self.correct_words += 1
        else:
            self.incorrect_words += 1

        self.word_index += 1

        if self.word_index >= len(self.words):
            self.end_game()
        else:
            self.new_word()

    def calculate_wpm(self) -> float:
        """Calculate words per minute."""
        if self.elapsed_time > 0:
            self.wpm = (self.typed_characters / 5) / (self.elapsed_time / 60)
        return self.wpm

    def calculate_accuracy(self) -> float:
        """Calculate accuracy rate."""
        if self.typed_characters > 0:
            self.accuracy = (self.correct_characters / self.typed_characters) * 100
        return self.accuracy

    def on_key_pressed(self, event: tk.Event) -> None:
        """Handle key press events from the input field."""
        if event.keysym == "Return":
            self.check_word()
            self.new_word()
        elif event.keysym == "BackSpace":
            self.typed_characters += 1
            self.incorrect_characters += 1
        elif event.keysym == "Escape":
            self.end_game()
        else:
            self.typed_characters += 1
            self.check_character()
        self.update_ui()

    def check_character(self) -> None:
        """Check if the typed character is correct."""
        typed_word = self.input_field.get()
        generated_word = self.words[self.word_index]
        if typed_word == generated_word[:len(typed_word)]:
            self.correct_characters += 1
        else:
            self.incorrect_characters += 1

    def update_ui(self) -> None:
        """Update the UI elements."""
        self.wpm = self.calculate_wpm()
        self.accuracy = self.calculate_accuracy()
        self.elapsed_time = time.time() - self.start_time

        self.timer_label.configure(text=f"Time: {int(self.time_limit - self.elapsed_time)}")
        self.wpm_label.configure(text=f"WPM: {int(self.wpm)}")
        self.accuracy_label.configure(text=f"Accuracy: {int(self.accuracy)}%")
        self.typed_characters_label.configure(text=f"Typed Characters: {int(self.typed_characters)}")
        self.correct_characters_label.configure(text=f"Correct Characters: {int(self.correct_characters)}")
        self.incorrect_characters_label.configure(text=f"Incorrect Characters: {int(self.incorrect_characters)}")

        if self.elapsed_time > self.time_limit:
            self.end_game()
        self.window.after(100, self.update_ui)

    def end_game(self) -> None:
        """End the game and show the game over screen."""
        self.wpm = self.calculate_wpm()
        self.accuracy = self.calculate_accuracy()
        self.wpm_label.configure(text=f"WPM: {int(self.wpm)}")
        self.accuracy_label.configure(text=f"Accuracy: {int(self.accuracy)}%")
        self.word_label.configure(text="Game Over")
        # Show game over screen
        pygame.init()
        game_over_screen = pygame.display.set_mode((500, 500))
        pygame.display.set_caption("Game Over")

        font = pygame.font.Font("freesansbold.ttf", 24)
        text = font.render(f"WPM: {int(self.wpm)}", True, (255, 255, 255))
        text_rect = text.get_rect()
        text_rect.center = (250, 250)
        game_over_screen.blit(text, text_rect)

        text = font.render(f"Accuracy: {int(self.accuracy)}%", True, (255, 255, 255))
        text_rect = text.get_rect()
        text_rect.center = (250, 300)
        game_over_screen.blit(text, text_rect)
        
        text = font.render("Press ESC to quit", True, (255, 255, 255))
        text_rect = text.get_rect()
        text_rect.center = (250, 350)
        game_over_screen.blit(text, text_rect)
        pygame.display.update()

        game_over = False
        while not game_over:
            for event in pygame.event.get():
                if event.type == pygame.QUIT:
                    game_over = True
                    pygame.quit()
                    quit()
                # if user click ESC key, quit the game
                elif event.type == pygame.KEYDOWN:
                    if event.key == pygame.K_ESCAPE:
                        game_over = True
                        pygame.quit()
                        quit()
        
    @staticmethod
    def run_game() -> None:
        """Run the game."""
        game = FastTypeGame()
        game.window.mainloop()
        

if __name__ == "__main__":
    FastTypeGame.run_game()

Using Python 3.10 in this project.

\$\endgroup\$

1 Answer 1

3
+100
\$\begingroup\$

Interesting little game.

It appears you have been using black or at least flake8; I am duly impressed with the obvious care that went into this code.


class FastTypeGame:
    """A class for the Fast Type Game GUI and its game logic."""

Thank you for the docstring. It could be tightened up a bit, omitting the redundant "A class". Let the code speak for itself, and put higher level insights in the English prose.

This is a useful docstring, as in the course of telling us what belongs here it also tells us that KitchenSink and similar craziness does not belong here. It is saying to future maintainers: "Go find another class for such things, don't clutter this one with stuff that does not belong."


    def __init__(self) -> None:
        """Initialize the game window, set up the UI and start the game."""
        self.window = tk.Tk()
        ...
        self.load_words()
        self.init_ui()
        self.window.mainloop()

The docstring is accurate. My objection is that it is explaining that the code does "too much". I would prefer to see the ctor set a bunch of variables, and then the caller requests UI behavior such as mainloop.

Let's consider load_words. It is nice and simple, quite self-contained. Imagine you wanted to write a unit test for it. You can't, can you? I mean, you need to have an object instance in hand in order to call it, right? How am I going to get one? Via the constructor. But it doesn't return, it runs the main loop.


There are quite a few variables in the constructor. Perhaps we need all of them, and they're all in the right place, at the right level. But I wonder if some of them are derived quantities, like wpm and accuracy, which could be @property methods.

Also, some of them might be grouped into an InputWords object, while others might be part of a TypingResults object.


    def setup_window_size(self, ) -> None:

style nit: Please avoid the trailing , comma.


    def load_words(self, file: TextIO = "words.txt") -> None:

The optional type hint there isn't right; it should be str. I can see changing it to Path. If you wish to support e.g. TextIOWrapper or StringIO, maybe for unit testing, then you'll need different logic in the body.

            # shuffle the words

Please delete this redundant comment. It is saying what the code already said eloquently.

This is a nice little helper, very compartmentalized. It introduces a local variable f which quickly goes out of scope, good. We evaluate it for side effects, on self.words, which the docstring didn't quite spell out. Consider converting this to a @staticmethod returning a list, so the caller does self.words = self.load_words(). That would make it crystal clear where the result went. And it would ease the task of writing a unit test for this.


Maybe there's good reason for this setter? set_time_limit

I'm afraid I am not yet seeing it. Recommend you delete the method and let the caller just set the variable.

Or get around to an item you might have had on your TODO list, like checking that proposed value falls within some range.


    def set_background_photo(self, photo: str = "backgrounds\\background.png") -> None:

Uggh, backwhacks! Please spell it in one of these ways:

  1. "backgrounds/background.png" (preferred)
  2. r"backgrounds\background.png"

The raw-string notation lets us stop worrying about crazy escaping.

Using str is fine; you might possibly prefer to use Path here.

The exception handler worked brilliantly when I ran this on my laptop.


        self.start_game()

    def start_game(self) -> None:

Excellent! init_ui was getting to be a bit long, so you broke it up.

Now, maybe init_ui should do just One Thing, and caller should be responsible for invoking both init_ui() and start_game(). But I will take wins where I can find them.


In new_word, consider destructively .pop()ping off the next word to display. Then you could dispense with maintaining an index.


            self.correct_words += 1
        else:
            self.incorrect_words += 1

I do like the parallel construction. But maybe we don't need a separate incorrect_words attribute, and could have a @property dynamically derive it from typed_words minus the correct ones?


Oh, look at that, in calculate_wpm! You're making my argument for me. There is almost no motivation for storing self.wpm, given the several dynamic values it depends upon. This is a natural fit for a @property method. Similarly for calculate_accuracy.

The input file is named "words.txt". I confess I don't exactly understand what should go in it. Initially I added three vocabulary words, one per line. Then once I saw how the game plays, I switched to three short lines of text, e.g. "the quick brown fox". Usually WPM is for SPACE delimited words, not CR delimited words; it's easier to rapidly hit the space bar than to hit RETURN.

Suppose we go with the latter interpretation, and we fill the file with a phrase on each line. Then I am skeptical the expression self.typed_characters / 5 is what we want. Probably we should divide by 6, right? To account for SPACE between the nominal 5-letter words?


        elif event.keysym == "BackSpace":
            self.typed_characters += 1
            self.incorrect_characters += 1

Wow, harsh game rule! Ok. I'm not expert on WPM measurements. I'm just surprised at such a penalty. I figure the poor typist already suffered some delay typing a character and then typing BACKSPACE.


Here is a UX remark, rather than a code remark. In check_character(), you might consider flashing or otherwise updating the screen so the typo is immediately apparent to the typist. (If you're reading the words then you're not reading "Accuracy: NN".)


In end_game we display several lines of text. It seems we don't have a super convenient API for doing that. Maybe there's a better pygame idiom for this? Or at a minimum, we should definitely have a tiny helper that blits some unicode text to an (x, y) location.

It seems the while not game_over: loop could be more robust. Under MacOS we have a TK window and a pygame window, each with a red "close" control in the upper left. Closing the pygame window in that way (instead of ESC) works great. Closing the original TK window in that way leads to an uncaught error.


Overall?

This is maintainable code which achieves its design goals.

Be wary of coupling, of a profusion of object attributes. Prefer short-lived local variables where possible. Prefer to compute derived quantities, rather than introduce another redundant variable. Group related variables in their own object.

This codebase has clearly focused on breaking out helpers, and that's good. There's still a few more that might be broken out.

\$\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.