7
\$\begingroup\$

I've created two classes that contain only sudoku logic. One class Sudoku generates sudoku grid and returns SudokuGrid object with has specific number of values in it(removing values from filled grid). Second class SudokuGrid contains all logic that player needs, checking whether any rules are broken. In my opinion Sudoku class turned out great, but SudokuGrid class not so much, I tried to clean it up as much as I could and this is best I could do:

SudokuGrid class:

package application;

import java.awt.*;
import java.util.*;
import java.util.List;
import java.util.stream.Collectors;

public class SudokuGrid {

    /**
     * Contains all values that grid can contain
     */
    public enum SudokuValue {
        ONE("1"),
        TWO("2"),
        THREE("3"),
        FOUR("4"),
        FIVE("5"),
        SIX("6"),
        SEVEN("7"),
        EIGHT("8"),
        NINE("9"),
        EMPTY(" ");

        public final String TEXT;

        private SudokuValue(String text) {
            this.TEXT = text;
        }
    }

    /**
     * Helps this class with point creating, validating
     */
    private static class SudokuPoint {

        /**
         * @param point - arbitrary point to check
         * @return true if point is ing grid else return false
         */
        private static boolean isValidPoint(Point point) {
            return point.x < SudokuGrid.ROWS && point.y < SudokuGrid.LINES;
        }

        /**
         * @param line - grids blocks line index(0,1,2)
         * @param row - grids blocks row index(0,1,2)
         * @param index - blocks element index
         * @return point in the grid
         */
        public static Point createBlockPoint(int line,int row,int index) {
            Point point = new Point();
            point.x = (line * BLOCK_SIZE) + (index / BLOCK_SIZE);
            point.y = (row * BLOCK_SIZE) + (index % BLOCK_SIZE);
            return point;
        }
    }

    /**
     * Total lines in a grid
     */
    public static final int LINES = 9;

    /**
     *  Total rows in a grid
     */
    public static final int ROWS = 9;

    /**
     * Height and width of a grid
     */
    public static final int BLOCK_SIZE = 3;

    /**
     * contains sudoku grid values
     * if value is zero then it is not set
     */
    private SudokuValue[][] grid;

    /**
     * Creates new Sudoku grid from existing grid
     * @param grid - filled sudoku grid, can contain empty(0) value
     */
    public SudokuGrid(SudokuValue grid[][]) {
        this.grid = grid;
    }

    /**
     * @param value - new value of grids point
     * @param point - x and y coordinates where new value will be applied
     */
    public void setField(SudokuValue value, Point point) {
        if(SudokuPoint.isValidPoint(point)) {
            grid[point.x][point.y] = value;
        } else {
            throw new RuntimeException("Parameters did not pass validation");
        }
    }

    /**
     * @return set of points with make contradiction
     * with sudoku rules. If there are not any contradictions present
     * then returns empty set
     */
    public Set<Point> getInvalidFields() {
        Set<Point> fields = new HashSet<>();
        fields.addAll(_getInvalidLineFields());
        fields.addAll(_getInvalidRowFields());
        fields.addAll(_getInvalidBlockFields());
        return fields;
    }

    /**
     * @return set of points with make contradiction in sudoku grid
     * if there are two same values in LINE returns these values coordinates
     * otherwise returns empty set
     */
    private Set<Point> _getInvalidLineFields() {
        Set<Point> fields = new HashSet<>();

        for(int line = 0; line < grid.length; line++) {
            final int x = line;
            List<SudokuValue> lineValues = Arrays.asList(grid[line]);

            fields.addAll(_getDuplicateIndexes(lineValues)
                .parallelStream()
                .map(y -> new Point(x, y))
                .collect(Collectors.toSet()
            ));
        }
        return fields;
    }

    /**
     *
     * @return set of points with make contradiction in sudoku grid
     * if there are two same values in ROW. returns these values coordinates
     * otherwise returns empty set
     */
    private Set<Point> _getInvalidRowFields() {
        Set<Point> fields = new HashSet<>();

        for(int row = 0; row < grid.length; row++) {
            final int y = row;
            List<SudokuValue> lineValues = _getRowAsArray(row);

            fields.addAll(_getDuplicateIndexes(lineValues)
                .parallelStream()
                .map(x -> new Point(x, y))
                .collect(Collectors.toSet()
            ));
        }
        return fields;
    }

    /**
     * @param row - grids row
     * @return new array that will contain all rows values
     */
    private List<SudokuValue> _getRowAsArray(int row) {
        List<SudokuValue> ret = new ArrayList<>();
        for(int index = 0; index < grid.length; index++) {
            ret.add(grid[index][row]);
        }
        return ret;
    }

    /**
     * @return set of points with make contradiction in sudoku grid
     * if there are two same values in BLOCK(3x3) returns these values coordinates
     * otherwise returns empty set
     */
    private Set<Point> _getInvalidBlockFields() {
        Set<Point> fields = new HashSet<>();

        for(int line = 0; line < BLOCK_SIZE; line++) {
            for(int row = 0; row < BLOCK_SIZE; row++) {
                fields.addAll(_getInvalidFieldsInBlock(line, row));
            }
        }
        return fields;
    }

    /**
     * @param blockLine - grids blocks line index(0,1,2)
     * @param blockRow - grids blocks row index(0,1,2)
     * @return set of points from grid that are duplicates in block
     */
    private Set<Point> _getInvalidFieldsInBlock(int blockLine, int blockRow) {
        List<SudokuValue> values = _getBlockAsList( blockLine, blockRow);
        return _getDuplicateIndexes(values)
            .parallelStream()
            .map(i -> SudokuPoint.createBlockPoint(blockLine, blockRow, i))
            .collect(Collectors.toSet());
    }

    /**
     * @param blockLine - grids blocks line index(0,1,2)
     * @param blockRow - grids blocks row index(0,1,2)
     * @return all values in the list that are in the block.
     * for example:
     *  4   5   6
     *  0   0   0
     *  1   2   3
     *  will return: [4,5,6,0,0,0,1,2,3]
     */
    private List<SudokuValue> _getBlockAsList(int blockLine, int blockRow) {
        List<SudokuValue> list = new ArrayList<>();
        for(int x = 0; x < BLOCK_SIZE; x++) {
            for(int y = 0; y < BLOCK_SIZE; y++) {
                list.add(grid[(blockLine * BLOCK_SIZE) + x][(blockRow * BLOCK_SIZE) + y]);
            }
        }
        return list;
    }

    /**
     * @param list - list in which duplicates will be searched
     * @return set of list indexes, that are duplicates for example:
     * [3,4,6,5,3] will return ==> [0,4]
     * note it ignores SudokuValue.EMPTY values
     */
    Set<Integer> _getDuplicateIndexes(List<SudokuValue> list) {
        Set<Integer> ret = new HashSet<>();

        for(int pointer = 0; pointer < list.size(); pointer++) {
            SudokuValue currNumber = list.get(pointer);
            for(int i = 0; i < list.size(); i++) {
                if(currNumber == list.get(i) && i > pointer && list.get(i) != SudokuValue.EMPTY) {
                    ret.add(i);
                    ret.add(pointer);
                    break;
                }
            }
        }
        return ret;
    }

    /**
     * prints grid in console
     */
    public void printGrid() {
        for(int index = 0; index < grid.length; index++) {
            for(int index2 = 0; index2 < grid[index].length; index2++) {
                System.out.print(grid[index][index2].TEXT + "  ");
            }
            System.out.print("\n");
        }
    }
}

Sudoku class:

package application;

import java.util.Random;
import java.util.Set;
import java.util.stream.Collectors;

import static application.SudokuGrid.*;
import static application.SudokuGrid.SudokuValue.*;

public class Sudoku {

    private Sudoku() {
        throw new AssertionError();
    }

    /**
     * Contains sudoku default(valid) grid values
     */
    private static final SudokuValue[][] grid = new SudokuValue[][] {
        {ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE},
        {FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, ONE, TWO, THREE},
        {SEVEN, EIGHT, NINE, ONE, TWO, THREE, FOUR, FIVE, SIX},

        {THREE, ONE, TWO, NINE, SEVEN, EIGHT, SIX, FOUR, FIVE},
        {SIX, FOUR, FIVE, THREE, ONE, TWO, NINE, SEVEN, EIGHT},
        {NINE, SEVEN, EIGHT, SIX, FOUR, FIVE, THREE, ONE, TWO},

        {TWO, THREE, ONE, EIGHT, NINE, SEVEN, FIVE, SIX, FOUR},
        {FIVE ,SIX ,FOUR ,TWO, THREE, ONE, EIGHT, NINE, SEVEN},
        {EIGHT, NINE, SEVEN, FIVE, SIX, FOUR, TWO, THREE, ONE}
    };

    /**
     * @param valueCount - number of digits with will be present in a grid
     * @return - SudokuGrid object with will contain random part of this grid
     * empty fields will be set to 0
     */
    public static SudokuGrid getSudokuGrid(int valueCount) {
        SudokuValue[][] grid = _getCopyOfGrid();
        _shuffleGrid(grid);
        _removeFields(grid, (ROWS * LINES) - valueCount);
        return new SudokuGrid(grid);
    }

    /**
     * do not pass objects that you don't want to be changed
     * pass filled grid otherwise it will remove empty(0) values
     * @param grid - sudoku grid from with specified number of fields
     * randomly chosen will be set to zero
     * @param count - number count with will be removed from grid
     */
    private static void _removeFields(SudokuValue[][] grid, int count) {
        Set<Integer> fields = new Random()
            .ints(0, LINES * ROWS)
            .distinct()
            .limit(count)
            .boxed()
            .collect(Collectors.toSet());

        fields.stream()
            .forEach( n -> {
                int line = n / LINES;
                int row = n % ROWS;
                grid[line][row] = EMPTY;
            });
    }

    /**
     * @return - copy of this objects grid
     */
    private static SudokuValue[][] _getCopyOfGrid() {
        SudokuValue[][] copyOfGrid = new SudokuValue[LINES][ROWS];
        for(int index = 0; index < LINES; index++) {
            System.arraycopy(grid[index], 0, copyOfGrid[index], 0, ROWS);
        }
        return copyOfGrid;
    }

    /**
     * Randomly shuffles grid swapping whole lines and whole rows in the same block
     */
    private static void _shuffleGrid(SudokuValue[][] grid) {
        for(int block, swapIndexOne, swapIndexTwo, index = 0; index < 1000; index++) {
            block = new Random().nextInt(BLOCK_SIZE) * BLOCK_SIZE;
            swapIndexOne = block + new Random().nextInt(BLOCK_SIZE);
            swapIndexTwo = block + new Random().nextInt(BLOCK_SIZE);
            _swapLines(grid, swapIndexOne, swapIndexTwo);
            _swapRows(grid, swapIndexOne, swapIndexTwo);
        }
    }

    /**
     * @param r1 - row one index from grid
     * @param r2 - row two index from grid
     * Swaps two rows with are in the same 3x3 block
     * throws RunTimeException if rows are not in the same block
     */
    private static void _swapRows(SudokuValue[][] grid, int r1, int r2) {
        if (_isSameBlock(r1, r2)) {
            for(int index = 0; index < ROWS; index++) {
                SudokuValue temp = grid[index][r1];
                grid[index][r1] = grid[index][r2];
                grid[index][r2] = temp;
            }
        } else {
            throw new RuntimeException("Rows are not in the same block");
        }
    }

    /**
     * @param l1 - is line one index from grid
     * @param l2 - us line two index from grid
     * Swaps two lines with are in the same 3x3 block
     * throws RunTimeException if lines are not in the same block
     */
    private static void _swapLines(SudokuValue[][] grid, int l1, int l2) {
        if (_isSameBlock(l1, l2)) {
            SudokuValue[] tempLine = new SudokuValue[ROWS];
            System.arraycopy(grid[l1], 0, tempLine, 0, ROWS);
            System.arraycopy(grid[l2], 0, grid[l1], 0, ROWS);
            System.arraycopy(tempLine, 0, grid[l2], 0, ROWS);
        } else {
            throw new RuntimeException("Lines are not in the same block");
        }
    }

    /**
     * @param block1 - is block one index from grid
     * @param block2 - us block two index from grid
     * @return - true if lines are in the same 3x3 block
     * false if lines are NOT in the same 3x3 block
     */
    private static boolean _isSameBlock(int block1, int block2) {
        return (block1/BLOCK_SIZE == block2/BLOCK_SIZE && block1/BLOCK_SIZE < BLOCK_SIZE);
    }
}
  1. What do you think about my implementation? Is there any better one?
  2. What about invalid fields methods that return set of Point objects? How can I create it better?
  3. What about structure of my classes? static class inside SudokuGrid and how I'm doing with comments?
  4. And in general what could be improved?

I'm using Java 8.

\$\endgroup\$
5
\$\begingroup\$

Parameterized Sudoku

In a standard Sudoku grid, there should be \$n^2\$ blocks of \$n^2\$ numbers, typically \$3^2 = 9\$ blocks of \$3^2 = 9\$ numbers, arranged in a square. As such, the LINES, ROWS, BLOCK_SIZE variables are a bit confusing, as they could all be derived from \$n = 3\$.

It would be nice if \$n\$ was a parameter of the Sudoku generator / validator, to work with standard sudoku grids of arbitrary sizes, instead of using those hardcoded static constants.

Class design

The class design and organization have many strange, unnatural elements. To name just the few most prominent ones:

  • SudokuGrid hardcodes the game parameters in static constants and an enum. This effectively prevents extending the game to other setups with \$n <> 3\$.

  • java.awt.Point shouldn't be used in this Sudoku model. java.awt.Point is a graphical element. To follow proper separation of MVC principles, objects from graphical libraries don't belong here. Just because you want to put x and y somewhere and there is an existing class, that doesn't make it suitable.

Coding style

There are many coding style issues. To highlight some of the most prominent:

  • Creating multiple instances of Random is both inefficient and unnecessary. You could create just one and reuse it
  • throw new AssertionError without a message parameter to explain itself
  • It's not a common convention to prefix method names with _. Stick to camelCase
  • Some for loops can be replaced with for-each
  • Some variables are poorly named. For example in printGrid, the variable names index and index2 are not good.
  • On a class named SudokuGrid, the term "grid" in printGrid is redundant
\$\endgroup\$
  • \$\begingroup\$ An enum is always static as there's no way for the singletons to refer an enclosing SudokuGrid instance. I guess you mean the OP should add the redundant modifier, but it sounds confusing. \$\endgroup\$ – maaartinus May 30 '15 at 0:55
  • \$\begingroup\$ Worth noting is that using an enum instead of an int nukes the parameterisation for bigger sudokus as well (see your first point) \$\endgroup\$ – AlexR May 30 '15 at 1:01
  • \$\begingroup\$ You're absolutely right @maaartinus, no, the OP shouldn't add a redundant modifier, I dropped that point from my post, thanks for pointing out. \$\endgroup\$ – janos May 30 '15 at 7:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.