2
\$\begingroup\$

I have an assignment for writing a Sudoku solver and have achieved the functionality required but a lot of the code is "ugly" and I'm wanting to simplify it but unsure how to.

def hiddenSingleCol(snapshot, cell):
    j = cell.getCol()                           # Get column number for cell
    colCells = snapshot.cellsByCol(j)           # Get all cells in that column
    for num in cell.getAvailNum():              # For each available number for the cell we are looking at
        hidden = True                           # Is it a hidden single? (set hidden as True to start)
        for tempCell in colCells:               # For each cell in the list of cells
            if tempCell == cell:                # If it is our cell, ignore
                pass
            elif num in tempCell.getAvailNum(): # If one of the available num is the same
                hidden = False                  # Set hidden to false
                break
        if hidden: 
            cell.setAvailNum(num)               # If hidden is still true, set available num to num
            break

More code snippets:

    for cell in cUnsolved:                          # we're only interested in cells without assigned values
    updateAvailNums(snapshot, cell)             # update the list of potential answers for each cell

for cell in cUnsolved:
    hiddenSingleCol(snapshot, cell)
    hiddenSingleRow(snapshot, cell)
    hiddenSingleBlock(snapshot, cell)

for cell in cUnsolved:
    if (len(cell.getAvailNum()) == 0):          # and if any have no possible numbers, this is a failed solution
        return None
    if (len(cell.getAvailNum()) == 1):          # and if there are any with only one possibility, we do this first
        x = cell.getRow()
        y = cell.getCol()
        break
    elif (len(cell.getAvailNum()) < minNum):    # otherwise we are interested in the cell with the least possible numbers so far
        x = cell.getRow()
        y = cell.getCol()
        minNum = len(cell.getAvailNum())

Not only are the multiple for-loops in here ugly, but I have 2 other versions, slightly altered, for the row and 3x3 blocks. I've tried looking at other code snippets on this but I seem incapable of applying it to my own work.

\$\endgroup\$

closed as off-topic by t3chb0t, Sᴀᴍ Onᴇᴌᴀ, Graipher, Gerrit0, Quill Dec 20 '18 at 0:03

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, Sᴀᴍ Onᴇᴌᴀ, Graipher, Gerrit0, Quill
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 5
    \$\begingroup\$ Hi. Welcome to Code Review! You might consider posting more of your code. It may be that people will suggest alternate approaches other than using hiddenSingleCol and your other functions. If you only post a single function, then you may be missing suggestions about improving the way that the function is used. \$\endgroup\$ – Brythan Mar 11 '15 at 21:43
  • 2
    \$\begingroup\$ I suspect you can just post the whole thing here; a sudoku solver isn't a massive thing and relatively long code isn't a problem here. Or, if it is a massive thing, all the more to help with ;). \$\endgroup\$ – Veedrac Mar 12 '15 at 9:12
  • \$\begingroup\$ The reason I am not willing to post the complete thing is because it is made in pygame and there is a lot of additional code. Also, this is current homework and this is more about wanting some help condensing my code (and not wanting to risk having someone going "oi!") \$\endgroup\$ – Syzorr Mar 12 '15 at 12:15
5
\$\begingroup\$

A for statement can have an else clause which suits your needs perfectly and allows you to eliminate the variable hidden. I would also prefer a simple if instead of the if pass elif construct:

for num in cell.getAvailNum():              # For each available number for the cell we are looking at
    for tempCell in colCells:               # For each cell in the list of cells
        if tempCell != cell and num in tempCell.getAvailNum(): # If one of the available num is the same
            break
    else: 
        cell.setAvailNum(num)               # If hidden is still true, set available num to num
        break
\$\endgroup\$
2
\$\begingroup\$

Try to extract functions, even small ones, that helps fighting "ugliness". For example, you can have function find_hidden_num_in_cells(cells). And have functions cells_in_same_row(cell), cells_in_same_column(cell), cells_in_same_3x3_area(cell), which could return list or yeild an iterator, all the cells except the given one.

In second exapmle you can start with just deviding function with one for in each, eg. hidden_single_areas(unresolved), find_next_cell_to_try(unresolved)

\$\endgroup\$
2
\$\begingroup\$

Replace this:

for cell in cUnsolved:
    if (len(cell.getAvailNum()) == 0):          # and if any have no possible numbers, this is a failed solution
        return None
    if (len(cell.getAvailNum()) == 1):          # and if there are any with only one possibility, we do this first
        x = cell.getRow()
        y = cell.getCol()
        break
    elif (len(cell.getAvailNum()) < minNum):    # otherwise we are interested in the cell with the least possible numbers so far
        x = cell.getRow()
        y = cell.getCol()
        minNum = len(cell.getAvailNum())

with this:

for cell in cUnsolved:
    avail = len(cell.getAvailNum())
    if avail == 0:          # and if any have no possible numbers, this is a failed solution
        return None
    x = cell.getRow()
    y = cell.getCol()
    if avail == 1:          # and if there are any with only one possibility, we do this first
        break
    if avail < minNum:    # otherwise we are interested in the cell with the least possible numbers so far
        minNum = avail

Explanation:

  • Remove multiple calls to cell.getAvailNum() by storing the result in a variable avail.
  • Remove duplicate calls to set x and y by moving them out of the if statements.
  • Remove parentheses in if statements.

Improvement based on the comment below:

for cell in cUnsolved:
    avail = len(cell.getAvailNum())
    if avail == 0:        # and if any have no possible numbers, this is a failed solution
        return None
    if avail < minNum:    # otherwise we are interested in the cell with the least possible numbers so far
        x = cell.getRow()
        y = cell.getCol()
        if avail == 1:    # and if there are any with only one possibility, we do this first
            break
        minNum = avail
\$\endgroup\$
  • \$\begingroup\$ The problem with this change is that by moving the x and y out of the if statements, it means that we're no longer getting the x and the y value for the cell with the lowest amount of available options \$\endgroup\$ – Syzorr Mar 13 '15 at 4:40
  • \$\begingroup\$ You're right. See my second version. This also removes the duplicate lines from your original version. \$\endgroup\$ – Brent Washburne Mar 13 '15 at 5:07

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