current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I have just started to learn programming with Python. I have been going through several online classes for the last month or two. Please bear with me if my questions are noobish.

One of the classes I am taking asked us to solve this problem.

# Define a procedure, check_sudoku,
# that takes as input a square list
# of lists representing an n x n
# sudoku puzzle solution and returns the boolean
# True if the input is a valid
# sudoku square and returns the boolean False
# otherwise.

# A valid sudoku square satisfies these
# two properties:

#   1. Each column of the square contains
#       each of the whole numbers from 1 to n exactly once.


#   2. Each row of the square contains each
#       of the whole numbers from 1 to n exactly once.

# You may assume the the input is square and contains at
# least one row and column.

After a while of trying to come up with the best code I can, this is what I ended up with.

def check_sudoku(sudlist):
    x = range(1, len(sudlist)+1) # makes a list of each number to be found
    rows = [[row[i] for row in sudlist] for i in range(len(sudlist))] # assigns all the rows to a flat list
    z = range(len(sudlist))
    for num in x:
        for pos in z:
            if num not in sudlist[pos] or num not in rows[pos]:
                return False
    return True

This code does work and passed all checks the class asked for. I just wonder what problems could the code have or how would you improve it?

I am just trying to improve by getting input from others. I appreciate any suggestions.

share|improve this question
    
The problem seems poorly stated. A Sudoku grid has three types of constraint: rows, columns, and blocks. A grid that only has row and column constraints is known as a Latin square. –  Gareth Rees Feb 17 '13 at 22:58

2 Answers 2

up vote 7 down vote

I would use set to do the checks, and you can build the columns using some zip magic:

def check_sudoku(sudlist) :
    numbers = set(range(1, len(sudlist) + 1))
    if (any(set(row) != numbers for row in sudlist) or
        any(set(col) != numbers for col in zip(*sudlist))) :
        return False
    return True

I have tested it with the following samples:

a = [[1, 2, 3, 4, 5],
     [2, 3, 5, 1, 4],
     [3, 5, 4, 2, 1],
     [4, 1, 2, 5, 3],
     [5, 4, 1, 3, 2]] # latin square

b = [[1, 2, 3, 4, 5],
     [2, 3, 5, 1, 4],
     [3, 5, 4, 2, 1],
     [4, 1, 2, 5, 3],
     [3, 4, 1, 5, 2]] # 1st and 4th elements of last row interchanged

c = [[1, 2, 3, 4, 5],
     [2, 4, 5, 1, 4],
     [3, 5, 4, 2, 1],
     [4, 1, 2, 5, 3],
     [5, 3, 1, 3, 2]] #1st and 5th elements of second column intechanged


>>> check_sudoku(a)
True
>>> check_sudoku(b)
False
>>> check_sudoku(c)
False
share|improve this answer
    
+1 for zip(*z)! –  Yulong Feb 5 '14 at 23:47
up vote 4 down vote 
def check_sudoku(sudlist):
    x = range(1, len(sudlist)+1) # makes a list of each number to be found'

Avoid storing things in single-letter variable names. It just makes it hearder to folllow your code

    rows = [[row[i] for row in sudlist] for i in range(len(sudlist))] # assigns all the rows to a flat list

You could actually do rows = zip(*sudlist) which will do the same thing. Admittedly, if you've not seen the trick before its non-obvious.

    z = range(len(sudlist))
    for num in x:

There's not really any reason to store the list in x, and then iterate over it later. Just combine the lines.

        for pos in z:

Instead use for column, row in zip(sudlist, rows): and you won't need to use the pos index

            if num not in sudlist[pos] or num not in rows[pos]:
                return False
    return True
share|improve this answer
1  
You're right that the OP's variable names are poorly chosen, but I don't think that "avoid single-letter names" is quite the right criterion. What names would you prefer here: def quadratic(a, b, c): return [(-b + sign * sqrt(b ** 2 - 4 * a * c)) / (2 * a) for sign in (-1,+1)] –  Gareth Rees Feb 18 '13 at 11:00
1  
@GarethRees, agreed that its not absolute, that's why I said "avoid" not "never do this" –  Winston Ewert Feb 18 '13 at 15:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.