Sudoku Checker in Python

Question

I have written the following Sudoku checker in Python. I feel like this could be written much shorter and perhaps more efficient. Especially the part with square_columns.

rows = []
columns = []
squares = []
sudoku_sets = []
for i in range(9):
 if i == 0:
 row = input("Input the sudoku values row by row.\n")
 else:
 row = input("")
 while len(row) != 9 or not row.isnumeric():
 row = input(f"Wrong input. Please insert 9 numbers for row number {i+1}.\n")
 rows.append(row)
for i in range(len(rows)):
 column = ''
 for j in range (len(rows)):
 column += rows[j][i]
 columns.append(column)
for i in range(0,7,3):
 square_columns = ["", "", ""]
 for j in range(3):
 square_columns[0] += rows[j+i][:3]
 square_columns[1] += rows[j+i][3:6]
 square_columns[2] += rows[j+i][6:9]
 for square in square_columns:
 squares.append(square)
sudoku_sets.append(rows)
sudoku_sets.append(columns)
sudoku_sets.append(squares)
def check_sudoku(sets):
 for s in sets:
 for item in s:
 if len(item) == len(set(item)):
 continue
 else:
 return "No"
 return "Yes"
print(check_sudoku(sudoku_sets))

• kubatucka: Check out this answer from stackoverflow stackoverflow.com/a/17606526/13756061.

Answer 1

Your code looks quite nice. Still there is some room for improvements.

Do not define variables in a single block at the beginning

This is a style from ancient C. Now the rule is to declare variables

• in the smallest possible scope
• as late as possible

As you (currently) have a global scope only the latter rule applies. If you do

sudoku_sets = []
sudoku_sets.append(rows)
sudoku_sets.append(columns)
sudoku_sets.append(squares)

the reader does not have to scan the 30 lines above if sudoku_sets is touched somewhere. It is absolutely clear what it holds.

Do not try to outsmart the language

for i in range(0,7,3):

is a little strange. If you want to loop a range of 9 in steps of 3 - write it like that.

for i in range(0, 9, 3):

The language is designed to allow to write code in a way that does not require error prone hand knitting and fiddling.

Constants

Your checker only works for a square size of 9. That could be stated early in the file. While some of your code tries to be size agnostic there are many lines where hard coded numbers work for size 9 only.

# rows fixed to nine 
for i in range(9):
# cols agnostic (but fixed to square)
for i in range(len(rows)):

Functions

Functions are good to reduce scope and introduce better readability. Your code is nicely(!) structured into atomic tasks. Still one has to read and understand the code to recognize. We can do an improvement and do a comment for each block or we can do a great improvement by extract the blocks into functions with a good name. We do 3 functions

def read_rows():
 ...
 return rows
 
def make_cols(rows):
 ...
 return cols
def make_squares(rows):
 ...
 return squares

That functions are self-contained and name the task and declare the dependencies. make_squares makes squares out of rows. It does not require columns and it does not fiddle with any other variable. When we use those functions we get a nicely readable top level code body.

rows = read_rows()
columns = make_cols(rows)
squares = make_squares(rows)
check_sudoku_sets([rows, columns, squares])

Also the nasty list initialisations are gone. They are inside the named functions and do not clutter the top level scope any more.

sudoku_sets

... is a bad name. It does not explain what it holds. Also there is no need for it as rows, cols and squares are not treated differently in the check. All 3 hold "regions". So we can do

regions = rows + columns + squares

and simplify the check to

def check_sudoku(regions):
 for item in regions:
 if len(item) == len(set(item)):
 continue
 else:
 return "No"
 return "Yes"

Do not I/O in your algorithms

That reduces usability and testability. You nearly did it right in your check_sudoku as there is no output. Still there is coupling to the output as you return strings designed for output. Return a bool instead and rename the function to a "predicate style".

def is_valid_sudoku(regions):
 for item in regions:
 if len(item) == len(set(item)):
 continue
 else:
 return False
 return True

which can be shortened to

def is_valid_sudoku(regions):
 return all((len(item) == len(set(item)) for item in regions))

The translation to string shall be done in the output.

Result so far

The code now looks like

# this implementation works for size = 9 only
def read_rows():
 rows = []
 for i in range(9):
 if i == 0:
 row = input("Input the sudoku values row by row.\n")
 else:
 row = input("")
 while len(row) != 9 or not row.isnumeric():
 row = input(f"Wrong input. Please insert 9 numbers for row number {i + 1}.\n")
 rows.append(row)
 return rows
def make_cols(rows):
 columns = []
 for i in range(len(rows)):
 column = ''
 for j in range(len(rows)):
 column += rows[j][i]
 columns.append(column)
 return columns
def make_squares(rows):
 squares = []
 for i in range(0, 7, 3):
 square_columns = ["", "", ""]
 for j in range(3):
 square_columns[0] += rows[j + i][:3]
 square_columns[1] += rows[j + i][3:6]
 square_columns[2] += rows[j + i][6:9]
 for square in square_columns:
 squares.append(square)
 return squares
def is_valid_sudoku(regions):
 return all((len(item) == len(set(item)) for item in regions))
rows = read_rows()
columns = make_cols(rows)
squares = make_squares(rows)
regions = rows + columns + squares
text = "Yes" if is_valid_sudoku(regions) else "No"
print(text)

Loops

Avoid looping over indices but loop over elements. In make_cols do not loop like

for j in range(len(rows)):
 column += rows[j][i]

but do

for row in rows:
 column += row[i]

If you need the index as well loop like

for i, e in enumerate(some_list):

Algorithms

You are concerned about make_squares. A simpler implementation would be to walk all cells by index and append the found digit to the corresponding square

def make_squares(rows):
 squares = ["" for _ in range(9)]
 for y, r in enumerate(rows):
 for x, e in enumerate(r):
 squares[(y//3)*3 + x//3] += e
 return squares

More nitpicking

In your input function you have a special case for the first input line - pull that out of the loop like

def read_rows():
 print("Input the sudoku values row by row")
 rows = []
 for i in range(9):
 row = input("")
 ...
 

Also you do not check for valid symbols completely, you also accept the digit '0'.