HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Sudoku solver with Tkinter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewobject-orientedtkinterstackoverflowpythonpython-2.xsudoku
withsolversudokutkinter

Problem

The code below is a Sudoku solver using backtracking. There is a fast mode which doesn't display the permutations in the Tk widget in real time; only the solved grid.

This is the second version of a sudoku solver using class inheritance and Tkinter. The first version was a one class version which was much faster (15 times). Can the code be improved to speed things up and match the speed of the previous version?

Is it legible and 'pythonic' enough? Is this the best way to structure the data and class inheritance? does instantiating always mean slower script sacrificing performance for clarity?

```
#!/usr/bin/python
#Xavier B. 2017

import time as tm
import Tkinter as tk
import cProfile

GRIDA = [
3, 0, 8, 0, 1, 4, 0, 0, 9,
0, 0, 2, 0, 6, 0, 1, 7, 4,
7, 1, 0, 5, 9, 0, 8, 0, 0,
0, 0, 0, 9, 0, 3, 4, 1, 7,
5, 9, 0, 2, 4, 0, 3, 0, 0,
4, 3, 7, 0, 0, 6, 0, 5, 0,
1, 0, 5, 4, 0, 0, 0, 3, 8,
0, 2, 0, 0, 3, 5, 7, 0, 1,
0, 4, 3, 6, 0, 1, 0, 9, 0
]
GRIDNIL = [
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0
]
ULTIGRID = [
0, 0, 0, 0, 0, 4, 2, 0, 0,
2, 0, 0, 5, 1, 0, 0, 0, 0,
7, 8, 0, 0, 0, 6, 4, 0, 0,
5, 9, 0, 0, 0, 7, 0, 0, 0,
0, 4, 0, 0, 0, 0, 0, 8, 0,
0, 0, 0, 2, 0, 0, 0, 9, 5,
0, 0, 7, 4, 0, 0, 0, 3, 2,
0, 0, 0, 0, 3, 9, 0, 0, 1,
0, 0, 3, 1, 0, 0, 0, 0, 0
]
GRIDB = [
0, 0, 5, 8, 0, 0, 0, 0, 2,
8, 0, 0, 0, 0, 0, 4, 0, 0,
0, 0, 9, 5, 0, 0, 0, 7, 8,
7, 0, 0, 3, 0, 0, 1, 0, 0,
0, 4, 0, 0, 0, 0, 0, 8, 0,
0, 0, 6, 0, 0, 8, 0, 0, 3,
6, 9, 0, 0, 0, 3, 7, 0, 0,

Solution

I will be commenting on the first version, since it is the one using Tkinter as per the question title.

A few things on the GUI part for starter:

  • Since you only call Grid.draw_grid() once, at the beginning, your cells can be in only two states: empty or solved. This means you only need to define 2 background colors since they will never be updated.



  • You try hard to draw extra bold lines in Grid.draw_grid() only to override them with each number's rectangle. You should instead do the opposite: draw the rectangles and then draw the lines. To simplify a bit, I’d draw a big rectangle a bit larger than the area for the border and the inner lines and then draw the 4 missing bolder lines.



  • You can get rid of the time module as time.sleep(2) followed by Solver.root.after(0, self.quit_solve) can be replaced by Solver.root.after(2000, self.quit_solve): the waiting will be done in Tkinter. But I don't really understand the need to auto-close the window.



  • You don't need a lambda to pass parameters to functions in an after call: everything you put after the function will be passed as parameter. So you can turn Solver.root.after(0, lambda: self.solve(index)) into Solver.root.after(0, self.solve, index).



Now that being said, you don't use classes nor inheritance the proper way. Inheritance, as in class Cell(Grid), is meant to describe an "is a" relationship, where Cell can do whatever Grid is up to plus a bit more. In this case, a cell is not a grid, there is absolutely no need to use inheritance here. However, cells belong to a grid, and that's what you represent in your Grid.populate_cells() method.

Comming from the same kind of misconception, you use class attributes the wrong way (like Grid._cells). What if I want to solve two sudoku in the same program? When instanciating the second one, Grid._cells that contained data for the first one would be overriden. This is wrong.

Cells should know nothing about other cells, only the grid should. Same: the grid should know nothing about the rendering engine (Solver in this case, which is a pretty bad name for a graphical component IMHO), only about its cells and how to solve the grid.

Basically, a cell should only hold information and have next to no action available. I'd just add a method to get the next possible value and a method to reset the cell to an empty state. Something along the lines of:

class Cell(object):
    """81 cells in a 9x9 sudoku grid."""
    def __init__(self, value):
        self.value = value
        self.solvable = value is None
        if value is None:
            self._possibilities = iter(range(1, 10))
        else:
            self._possibilities = iter([])

    @property
    def empty(self):
        return self.value is None

    @property
    def text(self):
        return ' ' if self.value is None else str(self.value)

    def __str__(self):
        return '.' if self.value is None else str(self.value)

    def reset(self):
        if self.solvable:
            self.value = None
            self._possibilities = iter(range(1, 10))

    def next(self):
        return next(self._possibilities)


@property hide a method behind an attribute. So you can do if cell.empty: which is nicer in my opinion. Here the cell hold 4 "attributes": value, solvable, empty, and text. And have 2 possible actions: getting the next value to try for this cell, and resetting the cell to an empty one (only if solvable: i.e. if it didn't have a value in the first place).

Now it's up to the Grid, which will be aware of rows and columns, to implement logic related to getting the right cell and trying the various values in order to solve the grid.

Only after that would the Tkinter part come into play.

My take on that would look like:

```
#!/usr/bin/python

import Tkinter as tk
import itertools

GRIDA = [
3, 0, 8, 0, 1, 4, 0, 0, 9,
0, 0, 2, 0, 6, 0, 1, 7, 4,
7, 1, 0, 5, 9, 0, 8, 0, 0,
0, 0, 0, 9, 0, 3, 4, 1, 7,
5, 9, 0, 2, 4, 0, 3, 0, 0,
4, 3, 7, 0, 0, 6, 0, 5, 0,
1, 0, 5, 4, 0, 0, 0, 3, 8,
0, 2, 0, 0, 3, 5, 7, 0, 1,
0, 4, 3, 6, 0, 1, 0, 9, 0,
]
GRIDNIL = [
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0,
]
ULTIGRID = [
0, 0, 0, 0, 0, 4, 2, 0, 0,
2, 0, 0, 5, 1, 0, 0, 0, 0,
7, 8, 0, 0, 0, 6, 4, 0, 0,
5, 9, 0, 0, 0, 7, 0, 0, 0,
0, 4, 0, 0, 0, 0, 0, 8, 0,
0, 0, 0, 2, 0, 0, 0, 9, 5,
0, 0, 7, 4, 0, 0, 0, 3, 2,
0, 0, 0, 0, 3, 9, 0, 0, 1,
0, 0, 3, 1, 0, 0, 0, 0, 0,
]
GRIDB = [
0, 0, 5, 8, 0, 0, 0, 0, 2,
8, 0, 0, 0, 0, 0, 4, 0, 0,
0, 0, 9, 5, 0, 0, 0, 7, 8,
7, 0, 0, 3, 0, 0, 1, 0, 0,
0, 4, 0, 0, 0, 0, 0, 8, 0,
0, 0, 6, 0, 0, 8, 0, 0, 3,
6, 9, 0, 0, 0, 3, 7, 0, 0,
0, 0, 2, 0, 0,

Code Snippets

class Cell(object):
    """81 cells in a 9x9 sudoku grid."""
    def __init__(self, value):
        self.value = value
        self.solvable = value is None
        if value is None:
            self._possibilities = iter(range(1, 10))
        else:
            self._possibilities = iter([])

    @property
    def empty(self):
        return self.value is None

    @property
    def text(self):
        return ' ' if self.value is None else str(self.value)

    def __str__(self):
        return '.' if self.value is None else str(self.value)

    def reset(self):
        if self.solvable:
            self.value = None
            self._possibilities = iter(range(1, 10))

    def next(self):
        return next(self._possibilities)
#!/usr/bin/python

import Tkinter as tk
import itertools


GRIDA = [
    3, 0, 8, 0, 1, 4, 0, 0, 9,
    0, 0, 2, 0, 6, 0, 1, 7, 4,
    7, 1, 0, 5, 9, 0, 8, 0, 0,
    0, 0, 0, 9, 0, 3, 4, 1, 7,
    5, 9, 0, 2, 4, 0, 3, 0, 0,
    4, 3, 7, 0, 0, 6, 0, 5, 0,
    1, 0, 5, 4, 0, 0, 0, 3, 8,
    0, 2, 0, 0, 3, 5, 7, 0, 1,
    0, 4, 3, 6, 0, 1, 0, 9, 0,
]
GRIDNIL = [
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0,
]
ULTIGRID = [
    0, 0, 0, 0, 0, 4, 2, 0, 0,
    2, 0, 0, 5, 1, 0, 0, 0, 0,
    7, 8, 0, 0, 0, 6, 4, 0, 0,
    5, 9, 0, 0, 0, 7, 0, 0, 0,
    0, 4, 0, 0, 0, 0, 0, 8, 0,
    0, 0, 0, 2, 0, 0, 0, 9, 5,
    0, 0, 7, 4, 0, 0, 0, 3, 2,
    0, 0, 0, 0, 3, 9, 0, 0, 1,
    0, 0, 3, 1, 0, 0, 0, 0, 0,
]
GRIDB = [
    0, 0, 5, 8, 0, 0, 0, 0, 2,
    8, 0, 0, 0, 0, 0, 4, 0, 0,
    0, 0, 9, 5, 0, 0, 0, 7, 8,
    7, 0, 0, 3, 0, 0, 1, 0, 0,
    0, 4, 0, 0, 0, 0, 0, 8, 0,
    0, 0, 6, 0, 0, 8, 0, 0, 3,
    6, 9, 0, 0, 0, 3, 7, 0, 0,
    0, 0, 2, 0, 0, 0, 0, 0, 9,
    1, 0, 0, 0, 0, 7, 2, 0, 0,
]


class Window(object):
    def __init__(self, solver):
        self.solver = solver
        self._root = root = tk.Tk()
        self._canvas = tk.Canvas(root, height=750, width=750, bg='white')
        self._labels = {}

        # Draw background, a bit larger
        self._canvas.create_rectangle(
                48, 48, 682, 682,
                fill='black', outline='black')

        # Draw cells
        for i, row in enumerate(solver.rows):
            for j, cell in enumerate(row):
                x = i * 70
                y = j * 70
                background_color = '#ffffff' if cell.solvable else '#f7d52e'
                self.rectangle = self._canvas.create_rectangle(
                        51 + y, 51 + x, 119 + y, 119 + x,
                        fill=background_color,
                        outline=background_color)
                self._labels[(i, j)] = label = tk.Label(
                        self._canvas, text=cell.text, relief=tk.FLAT,
                        bg=background_color, font=("Courier", 38))
                label.place(x=66 + y, y=55 + x)

        # Add extra bold lines
        def draw_line(x1, y1, x2, y2):
            self._canvas.create_line(
                    [(x1, y1), (x2, y2)], width=3,
                    joinstyle='miter', capstyle='projecting')
        draw_line(260, 50, 260, 680)
        draw_line(50, 260, 680, 260)
        draw_line(470, 50, 470, 680)
        draw_line(50, 470, 680, 470)

        self._canvas.pack(expand=True)

    def run(self, delay=None):
        if delay is None:
            self._root.after(0, self.iterate_fast)
        else:
            self._root.after(0, self.iterate, delay)
        self._root.mainloop()

    def iterate(self, delay):
        self.redraw()
        if not sel

Context

StackExchange Code Review Q#154230, answer score: 5

Revisions (0)

No revisions yet.