Skip to main content
added 242 characters in body
Source Link

SudokuDigit should be a Value Object. Apart from arguments from domain some syntactical clues are also in your code. SudokuDigit.setValue is not called anywhere. When the value of a Digit withing a Grid needs to change you set it to another Digit and do not mutate the Digit instead. Once you are content that Digit is a value object, you do the following:

  • Mark all fields as final.
  • remove setters and other mutators. They give compile errors, so easy enough.

SudokuDigit(Integer) is the basic constructor and SudokuDigit() is a specialized one that depends on it. Putting the basic constructor above and specialized one below makes it easier to read top-down.

SudokuDigit(char) does too much for a constructor, that is more than assignment to the fiels. And also knows too much: It really needn't concern itself with whether '.' corresponds to and empty digit. It belongs with other parsing code. The code should just work whether I use a ' ' or '.' to represent some value. It can be turned to static factory method. And moved to wherever it is actually needed.

Same goes for validateDigitValue(Integer). That it can be made static is a tell. Marking anything static that can be marked static is a good rule of thumb. A static method can be moved easily around and awkward calling syntax is a reminder that that piece of code is not at its natural home. Dependence of the method on magic constant 9 is a sign that it probably belongs to a class with a field whose value would be 9. SudokuGrid.size maybe? A Sudoku game can be trivially generalized to 1X1, 4X4, 16X16 grids with same rules. But this method would cause reuse and testing trouble if we attempted that.

The same arguments go for separating parsing concern from other concerns, ie single responsibility principle, goes for SudokuGrid. AlsoThat is convert the constructors that do parsing to factory methods, e.g. convert SudokuGrid(String contents) to static SudokuGrid parseGrid(String contents). Then Move it to a SudokuParser class, for example.

A side note: making SudokuDigit immutable prevents misuse of SudokuGrid's API:

grid.get(coords).setDigit(-12345); // ???

You shouldn't leak references to internal state, as done in SudokuGrid.getRow. It violates encapsulation:

grid.getRow(0)[0] = new SudokuDigit(-12345);

You can return an Iterable<SudokuDigit> from each of getRow, getColumn and getRegion that are backed by unmodifiable collections; also making their return types more uniform, more abstract and more composable, all at the same time.

I assume the Point type is not java.awt.Point. If so; just define a new class SudokuCoordinate {int row, col;} in 10 secs, and deny ever having used java.awt.Point vehemently.

Lastly;

  • Remove Sudoku- prefix from your classes.
  • Put each type in a separate source file.
  • Use "open resource" (in Eclipse: ctrl+shift+r) instead of "open type" to find your types easily.

SudokuDigit should be a Value Object. Apart from arguments from domain some syntactical clues are also in your code. SudokuDigit.setValue is not called anywhere. When the value of a Digit withing a Grid needs to change you set it to another Digit and do not mutate the Digit instead. Once you are content that Digit is a value object, you do the following:

  • Mark all fields as final.
  • remove setters and other mutators. They give compile errors, so easy enough.

SudokuDigit(Integer) is the basic constructor and SudokuDigit() is a specialized one that depends on it. Putting the basic constructor above and specialized one below makes it easier to read top-down.

SudokuDigit(char) does too much for a constructor, that is more than assignment to the fiels. And also knows too much: It really needn't concern itself with whether '.' corresponds to and empty digit. It belongs with other parsing code. The code should just work whether I use a ' ' or '.' to represent some value. It can be turned to static factory method. And moved to wherever it is actually needed.

Same goes for validateDigitValue(Integer). That it can be made static is a tell. Marking anything static that can be marked static is a good rule of thumb. A static method can be moved easily around and awkward calling syntax is a reminder that that piece of code is not at its natural home. Dependence of the method on magic constant 9 is a sign that it probably belongs to a class with a field whose value would be 9. SudokuGrid.size maybe? A Sudoku game can be trivially generalized to 1X1, 4X4, 16X16 grids with same rules. But this method would cause reuse and testing trouble if we attempted that.

The same arguments go for separating parsing concern from other concerns, ie single responsibility principle, goes for SudokuGrid. Also making SudokuDigit immutable prevents misuse of API:

grid.get(coords).setDigit(-12345); // ???

You shouldn't leak references to internal state, as done in SudokuGrid.getRow. It violates encapsulation:

grid.getRow(0)[0] = new SudokuDigit(-12345);

You can return an Iterable<SudokuDigit> from each of getRow, getColumn and getRegion that are backed by unmodifiable collections; also making their return types more uniform, more abstract and more composable, all at the same time.

I assume the Point type is not java.awt.Point. If so; just define a new class SudokuCoordinate {int row, col;} in 10 secs, and deny ever having used java.awt.Point vehemently.

Lastly;

  • Remove Sudoku- prefix from your classes.
  • Put each type in a separate source file.
  • Use "open resource" (in Eclipse: ctrl+shift+r) instead of "open type" to find your types easily.

SudokuDigit should be a Value Object. Apart from arguments from domain some syntactical clues are also in your code. SudokuDigit.setValue is not called anywhere. When the value of a Digit withing a Grid needs to change you set it to another Digit and do not mutate the Digit instead. Once you are content that Digit is a value object, you do the following:

  • Mark all fields as final.
  • remove setters and other mutators. They give compile errors, so easy enough.

SudokuDigit(Integer) is the basic constructor and SudokuDigit() is a specialized one that depends on it. Putting the basic constructor above and specialized one below makes it easier to read top-down.

SudokuDigit(char) does too much for a constructor, that is more than assignment to the fiels. And also knows too much: It really needn't concern itself with whether '.' corresponds to and empty digit. It belongs with other parsing code. The code should just work whether I use a ' ' or '.' to represent some value. It can be turned to static factory method. And moved to wherever it is actually needed.

Same goes for validateDigitValue(Integer). That it can be made static is a tell. Marking anything static that can be marked static is a good rule of thumb. A static method can be moved easily around and awkward calling syntax is a reminder that that piece of code is not at its natural home. Dependence of the method on magic constant 9 is a sign that it probably belongs to a class with a field whose value would be 9. SudokuGrid.size maybe? A Sudoku game can be trivially generalized to 1X1, 4X4, 16X16 grids with same rules. But this method would cause reuse and testing trouble if we attempted that.

The same arguments go for separating parsing concern from other concerns, ie single responsibility principle, goes for SudokuGrid. That is convert the constructors that do parsing to factory methods, e.g. convert SudokuGrid(String contents) to static SudokuGrid parseGrid(String contents). Then Move it to a SudokuParser class, for example.

A side note: making SudokuDigit immutable prevents misuse of SudokuGrid's API:

grid.get(coords).setDigit(-12345); // ???

You shouldn't leak references to internal state, as done in SudokuGrid.getRow. It violates encapsulation:

grid.getRow(0)[0] = new SudokuDigit(-12345);

You can return an Iterable<SudokuDigit> from each of getRow, getColumn and getRegion that are backed by unmodifiable collections; also making their return types more uniform, more abstract and more composable, all at the same time.

I assume the Point type is not java.awt.Point. If so; just define a new class SudokuCoordinate {int row, col;} in 10 secs, and deny ever having used java.awt.Point vehemently.

Lastly;

  • Remove Sudoku- prefix from your classes.
  • Put each type in a separate source file.
  • Use "open resource" (in Eclipse: ctrl+shift+r) instead of "open type" to find your types easily.
Source Link

SudokuDigit should be a Value Object. Apart from arguments from domain some syntactical clues are also in your code. SudokuDigit.setValue is not called anywhere. When the value of a Digit withing a Grid needs to change you set it to another Digit and do not mutate the Digit instead. Once you are content that Digit is a value object, you do the following:

  • Mark all fields as final.
  • remove setters and other mutators. They give compile errors, so easy enough.

SudokuDigit(Integer) is the basic constructor and SudokuDigit() is a specialized one that depends on it. Putting the basic constructor above and specialized one below makes it easier to read top-down.

SudokuDigit(char) does too much for a constructor, that is more than assignment to the fiels. And also knows too much: It really needn't concern itself with whether '.' corresponds to and empty digit. It belongs with other parsing code. The code should just work whether I use a ' ' or '.' to represent some value. It can be turned to static factory method. And moved to wherever it is actually needed.

Same goes for validateDigitValue(Integer). That it can be made static is a tell. Marking anything static that can be marked static is a good rule of thumb. A static method can be moved easily around and awkward calling syntax is a reminder that that piece of code is not at its natural home. Dependence of the method on magic constant 9 is a sign that it probably belongs to a class with a field whose value would be 9. SudokuGrid.size maybe? A Sudoku game can be trivially generalized to 1X1, 4X4, 16X16 grids with same rules. But this method would cause reuse and testing trouble if we attempted that.

The same arguments go for separating parsing concern from other concerns, ie single responsibility principle, goes for SudokuGrid. Also making SudokuDigit immutable prevents misuse of API:

grid.get(coords).setDigit(-12345); // ???

You shouldn't leak references to internal state, as done in SudokuGrid.getRow. It violates encapsulation:

grid.getRow(0)[0] = new SudokuDigit(-12345);

You can return an Iterable<SudokuDigit> from each of getRow, getColumn and getRegion that are backed by unmodifiable collections; also making their return types more uniform, more abstract and more composable, all at the same time.

I assume the Point type is not java.awt.Point. If so; just define a new class SudokuCoordinate {int row, col;} in 10 secs, and deny ever having used java.awt.Point vehemently.

Lastly;

  • Remove Sudoku- prefix from your classes.
  • Put each type in a separate source file.
  • Use "open resource" (in Eclipse: ctrl+shift+r) instead of "open type" to find your types easily.