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

Deepcopy usage in a Sudoku solver

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
solverusagesudokudeepcopy

Problem

In an effort to teach myself some python and programming in general I made a Sudoku Solver (as many others have done before me from what I can tell). The one thing that bugs me a little is my use of deepcopy which I think I can avoid by implementing __eq__ in my classes.

My questions:

  • Is this understanable?



  • How Pythonic is this?



  • Are there any glaring bugs / convention mistakes / missuses of structures?



  • How can I introduce unit testing?



Full code (400 lines) can be found here.

```
class SudokuCell:
"""Most Basic Sudoku Cell
-Contains 9 Possible values, initialized at true
-Can return the possible cases
"""
def __init__(self):
self.values = {1: True,2:True,3:True,4:True,5:True,6:True,7:True,8:True,9:True}

def check_cell(self):
"""return all values still possible"""
out = []
for k,d in self.values.items():
if d==True:
out.append(k)
return out

def set_cell(self, val):
"""set all values to False except val"""
for k,d in self.values.items():
if k != val:
self.values[k] = False
self.values[val]=True

def reset_cell(self):
self.values = {1: True,2:True,3:True,4:True,5:True,6:True,7:True,8:True,9:True}

class SudokuGrid:
def __init__(self):
self.Cells = []
for x in range(0,81):
Cell = SudokuCell()
self.Cells.append(Cell)
self.groups = self.__define_groups__()

def __eq__(self, other):
return self.Cells == other.Cells

def print_Grid(self):
"""print Sudoku Grid"""
values=self.__str_fill__()
print '-------------------'
for x in range (0,3):
print '|'+values[x9] +' '+ values[x9+1] +' ' + values [x9+2] + '|'+values[x9+3] +' '+ values[x9+4] +' ' + values [x9+5] + '|'+values[x9+6] +' '+ values[x9+7] +' ' + values [x*9+8] + '|'
print '---------------'
for x in range

Solution

First : the comments about aesthetic.

You can use List/set/dict comprehension more than you do :

self.values = {1: True,2:True,3:True,4:True,5:True,6:True,7:True,8:True,9:True}


becomes :

self.values = {n:True for n in range(10)}


or

self.values = dict.fromkeys(range(10), True)


out = []
    for k,d in self.values.items():
        if d==True:
            out.append(k)


becomes :

out = [k for k,d in self.values.items() if d]


From PEP 8 :


Don't compare boolean values to True or False using ==.

print '|'+values[x*9] +' '+ values[x*9+1] +' ' + values [x*9+2] + '|'+values[x*9+3] +' '+ values[x*9+4] +' ' + values [x*9+5] + '|'+values[x*9+6] +' '+ values[x*9+7] +' ' + values [x*9+8] + '|'


becomes :

print '|'+ ' '.join(values[x*9+i] for i in range(9)) + '|'


From PEP 8 :


For example, do not rely on CPython's efficient implementation of
in-place string concatenation for statements in the form a += b or a =
a + b. This optimization is fragile even in CPython (it only works for
some types) and isn't present at all in implementations that don't use
refcounting. In performance sensitive parts of the library, the
''.join() form should be used instead. This will ensure that
concatenation occurs in linear time across various implementations.

for x in self.Cells:
        if len(x.check_cell())!=1:
            return False
    return True


becomes :

return all(len(x.check_cell()) == 1 for x in self.Cells)


groups.append([x*9,x*9+1,x*9+2,x*9+3,x*9+4,x*9+5,x*9+6,x*9+7,x*9+8])


becomes :

groups.append([x*9+i for i in range(9)])


.

groups.append([x,x+9,x+18,x+27,x+36,x+45,x+54,x+63,x+72])


becomes :

groups.append([x+9*i for i in range(9)])


.

groups.append([x*3,x*3+1,x*3+2,x*3+9,x*3+10,x*3+11,x*3+18,x*3+19,x*3+20])


becomes :

groups.append([x*3+i*9+j for i in range(3) for j in range(3)])


.

out =[]
    i=0
    for x in self.Cells:
        out.append('*')
        if len(x.check_cell())==1:
            out[i]=str(x.check_cell()[0])
        i+=1


becomes first using enumerate :

out =[]
    for i,x in enumerate(self.Cells):
        out.append('*')
        if len(x.check_cell())==1:
            out[i]=str(x.check_cell()[0])


then, with ternary operator

out =[]
    for x in self.Cells:
        out.append(str(x.check_cell()[0]) if len(x.check_cell())==1 else '*')


finally with list comprehension :

out =[str(x.check_cell()[0]) if len(x.check_cell())==1 else '*' for x in self.Cells]


----------

Underscore is a conventional variable name for a value that we don't use (more info here).

for x in range(81):
        Cell = SudokuCell()
        self.Cells.append(Cell)


becomes :

for _ in range(81):
        Cell = SudokuCell()
        self.Cells.append(Cell)


----------

Now, more relevant comments :

Data structure : at the moment, you are representing cells with dictionnary mapping values to whether they are possible or not. A dictionnary is likely to be a bit overkill here : you might as well use :

  • a simple list to perform exactly the same mapping (reindexing your 1 to 9 range to a 0 to 8 range would be a good option).



  • a set to convey the same information : a value is possible if and only if it is in the set.



Whenever you are using this kind of logic, you should try to keep your structure as small as possible for performance reasons but also because it will make things harder to get wrong. In your case, there probably no need to store the result of __define_groups__() in each instance of the class because the value returned is always the same, you could compute this one and for all and use it for all instances.

Code Snippets

self.values = {1: True,2:True,3:True,4:True,5:True,6:True,7:True,8:True,9:True}
self.values = {n:True for n in range(10)}
self.values = dict.fromkeys(range(10), True)
out = []
    for k,d in self.values.items():
        if d==True:
            out.append(k)
out = [k for k,d in self.values.items() if d]

Context

StackExchange Code Review Q#45817, answer score: 15

Revisions (0)

No revisions yet.