Sudoku Solver - Recursive Solve

Any ideas on making my code a bit more clean and efficient? Any criticism is appreciated.

I don't like that I'm calling my lengthy DeepCopy method so many times, but the BinaryFormatter approach is much much slower. I haven't come up with a better approach than doing a deep copy of the entire SudokuBoard due to all of the interwoven pointers, but I'm sure one exists.

Cell

```
public class Cell
{
public int value { get; private set; }
public bool isBlank { get; set; }
public List possibilities { get; private set; }

public Section row { get; private set; }
public Section column { get; private set; }
public Section region { get; private set; }

public Cell(int v, bool b)
{
value = v;
isBlank = b;

if (!isBlank)
possibilities = new List();
else
possibilities = new List { 1, 2, 3, 4, 5, 6, 7, 8, 9 };
}

public void SetSections(Section r, Section c, Section reg)
{
row = r;
column = c;
region = reg;
}

public void AssignCellValue()
{
// Only continue if cell isn't a preset value
if (isBlank)
{
// If answer found, block cell from use and modify peer region possibilities
if (possibilities.Distinct().Count() == 1)
{
value = possibilities.First(); // Set value to only possibility
possibilities.Clear();
isBlank = false;

row.RefactorPossibilities(); // In peer regions change each cell's possibilities
column.RefactorPossibilities(); // to reflect value change
region.RefactorPossibilities();

row.SingleOut();
column.SingleOut();
region.SingleOut();
}
}
}

public void ForceCellValue(int value)
{
if (isBlank)
{
possibilities.Clear();
possibilities.Add(val

At a really quick glance at the Section.RefactorPossibilities() method I can give you some general suggestions

-
be consitent with the style you use. Here the first for loop encloses the body in braces {} but the second loop doesn't.

-
your comments here seems pointless, as they aren't commenting on why something is done.

public void RefactorPossibilities()
{
    for (int i = 0; i  1)
                    {
                        items[j].possibilities.Remove(items[i].value);
                    }

                    if (items[j].possibilities.Distinct().Count() == 1)     // if cell has only 1 possibility value left  
                    {
                        if (!items.Any(x => x.value == items[j].possibilities.First()))
                        {
                            items[j].AssignCellValue();                         // set cell to that value
                        }
                    }
                }
        }
    }
}

The same method refactored into two methods.

public void RefactorPossibilities()
{
    IList values = GetCurrentValues();

    for (int i = 0; i  x.value == items[i].possibilities.First()))
            {
                items[i].AssignCellValue();
            }
        }
    }
}
private IList GetCurrentValues()
{
    IList values = new List(item.Count);
    for (int i = 0; i < items.Count; i++)
    {
        if (items[i].isBlank) { continue; }
        values.Add(items[i].Value);
    }

    return values;
}

The GetCurrentValues() iterates over each item and if the item has a value it will be added to the list. The RefactorPossibilities() method takes these values and if a item doens't have a value it will remove each value from the possibilities.

As you see I have added guard clauses to the loops, to improve readability.

You should consider to add a RemovePosibilities(IList posibilities) to the Cell class. This would simplifiy the RefactorPossibilities() method and has the advantage that you could return a ReadOnlyCollection by the possibilities property of the Cell class.

Section.SingleOut() method

// Search the section for possibility values occurring once, and if found set it to specified cell
public void SingleOut()
{
    for (int i = 0; i ()
            .Where(x => x.isBlank)
            .Where(x => x.possibilities.Contains(i + 1))
            .Count();

        if (numSpecificPoss == 1)
        {
            Cell SingledOut = items.OfType()
                .Where(x => x.isBlank)
                .Where(x => x.possibilities.Contains(i + 1))
                .Single();

            //Console.WriteLine("Singled out");
            SingledOut.ForceCellValue(i + 1);
        }
    }
}

You can refactor the linq query to

IEnumerable cells = items.OfType()
                 .Where(x => x.isBlank)
                 .Where(x => x.possibilities.Contains(i + 1));

if (cells.Count() == 1)
{
    cells.Single().ForceCellValue(i + 1);
}

You can refactor the SectionVerified() method in a similiar way

public bool SectionVerified()
{
    IEnumerable cells = items.OfType()
            .Where(x => x.value > 0)
            .Select(x => x.value);

    return  (cells.Count() == cells.Distinct().Count());
}

SudokuBoard.Solved() method

public bool Solved()
{
    int regionSum = 0;
    int columnSum = 0;
    int rowSum = 0;
    bool legitimateSolution = true;

    for (int i = 0; i  x.value);
        columnSum = columns[i].items.Sum(x => x.value);
        rowSum = rows[i].items.Sum(x => x.value);

        if ((regionSum != 45) || (columnSum != 45) || (rowSum != 45))
        {
            legitimateSolution = false;
            break;
        }
    }

    if (legitimateSolution)
        return true;
    else
        return false;
}

Issues

• You are using magic numbers here. You should hide them behind a const.

-
If regionSum != 45 you won't need to sum columns and rows.

private const int solvedSum = 45;
public bool Solved()
{
    for (int i = 0; i  x.value) != solvedSum) { return false; }
        if (columns[i].items.Sum(x => x.value) != solvedSum) { return false; }
        if (rows[i].items.Sum(x => x.value) != solvedSum) { return false; }
    }

    return true;
}

Naming

Please check the Naming Guidlines.

• Properties should use PascalCasing for their names.

• Classes should be named using nouns. So Solve isn't a good name here. A better name would be SudokuSolver

General

• Avoid single letter variable names like public Cell(int v, bool b). Single letters can be used as loop iterators and are usually i,j,k.

• Again, be consitent with the your coding style. At first I thought, ok, for single if and else statements no braces {} are used but they are used for single else if statment. But, after reading more of the code I also found single else if statments without braces and also single if statements using braces.

• Use guard cl

StackExchange Code Review Q#69463, answer score: 13