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

Sudoku challenge driver program

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

Problem

For my planned Sudoku with Handicap challenge over at codegolf.SE, I need a driver program. I've decided to write it in Python, for learning purposes. Since I've got no Python experience, I thought I put it here to see whether it can be improved.

The code is intended to implement the rules as written in the the linked post, except that it tests only one Sudoku (combining the score of several Sudokus is easy to do).

The program gets as first argument the file containing the Sudoku, followed by the command line to start the entry to be scored.

The Sudoku is given in a file containing a 9×9 digit grid, with unknown digits represented by a dot. That grid is followed by an empty line, and a second grid containing the solution. The program checks that the solution indeed fits the original Sudoku (that way I've already spotted a typo on the solution of the first Sudoku in my post), but doesn't currently do any further tests of the validity of the input data.

While validation of the Sudoku file is only nice to have, correct validation of the entries is crucial: It should catch all clients that violate the rules, and even more importantly, it should not declare any valid entry as invalid.

Here's the contents of the file for my first test Sudoku (that's the file I've also done all my testing with):

4.5.7.89.
..2.5.6..
..79..542
..35.6489
...3.8...
6847.91..
238..59..
..6.9.3..
.79.3.2.1

415672893
892453617
367981542
723516489
951348726
684729135
238165974
146297358
579834261


And this is the code:

```
#!/usr/bin/python

from __future__ import print_function

import sys
import itertools
import subprocess
import re
import copy

# -------------------------------------------------------------------
# general functions
# -------------------------------------------------------------------

# Remove leading and trailing whitespace from all lines
def trim_lines(text):
return map(lambda s: s.strip(), text);

# (for debugging purposes only)
# print a list of lists a

Solution

That was quite a lot of code, so I don't think I'll cover all of it, but lets start from the top:

  • Add from __future__ import division – Just to make sure that your integer divisions doesn't produce a float here and there



  • Comments are supposed to be """docstrings""" – You have seemingly good comments, but they should be docstrings on the first line after the function definitions instead of hash-comments in front



-
Use list comprehension instead of map for trim_lines – List comprehension tends to be easier to read and maintain. Read accepted answer on Python List Comprehension vs. Map, and see code suggestion below:

def trim_lines2(texts):
    "Remove leading and trailing whitespace from all lines."

    return [text.strip() for text in texts]


-
_In arrayprint(): Simplify print calls, if possible – Even though only one line, it is really hard to read. Here are two variants, where I think I prefer the last one:

def arrayprint_v3(array, width):
    row_length = len(array[0])
    col_format = '{{:>{}}}'.format(width)
    for row in array:
        print((col_format*row_length).format(*row))

def arrayprint_v3(array, width):
    row_length = len(array[0])

    for row in array:
        print ''.join('{0:>{1}}'.format(col, width) for col in row)


-
Use
with ... for reading files – See http://www.dotnetperls.com/file-python, which explains why it is better to use with ... instead of ordinary try...except when reading files

  • What the duplicate in testall? – I'm not sure I understand what's happening in your testall as you seem to confuscate the original reduce(map(...)) with an extra lambda call?!? Can surely be more readable, and most likely a lot shorter... Shorter version all(map(predicate, list))



  • Try out the divmod() function – Try the following on for size: div, mod = divmod(k-1, 3)



-
Switch to a one-dimensional sudoku array, using index function – You have loads of double loops, and double indexing and inplace calculation to address a given sudoku field. This could be simplified greatly if you switch to a one-dimensional array, and having a function doing transforming from 2D to 1D. The logic is a little bit to confusing, so I might be wrong, but here is some code trying to illustrating simplifications which should be available:

# Replace
i = index / 3
j = index % 3
for k in xrange(0,3):
    for l in xrange(0,3):
        field[3*i+k][3*j+l] = combine(field[3*i+k][3*j+l],
                                      newvals[3*k+l])
# with something like
for i in xrange(9):
    field[index][i] = combine(field[index][i], newvals[i])

# optionally use some variatons over
i, j = divmod(index, 3)

def pos(x, y):
    return 3*x + y


-
You are overly fond of
for i in xrange, followed by list indexing – Your code would look neater if either you used for row in rows, or even for i, row in enumerate(rows). Then you could access row directly, and if you need to cross reference you could use the i in the latter option. Also remember that xrange(0, 9) is the same as xrange(9)

-
... me is exhausted trying to get through the code ...

Final note

Even though you have good variable names and function names, and some good vertical spacing (except not haveing two newlines before a function), your code is hard to read, and difficult to understand.

This in some part due to heavy use of lambdas and maps, combined with the complicated
for` loops. I do believe there is much room for improvement through simplifying list structures, and finding the more pythonic ways of doing things instead of repeating the habits of your older coding language.

Code Snippets

def trim_lines2(texts):
    "Remove leading and trailing whitespace from all lines."

    return [text.strip() for text in texts]
def arrayprint_v3(array, width):
    row_length = len(array[0])
    col_format = '{{:>{}}}'.format(width)
    for row in array:
        print((col_format*row_length).format(*row))

def arrayprint_v3(array, width):
    row_length = len(array[0])

    for row in array:
        print ''.join('{0:>{1}}'.format(col, width) for col in row)
# Replace
i = index / 3
j = index % 3
for k in xrange(0,3):
    for l in xrange(0,3):
        field[3*i+k][3*j+l] = combine(field[3*i+k][3*j+l],
                                      newvals[3*k+l])
# with something like
for i in xrange(9):
    field[index][i] = combine(field[index][i], newvals[i])

# optionally use some variatons over
i, j = divmod(index, 3)

def pos(x, y):
    return 3*x + y

Context

StackExchange Code Review Q#107230, answer score: 7

Revisions (0)

No revisions yet.