patterncsharpMajor
SudokuSharp Solver with advanced features
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
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
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:
Class summary
- SudokuFactory: Contains static methods to create some Sudoku variations
- SudokuBoard: Contains collection of
SudokuRuleand ofSudokuTile
- SudokuRule: Whether it's a box, a line, a row, or something entirely different doesn't matter. Contains a collection of
SudokuTilethat 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...
Should be:
When the first thing you see is this:
you wonder where
Speaking of the devil:
This line belongs just above the constructor that's using it (it's 30-some lines below its first usage).
This
You want to abuse LINQ? How about taking this:
And turning it into that:
It takes the
In
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
I like how your
That said, if your entire project is compiled into 1 single assembly (.exe/.dll), your usage of the
Not much left to say, except perhaps method
One last thing - this piece of list-initialization code:
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
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.