Python Turtle implementation of the 15 Puzzle

Question

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()

Answer 1

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.

• Robin Andrews: 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...