Hello Java World ~> Parsing a Sudoku Grid

Question

This is my first, very-very first attempt at java. I haven't started tackling the actual resolution of the sudoku puzzle (not even sure where to start, haven't looked at other weekend-challenge entries yet). I don't have much to go over here, but I'm sure it's filled with beginner mistakes already, so before I make things any worse I'd like to get a review of what I have.

The puzzle being parsed is the one provided as an example in the winning weekend-challenge suggestion.

public class SudokuGame {
 private final static String newLine = System.lineSeparator();
 private final static String puzzle = 
 "...84...9" + newLine +
 "..1.....5" + newLine +
 "8...2146." + newLine +
 "7.8....9." + newLine +
 "........." + newLine +
 ".5....3.1" + newLine +
 ".2491...7" + newLine +
 "9.....5.." + newLine +
 "3...84...";
 public static void main(String[] args){
 SudokuGrid grid = new SudokuGrid(puzzle);
 System.out.print(grid);
 }
}

The program takes the string and outputs this:

+===========+===========+===========+
| | | | 8 | 4 | | | | 9 |
-------------------------------------
| | | 1 | | | | | | 5 |
-------------------------------------
| 8 | | | | 2 | 1 | 4 | 6 | |
+===========+===========+===========+
| 7 | | 8 | | | | | 9 | |
-------------------------------------
| | | | | | | | | |
-------------------------------------
| | 5 | | | | | 3 | | 1 |
+===========+===========+===========+
| | 2 | 4 | 9 | 1 | | | | 7 |
-------------------------------------
| 9 | | | | | | 5 | | |
-------------------------------------
| 3 | | | | 8 | 4 | | | |
+===========+===========+===========+

Here's the SudokuGrid class:

/** A 9x9 Sudoku grid */
public class SudokuGrid {
 private SudokuDigit[][] digits;
 /** Creates a new SudokuGrid. Specified array must be 9x9. */
 public SudokuGrid(SudokuDigit[][] digits){
 this.digits = digits;
 }
 /** Creates a new SudokuGrid from specified string, assumed to be 9 lines of 9 characters. */
 public SudokuGrid(String contents) {
 this(contents.split("\\r?\\n"));
 }
 /** Creates a new SudokuGrid from specified string array, assumed to be 9 strings of 9 characters. */
 public SudokuGrid(String[] contents) {
 this(parseStringContent(contents));
 }
 private static SudokuDigit[][] parseStringContent(String[] contents) {
 SudokuDigit[][] result = new SudokuDigit[9][9];
 for (int i = 0; i < 9; i++) {
 String row = contents[i];
 for (int j = 0; j < 9; j++) {
 result[i][j] = new SudokuDigit(row.charAt(j));
 }
 }
 return result;
 }
 /** Sets the value of specified grid coordinates. */
 public void setDigit(Point coordinates, Integer value) {
 setDigit(coordinates, new SudokuDigit(value));
 }
 /** Sets the value of specified grid coordinates. */
 public void setDigit(Point coordinates, SudokuDigit value) {
 this.digits[coordinates.x][coordinates.y] = value;
 }
 /** Gets the SudokuDigit at specified grid coordinates. */
 public SudokuDigit getDigit(Point coordinates) {
 return this.digits[coordinates.x][coordinates.y];
 }
 /** Gets all SudokuDigits in specified row (0-8). */
 public SudokuDigit[] getRow(int row) {
 return this.digits[row];
 }
 /** Gets all SudokuDigits in specified column (0-8). */
 public SudokuDigit[] getColumn(int column) {
 SudokuDigit[] result = new SudokuDigit[9];
 for (int row = 0; row < 9; row++) {
 result[row] = this.digits[row][column];
 }
 return result;
 }
 /** Gets all SudokuDigits in specified 3x3 region (x: 0-2, y: 0-2). */
 public SudokuDigit[][] getRegion(Point coordinates) {
 SudokuDigit[][] result = new SudokuDigit[3][3];
 int startRow = coordinates.x * 3;
 int startCol = coordinates.y * 3;
 for (int row = 0; row < 3; row++) {
 for (int col = 0; col < 3; col++) {
 result[row][col] = this.digits[startRow + row][startCol + col];
 }
 }
 return result;
 }
 public String toString() {
 final String newLine = System.lineSeparator();
 String result = "+===========+===========+===========+" + newLine;
 for (int row = 0; row < this.digits.length; row++) {
 result = result.concat("|");
 for (int col = 0; col < this.digits[row].length; col++) {
 result = result.concat(" " + this.digits[row][col].toString() + " |");
 }
 if ((row + 1) % 3 == 0) {
 result = result.concat(newLine + "+===========+===========+===========+" + newLine);
 }
 else {
 result = result.concat(newLine + "-------------------------------------" + newLine);
 }
 }
 return result;
 }
}

And this is my SudokuDigit class:

/** An object that represents a single digit in a SudokuGrid, with or without a value. */
public class SudokuDigit {
 private Integer digit;
 /** Creates a new, empty value */
 public SudokuDigit() {
 this((Integer)null);
 }
 /** Creates a new digit from specified character. */
 public SudokuDigit(char value) {
 Integer numericValue;
 if (value == '.') {
 numericValue = null;
 }
 else {
 numericValue = Integer.parseInt(String.valueOf(value));
 }
 this.digit = numericValue;
 }
 /** Creates a new digit with specified value */
 public SudokuDigit(Integer value) {
 this.digit = value;
 }
 /** Gets the current value of the digit. Null if no value is specified. */
 public Integer getDigit() {
 return this.digit;
 }
 /** Sets the current value of the digit.
 * Specify null for no value. 
 * Throws UnsupportedOperationException if value is not valid.*/ 
 public void setDigit(Integer value) {
 if (!validateDigitValue(value)) throw new UnsupportedOperationException();
 this.digit = value;
 }
 private boolean validateDigitValue(Integer value) {
 return value == null || (value >= 1 && value <= 9); 
 }
 public String toString() {
 return this.digit != null ? this.digit.toString() : " ";
 }
}

This is really a "Hello World!" thing, but I want this code to be the grounds for an actual Sudoku resolver (which I'll post later, when/if I manage to put it together!). Any blatant mistakes that will drive me into a brick wall?

Answer 1

• Instead of your String puzzle you could use an String[] like below (not saying you have to, but you should know that the alternative exists). You are currently converting it to a String array later anyway, so why not have it as a String array right from the start?

  private final static String[] puzzle = new String[]{ "...84...9", "..1.....5", ... };
  

• This can be marked with final: private SudokuDigit[][] digits; to prevent any malicious programmer from even trying to use this.digits = null after it has once been initialized, or initialize a new 2-dimensional array

• 3 is a magic number. Yes it is, it's a magic number. In the past in the present and the future, I say three, is a magic number.

• 9 is also magic number. This means that you should use it as a constant:

  public static final int MAGIC_NUMBER = 9;
  

• A double for-loop with variables i and j can be called x and y instead, to avoid any possible confusion about which one is which.

• UnsupportedOperationException might not be the best exception to throw in your setDigit method. IllegalArgumentException would be better, since the argument to the method is incorrect.

• Simplification possible in your SudokuDigit char constructor:

  numericValue = value == '.' ? null : Integer.parseInt(String.valueOf(value));
  

• When writing JavaDoc, I believe it's convention to use it on multiple lines. Or at least that's more common than using only one line (Also, did you know that you could write /** before a method in Eclipse and then press enter to make Eclipse write the basic "layout" of it for you?)

  /**
   * Creates a new digit from specified character.
   */
  public SudokuDigit(char value) {
  

• You can use @throws in Javadoc to specify What exception can be thrown, and why.

  /** 
   * Sets the current value of the digit.
   * Specify null for no value. 
   * @throws UnsupportedOperationException if value is not valid.
   */ 
  public void setDigit(Integer value) {
  

• You can use Character.digit(char c, int radix) instead of Integer.parseInt to convert your chars to int. Such as Character.digit(value, 10)

If I wouldn't have known it, I wouldn't be able to tell that you are a Java beginner. You seem to be following nearly all of the common conventions and seem to be doing things well overall. Not much to complain about, really.

• Mathieu Guindon: +1 for the magic numbers. I realized that was going to bite me when I saw you resolve a {{1,0},{0,1}} "Sudoku puzzle"...
• Simon Forsberg: @retailcoder Thanks to reviewing your code, I remembered to constantify my magic numbers before submitting my code. Otherwise I would very likely also have been told the same thing.

Answer 2

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.