Python Tkinter Game - Treasure Hunt

Question

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")
        self.island.grid(row=0,column=1, stick=N+E+S+W, rowspan=3)
        # restart button
        self.restart_b = Button(app, text="Restart", bg="red", command=self.begin)  
        self.restart_b.grid(row=2, column=0, pady=20)
        # score labels and fields
        self.score_lbl = Label(app, text="Guesses: 0", bg="yellow")
        self.score_lbl.grid(row=1, column=0)

        # set keydown handler
        root.bind("<Key>", self.key_pressed)
        # best score variable
        self.best_score = 0
        # begin game
        self.begin()
        #print self.treasure_pos

    def begin(self):
        # game state variables
        root.after_cancel(self.tick)
        self.matrix = [["#" for col in range(8)] for row in range(8)]
        self.current_pos = [0,0]
        self.treasure_pos = [random.randrange(8), random.randrange(8)]
        #self.treasure_pos = [0,0]
        #print self.treasure_pos
        self.blink = False
        self.guesses = 0
        self.end_tick = False
        self.tick()

    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)

    def process_guess(self):
        self.guesses += 1
        self.score_lbl.config(text="Guesses: " + str(self.guesses))
        if not (self.current_pos[0] == self.treasure_pos[0] and self.current_pos[1] == self.treasure_pos[1]):
            #print "NOT HERE"
            dist = int(round(math.sqrt((self.current_pos[0] - self.treasure_pos[0]) ** 2 + (self.current_pos[1] - self.treasure_pos[1]) ** 2)))
            self.matrix[self.current_pos[0]][self.current_pos[1]] = str(dist)
            self.display_grid()
        else:
            self.end_tick = True

    def finish(self):
        self.matrix[self.treasure_pos[0]][self.treasure_pos[1]] = "$"
        self.display_grid()
        self.island.insert(END, "Gold!")

    def tick(self):
        '''timer for blinking cursor'''
        if self.blink == False:
            self.matrix[self.current_pos[0]][self.current_pos[1]] = "#"
        elif self.blink == True:
            self.matrix[self.current_pos[0]][self.current_pos[1]] = " "
        self.blink = not self.blink
        self.display_grid()
        if not self.end_tick:
            root.after(200, self.tick)
        else:
            self.finish()

    def key_pressed(self, event):
        if event.keysym == "Right" and self.current_pos[1] < 7:
            self.current_pos[1] += 1
        elif event.keysym == "Left" and self.current_pos[1] > 0:
            self.current_pos[1] -= 1
        elif event.keysym == "Up" and self.current_pos[0] > 0:
            self.current_pos[0] -= 1
        elif event.keysym == "Down" and self.current_pos[0] < 7:
            self.current_pos[0] += 1
        elif event.keysym == "Return":
            self.process_guess()
        self.display_grid()
        self.matrix = [["#" for col in range(8)] for row in range(8)] # is here the best place for this?


root = Tk()
game = TreasureHunt(root)
root.mainloop()

Answer 1

import tkFont, random, math

Hey, at least you aren't using wildcard imports (from <module> 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[...] < 7: You should be using constants instead of magic numbers. It would be self.current_pos[0] < NUM_ROWS - 1 or self.current_pos[1] < NUM_COLS - 1

self.matrix = ...

I would put that in its own method. You have used that same code in another place. Duplicate code -> new function.