Skip to main content
no need for obnoxious EDIT banners
Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 194
  • 468

EDIT

END EDIT

EDIT

END EDIT

added 1198 characters in body
Source Link

EDIT

Your model does not differentiate between an empty board and a partial assignment; A position of a board and an assignment to that position you call both Tile. You call Tuple<int,int> position in places.

  • _rowAddIndex looks like an orphan. It clearly do not belong to that class.

  • AddRow should be called some exact number of times.

  • Before calling anything else. Seems like a constructor to me.

Also there is not differentiation between a puzzle and its solving method. A puzzle is an immutable structure. Every one trying to solve some puzzle on the newspaper is trying to solve the same puzzle. They use different methods and use different temporary data structures.

Board board = samuraiSudokuBoard.create();
PartialAssignment puzzle = new PartialAssignment(board, parse(puzzleStr));
SolutionStrategy strategy = new RecursiveStrategy(maxDepth);
var solutions = strategy.Solve(puzzle);
var solutions2 = new IterativeStrategy(maxStackSize).Solve(puzzle);
var solutions2 = new ConcurrentStrategy(maxThreads).Solve(puzzle);
var comparison = CompareSolutionTimes(puzzle, strategies);

END EDIT

EDIT

Your model does not differentiate between an empty board and a partial assignment; A position of a board and an assignment to that position you call both Tile. You call Tuple<int,int> position in places.

  • _rowAddIndex looks like an orphan. It clearly do not belong to that class.

  • AddRow should be called some exact number of times.

  • Before calling anything else. Seems like a constructor to me.

Also there is not differentiation between a puzzle and its solving method. A puzzle is an immutable structure. Every one trying to solve some puzzle on the newspaper is trying to solve the same puzzle. They use different methods and use different temporary data structures.

Board board = samuraiSudokuBoard.create();
PartialAssignment puzzle = new PartialAssignment(board, parse(puzzleStr));
SolutionStrategy strategy = new RecursiveStrategy(maxDepth);
var solutions = strategy.Solve(puzzle);
var solutions2 = new IterativeStrategy(maxStackSize).Solve(puzzle);
var solutions2 = new ConcurrentStrategy(maxThreads).Solve(puzzle);
var comparison = CompareSolutionTimes(puzzle, strategies);

END EDIT

added 562 characters in body
Source Link

You can write your Box method without foreach loops or yield return:

public static IEnumerable<Tuple<int, int>> Box(int sizeX, int sizeY)
{
    return 
        from x in Enumerable.Range(0, sizeX)
        from y in Enumerable.Range(0, sizeY)
        select Tuple.Create(x,y);
}

or equivalently:

public static IEnumerable<Tuple<int, int>> Box2(int sizeX, int sizeY)
{
    return 
        Enumerable.Range(0,sizeX).SelectMany(x => 
        Enumerable.Range(0,sizeY).Select(y => 
        Tuple.Create(x,y)));
}

Standard, and less verbose, method of creating Tuples is Tuple.Create.

If you had made your Tile, Rule, and Board immutable, as they are; sharing state between them would not have been a problem. For example two Samurai Sudoku puzzles do share rules.

You are aggregating here.

SudokuProgress result = SudokuProgress.NO_PROGRESS;
foreach (SudokuTile tile in withoutNumber)
    result = SudokuTile.CombineSolvedState(result, 
                 tile.RemovePossibles(existingNumbers));
return result;

can be rewritten more clearly as:

return withoutNumber.Aggregate(
    SudokuProgress.NO_PROGRESS,
    (result, tile) => SudokuTile.CombineSolvedState(
                         result, 
                         tile.RemovePossibles(existingNumbers)));

You can write your Box method without foreach loops or yield return:

public static IEnumerable<Tuple<int, int>> Box(int sizeX, int sizeY)
{
    return 
        from x in Enumerable.Range(0, sizeX)
        from y in Enumerable.Range(0, sizeY)
        select Tuple.Create(x,y);
}

or equivalently:

public static IEnumerable<Tuple<int, int>> Box2(int sizeX, int sizeY)
{
    return 
        Enumerable.Range(0,sizeX).SelectMany(x => 
        Enumerable.Range(0,sizeY).Select(y => 
        Tuple.Create(x,y)));
}

Standard, and less verbose, method of creating Tuples is Tuple.Create.

If you had made your Tile, Rule, and Board immutable, as they are; sharing state between them would not have been a problem. For example two Samurai Sudoku puzzles do share rules.

You can write your Box method without foreach loops or yield return:

public static IEnumerable<Tuple<int, int>> Box(int sizeX, int sizeY)
{
    return 
        from x in Enumerable.Range(0, sizeX)
        from y in Enumerable.Range(0, sizeY)
        select Tuple.Create(x,y);
}

or equivalently:

public static IEnumerable<Tuple<int, int>> Box2(int sizeX, int sizeY)
{
    return 
        Enumerable.Range(0,sizeX).SelectMany(x => 
        Enumerable.Range(0,sizeY).Select(y => 
        Tuple.Create(x,y)));
}

Standard, and less verbose, method of creating Tuples is Tuple.Create.

If you had made your Tile, Rule, and Board immutable, as they are; sharing state between them would not have been a problem. For example two Samurai Sudoku puzzles do share rules.

You are aggregating here.

SudokuProgress result = SudokuProgress.NO_PROGRESS;
foreach (SudokuTile tile in withoutNumber)
    result = SudokuTile.CombineSolvedState(result, 
                 tile.RemovePossibles(existingNumbers));
return result;

can be rewritten more clearly as:

return withoutNumber.Aggregate(
    SudokuProgress.NO_PROGRESS,
    (result, tile) => SudokuTile.CombineSolvedState(
                         result, 
                         tile.RemovePossibles(existingNumbers)));
Source Link
Loading