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

SudokuSharp Solver with advanced features

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

Problem

Even though it's the first time I'm writing something this "big", it feels like I know C# quite well (it is very similar to Java after all). It's been nice to learn LINQ also and I am very impressed by the features (which is just like Steams in Java 8), and perhaps I have overused it here (if it's possible to do that).

Class summary

  • SudokuFactory: Contains static methods to create some Sudoku variations



  • SudokuBoard: Contains collection of SudokuRule and of SudokuTile



  • SudokuRule: Whether it's a box, a line, a row, or something entirely different doesn't matter. Contains a collection of SudokuTile that must be unique.



  • SudokuTile: Each tile in the puzzle. Can be "blocked" (like a hole in the puzzle), remembers it's possibleValues, and also contains a value (0 is used for tiles without a value)



  • SudokuProgress: Used to know what the progress of a solving step was.



  • Program: Main starting point. Contains tests for seven different Sudokus. All have been verified to be solved correctly.



Since this is the first time I'm using C# and LINQ, please tell me anything. All suggestions welcome. Except for the fact that the method box should be called Box. I'd be especially interested in cases where I could simplify some of the LINQ usage (trust me, there is a lot). I hope you are able to follow all the LINQ queries. I have tried to put some short comments where needed to explain what is happening. If you want an explanation for some part, post a comment and I will explain.

As usual, I have a tendency to make the challenge into something super-flexible with support for a whole lot of more or less unnecessary things. Some of the possible puzzles that this code can solve is:

  • A hard classic 9x9 Sudoku with 3x3 boxes that requires more advanced techniques (or in my case, more or less "brute force" by trial and error)



  • Nonomino



  • HyperSudoku



  • Samurai Sudoku



  • A classic Sudoku of any size with any number of boxes and size of boxes (only completely

Solution

Impressive. I mean it.

Couple observations:

Your enums...

public enum SudokuProgress { FAILED, NO_PROGRESS, PROGRESS }


Should be:

public enum SudokuProgress { Failed, NoProgress, Progress }


When the first thing you see is this:

public class SudokuBoard
{
    public SudokuBoard(SudokuBoard copy)
    {
        _maxValue = copy._maxValue;
        tiles = new SudokuTile[copy.Width, copy.Height];
        CreateTiles();


you wonder where _maxValue and tiles come from, and why _maxValue (whose naming convention is that of a private field) can be accessed like that - I would expose it as a get-only property. Accessing private fields from another object doesn't seem instinctively right to me.

Speaking of the devil:

private int _maxValue;


This line belongs just above the constructor that's using it (it's 30-some lines below its first usage).

This box method which should be named Box (actually box is a bad name because it's the name of a CIL instruction that your C# compiles to), is returning a not-so-pretty Tuple - The framework has a type called Point which has X and Y properties; if that's not appropriate, I don't know what is. Side note, Point is a value type, so there's no boxing actually going on if you use it over a Tuple, which is a reference type (incurs boxing). Bottom line, use a Point and call that method something else:

public static IEnumerable Box(int sizeX, int sizeY)
{
    foreach (int x in Enumerable.Range(0, sizeX))
    {
        foreach (int y in Enumerable.Range(0, sizeY))
        {
            yield return new Point(x, y);
        }
    }
}


You want to abuse LINQ? How about taking this:

private SudokuTile[,] tiles;
private void CreateTiles()
{
    foreach (var pos in SudokuFactory.box(tiles.GetLength(0), tiles.GetLength(1)))
    {
        tiles[pos.Item1, pos.Item2] = new SudokuTile(pos.Item1, pos.Item2, _maxValue);
    }
}


And turning it into that:

private Dictionary tiles;
private void CreateTiles()
{
    tiles = SudokuFactory
                  .Box(tiles.GetLength(0), tiles.GetLength(1))
                  .Select(p => new KeyValuePair{ Key = p, Value = new SudokuTile(p.X, p.Y, _maxValue)})
                  .ToDictionary(kvp => pkv.Key, kvp => kvp.Value);
}


It takes the IEnumerable returned by the modified Box method, selects each point into the Key of a KeyValuePair and a new SudokuTile as the vale, and then ToDictionary selects the Enumerable into a dictionary, which gets assigned to tiles. (C#: 1, Java: 0) Lines of code: 1.

In SudokuRule, your private fields can be marked as readonly.

This is only a partial review, I'll write more after I've implemented my own solution - I purposely haven't looked at your puzzle-resolution code :)

Overall looks quite good (except for all that static stuff that doesn't need to be, but that's dependency-injection me talking, doesn't make it any worse c#, but testing might be more enjoyable with non-static dependencies), It's great that you gave C# a bit of lovin' this week. I know Visual Studio isn't Eclipse, but I can assure you that VS with ReSharper would have made it a similar experience (and could have shown you some LINQ tricks!), at least in terms of code inspections (R# makes VS actually better than Eclipse... but I'm biased, and drifting, so I'll keep it at that!)...

I like how your Solve() method yield returns all found solutions.

That said, if your entire project is compiled into 1 single assembly (.exe/.dll), your usage of the internal access modifier is equivalent to public - internal basically means "assembly scope", so an internal type or method cannot be accessed from another assembly; if there's no other assembly, everything in the project can "see" it, so I don't see a point for internal here.

Not much left to say, except perhaps method IsValuePossible might be better off as IsPossibleValue, but that's mere nitpicking. Very neat, I'm jealous.

One last thing - this piece of list-initialization code:

var queriesForBlocked = new List>();
queriesForBlocked.Add(from pos in box(BoxSize, BoxSize*2) select board.Tile(pos.Item1 + DefaultSize, pos.Item2                            ));
queriesForBlocked.Add(from pos in box(BoxSize, BoxSize*2) select board.Tile(pos.Item1 + DefaultSize, pos.Item2 + DefaultSize * 2 - BoxSize));
queriesForBlocked.Add(from pos in box(BoxSize*2, BoxSize) select board.Tile(pos.Item1                            , pos.Item2 + DefaultSize));
queriesForBlocked.Add(from pos in box(BoxSize*2, BoxSize) select board.Tile(pos.Item1 + DefaultSize * 2 - BoxSize, pos.Item2 + DefaultSize));


Could use a collection initializer and be written like this:

```
var queriesForBlocked = new List>
{
{ box(BoxSize, BoxSize*2).Select(pos => board.Tile(pos.Item1 + DefaultSize, pos.Item2)) },
{ box(BoxSize, BoxSize*2).Select(pos => board.Tile(pos.Item1 + DefaultSize, pos.Item2 + De

Code Snippets

public enum SudokuProgress { FAILED, NO_PROGRESS, PROGRESS }
public enum SudokuProgress { Failed, NoProgress, Progress }
public class SudokuBoard
{
    public SudokuBoard(SudokuBoard copy)
    {
        _maxValue = copy._maxValue;
        tiles = new SudokuTile[copy.Width, copy.Height];
        CreateTiles();
private int _maxValue;
public static IEnumerable<Point> Box(int sizeX, int sizeY)
{
    foreach (int x in Enumerable.Range(0, sizeX))
    {
        foreach (int y in Enumerable.Range(0, sizeY))
        {
            yield return new Point(x, y);
        }
    }
}

Context

StackExchange Code Review Q#37448, answer score: 49

Revisions (0)

No revisions yet.