Python Tkinter Game - Treasure Hunt

I used to write a bit of BASIC years ago, and recently found an old book called "Fun Mathematics on your Microcomputer" with some mathematical games in. I'm learning Python and have tried to implement one of the games, called "Treasure Hunt," which I played as a kid on a ZX81(!).

This is the first project for me where I've used either Tkinter or Classes, so it's been a bit of a learning curve.

I feel I've done the best I can on my own, and would appreciate any feedback on my attempt, in terms of the structure, logic or anything else that either I've done well or could be improved.

Two things that I know need fixing are the "timer" behaviour after pressing "reset," and also the persistence of the distance values after pressing Return.

I must say, that BASIC seems like a walk in the park compared to all this event-driven, object-oriented post 1985 coding business!

```
from Tkinter import *
import tkFont, random, math

class TreasureHunt:

def __init__(self, master):
app = Frame(master, bg="yellow")
app.grid_rowconfigure(0, weight=1)
app.grid_columnconfigure(1, weight=1)
app.grid_columnconfigure(0, weight=1)
app.pack(fill="both", expand=True)
# instructions and fonts
self.mono_font = tkFont.Font(family="Courier",size=24,weight="bold")
self.instructions = "Find the hidden treasure!\n\nUse the arrow keys to select where to look, then press Enter to check. \
There is a 50/50 chance you will be told the distance from the treasure. Keep hunting until you find it. Good luck!"
# create instructions widget
self.info = Text(app, wrap=WORD, padx=10, pady=10,width=15,bd=0, height=19, bg="yellow")
self.info.insert(1.0,self.instructions)
self.info.grid(row=0,column=0,sticky=N+E+S+W)
# create island widget
self.island = Text(app, bg="cyan", padx=40, pady=40, font=self.mono_font,width=15, height=9, wrap=NONE,bd=0)
self.island.insert(1.0, "ready")

import tkFont, random, math

Hey, at least you aren't using wildcard imports (from import *), but PEP 8, the Python style guide, has this to say about that line:


Imports should usually be on separate lines, e.g.:

Yes: import os
     import sys

No:  import sys, os

It's okay to say this though:

from subprocess import Popen, PIPE

I would recommend that you read that whole PEP.

self.matrix = [["#" for col in range(8)] for row in range(8)]

Incredible! I passed about 30 lines without comment. This is mostly good. You used a list comprehension instead of list multiplication. That's excellent. Just a small tweak: you aren't using col or row, so you might as well use _ as the variable name. It makes it more obvious that the variables aren't being used. I could even see someone disagreeing with me on that, though, because it does make it more obvious why you chose range(8). Meh, do what you want, but now you know. Speaking of how you chose those numbers, they are what are called magic numbers. You should define a constant at the beginning of the file that defines how many rows and how many columns there are in the grid. You would also use that number in th definition of self.treasure_pos.

def display_grid(self):
    '''Displays current visual game state'''
    self.island.delete(1.0, END) 
    m_str = ""
    for row in range(len(self.matrix)):
        m_str += (" ".join(self.matrix[row]) + "\n")
    self.island.insert(1.0, m_str)

I would move the deleting line until just before the insertion. That makes it less likely that the user will notice the text disappear and re-appear. Even better, insert each line:

def display_grid(self):
    self.island.delete(1.0, END)
    for row in self.matrix:
        self.island.insert(END, " ".join(row) + "\n")

I also changed what your loop was iterating through. You can iterate through any sequence, not just a range().

if not (self.current_pos[0] == self.treasure_pos[0] and self.current_pos[1] == self.treasure_pos[1]):
    ...
else:
    ...

Why have a not before everything if you have an else:? Why not remove the not and switch the blocks? Just one more thing, though. self.current_pos and self.treasure_pos are both two-item lists. You can simply check their equality instead of checking the equality of each of their items:

if self.current_pos == self.treasure_pos:
    self.end_tick = True
else:
    ...

dist = int(round(math.sqrt((self.current_pos[0] - self.treasure_pos[0]) ** 2 + (self.current_pos[1] - self.treasure_pos[1]) ** 2)))

That's 131 characters, not including indentation. PEP 8 says:


Limit all lines to a maximum of 79 characters.

It's a little hard to tell what is going on with a line that long. Maybe this:

# Pythagorean theorem
row_dist = (self.current_pos[0] - self.treasure_pos[0])
col_dist = (self.current_pos[1] - self.treasure_pos[1])
dist = int(round(math.sqrt(row_dist ** 2 + col_dist ** 2)))
...

if self.blink == False:
...
elif self.blink == True:

if ... is seeing if ... is True. Using if ... == True: is redundant. Using if ... == False: could be simplified to if not ...:. Yet again we see the pattern self.matrix[...[0]][...[1]]. You might consider creating a method that will get or set the cell at given coordinates in the matrix.

if event.keysym == "..." and self.current_pos[...] < ...:

What if you said "right" instead of "Right"? It might be difficult to find out why your program wasn't working correctly. There would be no error; that code just wouldn't execute. You should use the built-in Tkinter symbols:

if event.lower() == RIGHT.lower() and ...

My second problem is the use of `self.current_pos[...] new function.

StackExchange Code Review Q#124609, answer score: 2