2
\$\begingroup\$

Please can I have some feedback on my Python Turtle implementation of the 15 Puzzle?

I have deliberately avoided using OOP as my aim is to develop a clear and clean coding style suitable for teaching programming at school (at a level before OOP is relevant). I also tried to avoid using globals by passing arguments, but found that doing so introduced more complexity than seemed justifiable.

I would like comments on the quality and consistency of the code, and any improvements that seem necessary to meet the criteria of "code that doesn't suck."

The images can be found here: https://github.com/Robin-Andrews/15-Puzzle-Python-Turtle

I would like confirmation that the logic is completely correct, and given that the design of the solution was entirely mine, perhaps some indication in the great scheme of things of my apparent skill level at this stage.

import turtle
import tkinter as tk
import random

NUM_ROWS = 4
NUM_COLS = 4
TILE_WIDTH = 90  # Actual image size
TILE_HEIGHT = 90  # Actual image size
FONT_SIZE = 24
FONT = ('Helvetica', FONT_SIZE, 'normal')
SCRAMBLE_DEPTH = 10

images = []
for i in range(NUM_ROWS * NUM_COLS - 1):
    file = f"number-images/{i+1}.gif"
    images.append(file)

images.append("number-images/empty.gif")


def register_images():
    global screen
    for i in range(len(images)):
        screen.addshape(images[i])


def index_2d(my_list, v):
    """Returns the position of an element in a 2D list."""
    for i, x in enumerate(my_list):
        if v in x:
            return (i, x.index(v))


def swap_tile(tile):
    """Swaps the position of the clicked tile with the empty tile."""
    global screen

    current_i, current_j = index_2d(board, tile)
    empty_i, empty_j = find_empty_square_pos()
    empty_square = board[empty_i][empty_j]

    if is_adjacent([current_i, current_j], [empty_i, empty_j]):
        temp = board[empty_i][empty_j]
        board[empty_i][empty_j] = tile
        board[current_i][current_j] = temp

        draw_board()


def is_adjacent(el1, el2):
    """Determines whether two elements in a 2D array are adjacent."""
    if abs(el2[1] - el1[1]) == 1 and abs(el2[0] - el1[0]) == 0:
        return True
    if abs(el2[0] - el1[0]) == 1 and abs(el2[1] - el1[1]) == 0:
        return True
    return False


def find_empty_square_pos():
    """Returns the position of the empty square."""
    global board
    for row in board:
        for candidate in row:
            if candidate.shape() == "number-images/empty.gif":
                empty_square = candidate

    return index_2d(board, empty_square)


def scramble_board():
    """Scrambles the board in a way that leaves it solvable."""
    global board, screen

    for i in range(SCRAMBLE_DEPTH):
        for row in board:
            for candidate in row:
                if candidate.shape() == "number-images/empty.gif":
                    empty_square = candidate

        empty_i, empty_j = find_empty_square_pos()
        directions = ["up", "down", "left", "right"]

        if empty_i == 0: directions.remove("up")
        if empty_i >= NUM_ROWS - 1: directions.remove("down")
        if empty_j == 0: directions.remove("left")
        if empty_j >= NUM_COLS - 1: directions.remove("right")

        direction = random.choice(directions)

        if direction == "up": swap_tile(board[empty_i - 1][empty_j])
        if direction == "down": swap_tile(board[empty_i + 1][empty_j])
        if direction == "left": swap_tile(board[empty_i][empty_j - 1])
        if direction == "right": swap_tile(board[empty_i][empty_j + 1])


def draw_board():
    global screen, board

    # Disable animation
    screen.tracer(0)

    for i in range(NUM_ROWS):
        for j in range(NUM_COLS):
            tile = board[i][j]
            tile.showturtle()
            tile.goto(-138 + j * (TILE_WIDTH + 2), 138 - i * (TILE_HEIGHT + 2))

    # Restore animation
    screen.tracer(1)


def create_tiles():
    """
    Creates and returns a 2D list of tiles based on turtle objects
    in the winning configuration.
    """
    board = [["#" for _ in range(NUM_COLS)] for _ in range(NUM_ROWS)]

    for i in range(NUM_ROWS):
        for j in range(NUM_COLS):
            tile_num = 4 * i + j
            tile = turtle.Turtle(images[tile_num])
            tile.penup()
            board[i][j] = tile

            def click_callback(x, y, tile=tile):
                """Passes `tile` to `swap_tile()` function."""
                return swap_tile(tile)

            tile.onclick(click_callback)

    return board


def create_scramble_button():
    global screen
    canvas = screen.getcanvas()
    button = tk.Button(canvas.master, text="Scramble", background="cadetblue", foreground="white", bd=0,
                       command=scramble_board)
    canvas.create_window(0, -240, window=button)


def main():
    global screen, board

    # Screen setup
    screen = turtle.Screen()
    screen.setup(600, 600)
    screen.title("15 Puzzle")
    screen.bgcolor("aliceblue")
    screen.tracer(0)  # Disable animation
    register_images()

    # Initialise game and display
    board = create_tiles()
    create_scramble_button()
    scramble_board()
    draw_board()
    screen.tracer(1)  # Restore animation


main()
turtle.done()
\$\endgroup\$
1
\$\begingroup\$

General suggestions:

  1. black can automatically format your code to be more idiomatic.
  2. isort can group and sort your imports automatically.
  3. flake8 with a strict complexity limit will give you more hints to write idiomatic Python:

    [flake8]
    max-complexity = 4
    ignore = W503,E203
    

    That limit is not absolute by any means, but it's worth thinking hard whether you can keep it low whenever validation fails. For example, I'm working with a team on an application since a year now, and our complexity limit is up to 7 in only one place. Conversely, on an ugly old piece of code I wrote without static analysis support I recently found the complexity reaches 87!)

  4. I would then normally recommend adding type hints everywhere and validating them using a strict mypy configuration:

    [mypy]
    check_untyped_defs = true
    disallow_untyped_defs = true
    ignore_missing_imports = true
    no_implicit_optional = true
    warn_redundant_casts = true
    warn_return_any = true
    warn_unused_ignores = true
    

    I'm ambivalent about introducing this in code for beginners since it's a complex thing to get right, but in example code I think it could be useful to explain what is happening.

Specific suggestions:

  1. I would argue that globals are more confusing than passing arguments around. After all, keeping that global state in mind as we jump from function to function is one of the main reasons it's unpopular for readability, debuggability and maintainability.
  2. When introducing programming I think it would be helpful to expand every non-obvious abbreviation, such as NUM_COLS. A programmer will recognize that pretty much instantly, but a beginner may be better served with more legibility rather than succinctness. I would also avoid using the indirection of aliases such as tk for tkinter - it may be common to do so among tkinter users, but it's an entirely new concept for students.
  3. On that note, naming is incredibly important for maintainability, because it makes the code easier to read and it especially makes it possible to read each part of the code in isolation and understand what it does, without reference to the rest of it. So I would recommend expanding names like v to value, i to index and el1 to first_element and avoiding vacuous names like temp wherever possible.
  4. You can provide an extra parameter to range to start at 1 instead of 0, simplifying your f-string.
  5. There's some dead code in here (using an IDE like the excellent PyCharm Community Edition will show you most of these and more for free):
    • swap_tile's empty_square.
    • scramble_board's empty_square.
    • FONT and FONT_SIZE.
    • click_callback's x and y parameters.
  6. abs(±0) is zero, so abs(el2[1] - el1[1]) == 0 can be simplified to el2[1] - el1[1] == 0.
  7. is_adjacent does the same calculations twice unless the first conditional is true. Pulling out variables like x_adjacent and y_adjacent has the additional benefit of simplifying the expression to just return x_adjacent or y_adjacent.
  8. find_empty_square_pos should stop as soon as it finds the empty square. You can simply return index_2d(board, candidate).
  9. for i in range(SCRAMBLE_DEPTH): would be more idiomatic as for _ in range(SCRAMBLE_DEPTH): since the index variable is not used, but that may not be worth explaining.
  10. There are a few "magic" values which would be easier to read as constants, like 600 (WINDOW_SIZE? Could this be calculated based on the tile size and margins?), -240 (WINDOW_Y_OFFSET?), 4 (COLUMN_COUNT, aka. NUM_COLS?) and 2 (TILE_PADDING?).
  11. The canonical way to make an executable Python script is to put this ugly thing (and nothing else) at the end of the file:

    if __name__ == "__main__":confirmation that the logic is completely correct
        main()
    

    doing that makes the script contents reusable, because they can be imported by other scripts without actually running the program. This might be too much for beginners, but it's an extremely common pattern so I thought it was worth mentioning.

As for

confirmation that the logic is completely correct

I'm pretty sure you can't get a definite answer from anyone on this site unless there are actually obvious bugs. A comprehensive test suite would help, and is probably the simplest way to achieve some confidence in the workings. Unfortunately this code would be hard to test, because display and logic are tightly coupled.

\$\endgroup\$
  • \$\begingroup\$ Thanks @l0b0. All very useful. I forgot to keep the logic separate from the display in my haste to get the thing working! Next time... \$\endgroup\$ – Robin Jul 22 at 19:57

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.