Python Sudoku GUI

Question

This code implements a data structure for representing a Sudoku board, a very simple algorithm for setting up the board, and a GUI written in tkinter(requires tkinter and tkinter.tix).

#!/usr/bin/python
# Sudoku board code, and Sudoku board generation code.
# TODO:
# - Sudoku Solver
# - GUI Load/Save game (DONE)
# - GUI Board Drawing (DONE)
# - GUI Board Sync (DONE)
# - GUI Board Interaction (DONE)
# - GUI End Game Mode
# - Reimplment SudokuBoard more efficently and with less complexity
import random
import time
import os
import tkinter.tix
import pickle
from tkinter import *
from tkinter.constants import *
from tkinter.tix import FileSelectBox, Tk
random.seed(time.time())
# There are probably a few bugs in this class, and it could be implemented 
# better I think.
class SudokuBoard:
 """
 Data structure representing the board of a Sudoku game.
 """
 def __init__(self):
 self.clear()
 def clear(self):
 """
 Empty the board.
 """
 self.grid = [[0 for x in range(9)] for y in range(9)]
 self.locked = []
 def get_row(self, row):
 return self.grid[row]
 def get_cols(self, col):
 return [y[col] for y in self.grid]
 def get_nearest_region(self, col, row):
 """
 Regions are 3x3 sections of the grid.
 """
 def make_index(v):
 if v <= 2:
 return 0
 elif v <= 5:
 return 3
 else:
 return 6
 return [y[make_index(col):make_index(col)+3] for y in 
 self.grid[make_index(row):make_index(row)+3]]
 def set(self, col, row, v, lock=False):
 if v == self.grid[row][col] or (col, row) in self.locked:
 return
 for v2 in self.get_row(row):
 if v == v2:
 raise ValueError()
 for v2 in self.get_cols(col):
 if v == v2:
 raise ValueError()
 for y in self.get_nearest_region(col, row):
 for x in y:
 if v == x:
 raise ValueError()
 self.grid[row][col] = v
 if lock:
 self.locked.append((col, row))
 def get(self, col, row):
 return self.grid[row][col]
 def __str__(self):
 strings = []
 newline_counter = 0
 for y in self.grid:
 strings.append("%d%d%d %d%d%d %d%d%d" % tuple(y))
 newline_counter += 1
 if newline_counter == 3:
 strings.append('')
 newline_counter = 0
 return '\n'.join(strings)
def sudogen_1(board):
 """
 Algorithm:
 Add a random number between 1-9 to each subgrid in the 
 board, do not add duplicate random numbers.
 """
 board.clear()
 added = [0]
 for y in range(0, 9, 3):
 for x in range(0, 9, 3):
 if len(added) == 10:
 return
 i = 0
 while i in added:
 i = random.randint(1, 9)
 try:
 board.set(random.randint(x, x+2), random.randint(y, y+2), i, lock=True)
 except ValueError:
 print("Board rule violation, this shouldn't happen!")
 added.append(i)
def rgb(red, green, blue):
 """
 Make a tkinter compatible RGB color.
 """
 return "#%02x%02x%02x" % (red, green, blue)
class SudokuGUI(Frame):
 board_generators = {"SudoGen v1 (Very Easy)":sudogen_1}
 board_generator = staticmethod(sudogen_1)
 def new_game(self):
 self.board.clear()
 self.board_generator(self.board)
 self.sync_board_and_canvas()
 def make_modal_window(self, title):
 window = Toplevel()
 window.title(title)
 window.attributes('-topmost', True)
 window.grab_set()
 window.focus_force()
 return window
 def load_game(self):
 def _load_game(filename):
 with open(filename, 'rb') as f:
 board = pickle.load(f)
 if not isinstance(board, SudokuBoard):
 # TODO: Report bad file
 return
 self.board = board
 self.sync_board_and_canvas()
 window.destroy()
 window = self.make_modal_window("Load Game")
 fbox = FileSelectBox(window, command=_load_game)
 fbox.pack()
 window.mainloop()
 def save_game(self):
 def _save_game(filename):
 with open(filename, 'wb') as f:
 pickle.dump(self.board, f, protocol=2)
 window.destroy()
 window = self.make_modal_window("Save Game")
 fbox = FileSelectBox(window, command=_save_game)
 fbox.pack()
 window.mainloop()
 def query_board(self):
 window = self.make_modal_window("Set Board Algorithm")
 scroll = Scrollbar(window)
 scroll.pack(side='right', fill='y')
 listbox = Listbox(window, yscrollcommand=scroll.set) 
 scroll.config(command=listbox.yview)
 bframe = Frame(window)
 for s in self.board_generators.keys():
 listbox.insert(-1, s)
 def do_ok():
 self.board_generator = self.board_generators[listbox.get(ACTIVE)]
 window.destroy()
 def do_cancel():
 window.destroy()
 cancel = Button(bframe, command=do_cancel, text="Cancel")
 cancel.pack(side='right', fill='x')
 ok = Button(bframe, command=do_ok, text="Ok")
 ok.pack(side='right', fill='x')
 listbox.pack(side='top', fill='both', expand='1')
 bframe.pack(side='top', fill='x', expand='1')
 window.mainloop()
 def make_grid(self):
 c = Canvas(self, bg=rgb(128,128,128), width='512', height='512')
 c.pack(side='top', fill='both', expand='1')
 self.rects = [[None for x in range(9)] for y in range(9)]
 self.handles = [[None for x in range(9)] for y in range(9)]
 rsize = 512/9
 guidesize = 512/3
 for y in range(9):
 for x in range(9):
 (xr, yr) = (x*guidesize, y*guidesize)
 self.rects[y][x] = c.create_rectangle(xr, yr, xr+guidesize, 
 yr+guidesize, width=3)
 (xr, yr) = (x*rsize, y*rsize)
 r = c.create_rectangle(xr, yr, xr+rsize, yr+rsize)
 t = c.create_text(xr+rsize/2, yr+rsize/2, text="SUDO",
 font="System 15 bold")
 self.handles[y][x] = (r, t)
 self.canvas = c
 self.sync_board_and_canvas()
 def sync_board_and_canvas(self):
 g = self.board.grid
 for y in range(9):
 for x in range(9):
 if g[y][x] != 0:
 self.canvas.itemconfig(self.handles[y][x][1], 
 text=str(g[y][x]))
 else:
 self.canvas.itemconfig(self.handles[y][x][1], 
 text='')
 def canvas_click(self, event):
 print("Click! (%d,%d)" % (event.x, event.y))
 self.canvas.focus_set()
 rsize = 512/9
 (x,y) = (0, 0)
 if event.x > rsize:
 x = int(event.x/rsize)
 if event.y > rsize:
 y = int(event.y/rsize)
 print(x,y)
 if self.current:
 (tx, ty) = self.current
 #self.canvas.itemconfig(self.handles[ty][tx][0], fill=rgb(128,128,128))
 self.current = (x,y)
 # BUG: Changing the color of the background of a tile erases parts of
 # the thick gridlines
 #self.canvas.itemconfig(self.handles[y][x][0], fill=rgb(255,255,255))
 def canvas_key(self, event):
 print("Clack! (%s)" % (event.char))
 if event.char.isdigit() and int(event.char) > 0 and self.current:
 (x,y) = self.current
 #self.canvas.itemconfig(self.handles[y][x][0], fill=rgb(128,128,128))
 try:
 self.board.set(x, y, int(event.char))
 self.sync_board_and_canvas()
 except ValueError:
 # TODO: I'd rather set the erroneous value anyway and simply
 # not consider it valid, and perhaps set the text color
 # to red.
 pass
 def __init__(self, master, board):
 Frame.__init__(self, master)
 if master:
 master.title("SudokuGUI")
 self.board = board
 self.board_generator(board)
 bframe = Frame(self)
 self.ng = Button(bframe, command=self.new_game, text="New Game")
 self.ng.pack(side='left', fill='x', expand='1')
 self.sg = Button(bframe, command=self.save_game, text="Save Game")
 self.sg.pack(side='left', fill='x', expand='1')
 self.lg = Button(bframe, command=self.load_game, text="Load Game")
 self.lg.pack(side='left', fill='x', expand='1')
 self.query = Button(bframe, command=self.query_board, text="Set Board Algorithm")
 self.query.pack(side='left', fill='x', expand='1')
 bframe.pack(side='bottom', fill='x', expand='1')
 self.make_grid()
 self.canvas.bind("<Button-1>", self.canvas_click)
 self.canvas.bind("<Key>", self.canvas_key)
 self.current = None
 self.pack()
if __name__ == '__main__':
 board = SudokuBoard()
 tk = Tk()
 gui = SudokuGUI(tk, board)
 gui.mainloop()

Answer 1

In general I think the code is clear, self-explanatory and well implemented. For some points that could use improvement:

• SudokuBoard.get_nearest_region

  • make_index is unnecessary, use v // 3 * 3 instead (in case you don't know, // means "integer division" in Python); (Edit: as @NolenRoyalty pointed out, keeping it is better for code clarity, though it can be simplified using the expression above)

  • Since this function is only used in set, you might as well make its results more convenient for it (by flattening the result):

    def make_index(v):
     """Index of the closest row/column (to the top/left)"""
     return v // 3 * 3
    return [c for y in self.grid[make_index(row):make_index(row)+3] for c in y[make_index(col):make_index(col)+3]]
    

    This way the calling code will be much simpler:

    if v in self.get_row(row) + self.get_cols(col) + self.get_nearest_region(col,row):
     raise ValueError()
    

    Note also that I replaced the 3 for loops for a single if, using list concat to get all the invalid values (maybe with some repetition) and in to check if your value is in that list. Set union would work too: set(list1) | set(list2) | set(list3) (I'm referring to the builtin type set, not your SudokuBoard.set function).

• In SudokuBoard.__str__, you could use enumerate to get the index of each element together with the element, while iterating, and using remainder (modulus) to detect third rows:

  strings = []
  for index,y in enumerate(self.grid):
   strings.append("%d%d%d %d%d%d %d%d%d" % tuple(y))
   if index % 3 == 2:
   strings.append('')
  return '\n'.join(strings)
  

• In sudogen_1, instead of repeateadly generating random numbers, you could just shuffle the interval and pick from it (since you don't want repeats):

  board.clear()
  r_vals = range(1,9) # Generate all values from 1 to 9
  random.shuffle(r_vals) # Shuffle those values, to they will appear in random order
  for y in range(0, 9, 3):
   for x in range(0, 9, 3):
   if not r_vals:
   return
   i = r_vals.pop() # Gets (and removes) one value from list
   try:
   board.set(random.randint(x, x+2), random.randint(y, y+2), i, lock=True)
   except ValueError:
   print("Board rule violation, this shouldn't happen!")
  

• In sync_board_and_canvas I'd personally not use that if-else, but move it to the text parameter only (that's a bit subjective though):

  self.canvas.itemconfig(self.handles[y][x][1], 
   text=str(g[y][x]) if g[y][x] else '')
  

That's it. I have no experience with tkinter, so I limited my feedback to general aspects of your code. Maybe someone with better knowledge of it can contribute with that other part.

• Zachary Richey: Thanks. I didn't know about python's integer division operator, and it never occurred to me that it is much more efficient to shuffle a range of values once rather than generate and --possibly discard-- numbers over and over. I did actually decide to rewrite the board code a few hours ago, which can be seen here: https://github.com/Mach1723/sudoku/blob/board_rewrite/sudoku.py. So that particular bit of code is much better now I think, but once again thanks for reviewing!
• Nolen Royalty: I agree that make_index is unnecessary but I think having a function like def make_index(x): return x//3 would actually add to clarity, I know I used something similar when I wrote a sudoku solver. This is a great, comprehensive post though. +1
• mgibsonbr: @NolenRoyalty I agree, substituting this "magic expression" for a meaningul function would indeed add to clarity. Answer updated
• cmh: @mgibsonbr It also allows easier abtraction when you want to change the size of the board to some size other than 3x3.