Is this Python/Pygame that draws a maze well structured?

Question

I'm not very confident about this piece of code I created, it basically takes a file like this:

##    #
## ## #
#B #  #
# ## ##
     ##
A######

Then proceeds to draw a maze in the window. The class Maze has 3 variables: columns, rows and maze_layout (a matrix each location is a character) example 2x2 maze: [["#" "A"],["#","B"]].

The code is this:

import pygame
import sys
import os

# Size of each square on the Maze
MAZE_SQUARESIZE = 40

# Colors
GREEN = (34, 139, 34)
ROSERED = (180, 75, 95)
DARKGRAY = (69, 69, 69)
WHITE = (255, 255, 255)
BLACK = (0, 0, 0)
DARKPURPLE = (48, 25, 52)

    # Creates a Maze with pygame
    class Maze:
        def __init__(self, filename):
            if not os.path.isfile(filename):
                raise FileNotFoundError("File not found:", filename)
            with open(filename, 'r') as file:
                self.maze_layout = [list(line.rstrip('\n')) for line in file.readlines()]
                self.cols = len(self.maze_layout[0])
                self.rows = len(self.maze_layout)
        
        def display_maze(self, screen):
            self.draw_grid(screen)
            while True:
                for event in pygame.event.get():
                    if event.type == pygame.QUIT:
                        pygame.quit()
                        sys.exit()
                pygame.display.update()
        
        # Drawing functions for maze
        def draw_grid(self, window):
            for x in range(self.rows):
                for y in range(self.cols):
                    rect = pygame.Rect(y * MAZE_SQUARESIZE, x * MAZE_SQUARESIZE, MAZE_SQUARESIZE, MAZE_SQUARESIZE)
                    if self.maze_layout[x][y] == "#":
                        pygame.draw.rect(window, DARKPURPLE, rect)
                    elif self.maze_layout[x][y] == " ":
                        pygame.draw.rect(window, WHITE, rect, 1)
                    elif self.maze_layout[x][y] == "A":
                        pygame.draw.rect(window, ROSERED, rect)
                    elif self.maze_layout[x][y] == "B":
                        pygame.draw.rect(window, GREEN, rect)
                    
    def main():
        pygame.init()
        filename = "maze1.txt"
        maze = Maze(filename)
        screen = pygame.display.set_mode((maze.cols * MAZE_SQUARESIZE, maze.rows * MAZE_SQUARESIZE))
        maze.display_maze(screen)
        pygame.quit()
    
    if __name__ == "__main__":
        main()

If someone could find ways to improve this code, I would be very grateful. I'm open to any sort of criticism.

\$\begingroup\$ line 17 class Maze: IndentationError: unexpected indent \$\endgroup\$

ggorlen
– ggorlen

2024-04-19 12:14:28 +00:00
Commented Apr 19, 2024 at 12:14 — ggorlen
– ggorlen, Commented Apr 19, 2024 at 12:14

J_H · Accepted Answer · 2024-04-20 18:33:30Z

The structure looks great. In particular, each method is small, and its single responsibility is clear. Also there's a nice __main__ guard, so unit tests can safely import this module.

EAFP over LBYL

    if not os.path.isfile(filename):
        raise FileNotFoundError("File not found:", filename)
    with open(filename, 'r') as file:

In the python community we often say it's easier to ask forgiveness than permission -- no need to look before you leap. Just let the exception happen.

Here, we could delete the {if, raise} entirely, since open() will raise if need be. Strictly speaking, there is a race condition in the OP code; after the .isfile() call succeeds, the filesystem could still rm the file and the open fails. Which makes us wonder if the "extra" check is helping at all. And we've not even considered permission errors like a root-owned text file that isn't world readable.

The diagnostic error message does not appear to be correctly formatted:

FileNotFoundError: [Errno File not found:] maze1.txt

You're passing in (str, str), but I think in the multiple-arg case it wants (int, str).

anonymous parameter

    filename = "maze1.txt"
    maze = Maze(filename)

Given that we won't use that filespec again, we would usually just write

    maze = Maze("maze1.txt")

or

    maze = Maze(Path("maze1.txt"))

command line arg

Consider doing an import of typer, and then a signature of def main(filename): or even def main(filename="maze1.txt"): would offer convenient CLI access to multiple maze files.

pathlib

Use this module when you can. It's more descriptive to pass in a Path than to pass in a str.

It's more convenient to ask if filename.exists(): than to rely on the ancient os.path module.

BTW, mode='r' is the default on open() and may be elided.

annotation

The identifiers we see in your Public API, such as filename and screen, are terrific. They are very clear and self-explanatory. But do consider spelling out the type, such as filename: Path. Then the mypy linter can help you with verifying that callers are passing the right params in the right order.

And while you're linting, might as well run black and isort over your code every now and again, plus the occasional ruff check.

Kudos on offering those .cols and .rows attributes to the caller.

docstring

# Creates a Maze with pygame
class Maze:

Prefer to write that as a """docstring""" immediately after the class line.

Similarly for the draw_grid() comment.

performance

That while True: loop is burning more CPU cycles than necessary.

Simplest fix is to pause for 100 msec:

from time import sleep
...
    while True:
        sleep(0.1)
        ...

A more sophisticated approach would rely on pygame.event.wait(), as that lets us specify a suitable timeout value.

Standard idiom would be to use Clock.tick(10) if we want to sleep 100 msec to ensure a window close event will soon be seen.

kwarg

This is perfectly nice as-is.

    pygame.draw.rect(window, WHITE, rect, 1)

But consider spelling out ..., rect, width=1) for the benefit of the Gentle Reader who perhaps hasn't memorized the order of that method's optional args.

mapping

We could DRY up some of those repeated elifs with a dict:

grid_appearance = {
    "#": DARKPURPLE,
    " ": WHITE,
    "A": ROSERED,
    "B": GREEN,
}

We could add the width detail, as well, for retrieval with tuple unpack:

grid_appearance = {
    "#": (0, DARKPURPLE),
    " ": (1, WHITE),
    "A": (0, ROSERED),
    "B": (0, GREEN),
}
...
    for y ...
        rect = ...
        width, color = grid_appearance[self.maze_layout[x][y]]
        pygame.draw.rect(window, color, rect, width=width)

Kate · Accepted Answer · 2024-04-19 18:00:05Z

Inside class Maze you have:

def display_maze(self, screen):
    self.draw_grid(screen)
    while True:
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                pygame.quit()
                sys.exit()
        pygame.display.update()

pygame.quit() + sys.exit() are redundant.

A class or a package is stuff that is meant to be reused and that you import for a specific purpose, but it should stick to operating within its own scope. It shouldn't affect the execution flow of the upstream (caller) routine. It is not the responsibility of your class to exit the application.

It should be sufficient to rewrite your loop like:

while event != pygame.QUIT:
    ...

When the condition is met then the class will return and execution will resume inside the main block, where the very next statement is indeed pygame.quit() and this is the only one you need to have:

def main():
    pygame.init()
    filename = "maze1.txt"
    maze = Maze(filename)
    screen = pygame.display.set_mode((maze.cols * MAZE_SQUARESIZE, maze.rows * MAZE_SQUARESIZE))
    maze.display_maze(screen)
    pygame.quit()

Also, this can be simplified:

self.maze_layout = [list(line.rstrip('\n')) for line in file.readlines()]

Since readlines already returns a list of lines you can simply do:

self.maze_layout = file.readlines()

In case you need to run on a different environment like Windows that has different line endings, you would simply adapt the file open like this:

f = open(file,mode='r', encoding='utf8', newline='\r\n')

toolic · Accepted Answer · 2024-04-19 12:23:37Z

Overview

You've done a good job:

The code layout is good
You leveraged code written by others with the imports
Used meaningful names for classes, functions and variables

Here are some adjustments for you to consider, mainly for coding style.

Indentation

When I ran the code I copied from the question, I got a syntax error due to the indentation of the class and the code that followed. If I remove the indentation, the code runs without a problem. Either you had a problem posting the code into the question, or the version of Python you used to run your code is more forgiving than mine.

Documentation

Add docstrings for the class and functions. These should briefly describe the purpose of the code.

Unused code

Since these colors are not used, simplify the code by removing them:

DARKGRAY = (69, 69, 69)
BLACK = (0, 0, 0)

Lint check

pylint recommends moving the standard imports for os and sys before import pygame.

Input checking

Consider adding some checking of the input file contents. The code expects the following 4 characters: A, B, # and space. For example, what should happen if the input file has the character C.

Stack Exchange Network

Is this Python/Pygame that draws a maze well structured?

3 Answers 3

EAFP over LBYL

anonymous parameter

command line arg

pathlib

annotation

docstring

performance

kwarg

mapping

Overview

Indentation

Documentation

Unused code

Lint check

Input checking

You must log in to answer this question.

Hot Network Questions

Is this Python/Pygame that draws a maze well structured?

3 Answers 3

EAFP over LBYL

anonymous parameter

command line arg

pathlib

annotation

docstring

performance

kwarg

mapping

Overview

Indentation

Documentation

Unused code

Lint check

Input checking

You must log in to answer this question.

Related

Hot Network Questions