2
\$\begingroup\$

I have created a Sudoku generator in Java. Here I am using a basic solved sudoku as seed and by transposing it and shuffling its row and columns I get a new solved sudoku.

 //import java.util.Arrays;

import java.util.Random;

public class SudokuGenerator {
    private char[][] board = new char[9][9];
    private int[] randomizeSudoku = new int[9];
    private char[][] transposedSeed = new char[][]{{'8', '2', '7', '1', '5', '4', '3', '9', '6'},
            {'9', '6', '5', '3', '2', '7', '1', '4', '8'},
            {'3', '4', '1', '6', '8', '9', '7', '5', '2'},
            {'5', '9', '3', '4', '6', '8', '2', '7', '1'},
            {'4', '7', '2', '5', '1', '3', '6', '8', '9'},
            {'6', '1', '8', '9', '7', '2', '4', '3', '5'},
            {'7', '8', '6', '2', '3', '5', '9', '1', '4'},
            {'1', '5', '4', '7', '9', '6', '8', '2', '3'},
            {'2', '3', '9', '8', '4', '1', '5', '6', '7'},};
    private char[][] seed = new char[9][9];
    private Random random = new Random();

    public static void main(String[] args) {
        SudokuGenerator s = new SudokuGenerator();
        int n = 2;
        s.transpose();
        s.shuffle();
        s.seedChanger();
        while (n > 0) {
            System.out.println("\n\n------ New Board --------\n");
            s.transpose();
            s.shuffle();
            s.display();
            s.seedChanger();
            n--;
        }
    }

    private void transpose() {
        for (int i = 0; i < 9; i++) {
            for (int j = 0; j < 9; j++) {
                seed[j][i] = transposedSeed[i][j];
            }
        }
    }

    private void seedChanger() {
        for (int i = 0; i < 9; i++) {
            System.arraycopy(board[i], 0, transposedSeed[i], 0, board.length);
        }
    }

    private void randomSudokuGenerator() {
        for (int i = 0; i < randomizeSudoku.length; i++) {
            randomizeSudoku[i] = 9;
        }
        int i = 0;
        for (; i < randomizeSudoku.length; ++i) {
            int r = random.nextInt(2);
            for (int i1 = 0; i1 < i; ++i1) {
                int x = randomizeSudoku[i1];
                if (x == r) {
                    if (i < 3) {
                        r = random.nextInt(3);
                    } else if (i < 6) {
                        r = random.nextInt(3) + 3;
                    } else if (i < 9) {
                        r = random.nextInt(3) + 6;
                    }
                    i1 = -1;
                }
            }
            randomizeSudoku[i] = r;
        }
    }

    private void shuffle() {
        randomSudokuGenerator();
//        System.out.println(Arrays.toString(randomizeSudoku));
        for (int x = 0; x < 9; x++) {
            board[0][x] = seed[randomizeSudoku[0]][x];
            board[1][x] = seed[randomizeSudoku[1]][x];
            board[2][x] = seed[randomizeSudoku[2]][x];
            board[3][x] = seed[randomizeSudoku[3]][x];
            board[4][x] = seed[randomizeSudoku[4]][x];
            board[5][x] = seed[randomizeSudoku[5]][x];
            board[6][x] = seed[randomizeSudoku[6]][x];
            board[7][x] = seed[randomizeSudoku[7]][x];
            board[8][x] = seed[randomizeSudoku[8]][x];
        }
        for (int x = 0; x < 9; x++) {

            if (randomizeSudoku[0] == 0) swapping(board, x, 1, 0);
            if (randomizeSudoku[0] == 1) swapping(board, x, 2, 0);
            if (randomizeSudoku[0] == 0) swapping(board, x, 5, 4);
            if (randomizeSudoku[0] == 1) swapping(board, x, 5, 3);
            if (randomizeSudoku[0] == 2) swapping(board, x, 8, 6);
        }
    }

    private void swapping(char[][] a, int commonIndex, int first, int second) {
        char swap = a[commonIndex][first];
        a[commonIndex][first] = a[commonIndex][second];
        board[commonIndex][second] = swap;
    }

    private void display() {
        int i, j;
        for (i = 0; i <= 8; ++i) {
            if (i == 0) {
                System.out.print("\t\t\t_______________________________________\n\t row " + (i + 1) + "\t");
            } else {
                System.out.print("\t\t\t|---|---|---||---|---|---||---|---|---|\n\t row " + (i + 1) + "\t");
            }
            for (j = 0; j <= 8; ++j) {
                if (j == 3) {
                    System.out.print("|");
                }
                if (j == 6) {
                    System.out.print("|");
                }
                if (j == 8) {
                    System.out.println("| " + board[i][j] + " |");
                } else {
                    System.out.print("| " + board[i][j] + " ");
                }
            }
            if (i == 2) {
                System.out.println("\t\t\t|---|---|---||---|---|---||---|---|---|");
            }
            if (i == 5) {
                System.out.println("\t\t\t|---|---|---||---|---|---||---|---|---|");
            }
            if (i == 8) {
                System.out.println("\t\t\t---------------------------------------");
                System.out.println("\tcolumns   1   2   3    4   5   6    7   8   9  \n\n\n");
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Which version of Java are you using? \$\endgroup\$ – T145 May 18 at 18:32
  • \$\begingroup\$ Thanks for editing @200_success , I will try to do better next time. \$\endgroup\$ – Akshay soni May 18 at 19:02
  • \$\begingroup\$ @T145 using java 8, why what happened? \$\endgroup\$ – Akshay soni May 18 at 19:03
  • \$\begingroup\$ Nothing. This would run on earlier versions, but knowing the version you're on gives knowledge of what tools can be used. \$\endgroup\$ – T145 May 18 at 19:08
  • \$\begingroup\$ Oh OK, thanks. I glad to know it can be used on older versions, and yes maybe more tools of Java 8 can be used. But I have just started to learn Java, when I made this I just knew this much tools. 🙂 \$\endgroup\$ – Akshay soni May 18 at 19:13
1
\$\begingroup\$

For this review, I'm operating under the assumption that this was a class assignment / programming challenge and/or has portions copy-pasted / is made by more than one person. The main reason for this is b/c of the excessive inconsistencies w/ formatting. Which is a pretty good segway into:

Formatting

Since you're using Java, I'll briefly touch on OOD (Object Oriented Design). Even though this is a controlled program / script, it would be a good practice to split up your main function into a separate class. This will let you design your Sedoku solver as a separate object. This is pretty important since you use SudokuGenerator in your main function.

On to styling, your for loops have awkwardly inconsistent formatting. You bounce btwn. using ++i and i++ (I prefer prefix b/c it used to give a performance boost, though now it probably doesn't matter), declaring variables inside and outside the loops, and using i < 9 and i <= 8. The principle here is to just one form of logic. So let's start just lightly changing the program using these principles.

Main.java

    public static void main(String[] args) {
        SudokuGenerator gen = new SudokuGenerator();
        // initial transpose, shuffle & seedChange happens in a constructor
        for (int i = 2; i > 0; --i) {
            System.out.println("\n\n------ New Board --------\n");
            gen.transpose();
            gen.shuffle();
            gen.display();
            gen.seedChanger();
        }
        /*
        int n = 2;
        s.transpose();
        s.shuffle();
        s.seedChanger();
        while (n > 0) {
            System.out.println("\n\n------ New Board --------\n");
            s.transpose();
            s.shuffle();
            s.display();
            s.seedChanger();
            n--;
        }
        */
    }

As the comment here says, the initial stuff you did to prepare the generator is now handled in its default constructor. You never edit n in those calls, so just declaring and modifying it in a for loop over a while is best here.

After some modification

Main.java

package T145.sudokugen;

public class Main {

    static char[][] transposedSeed = new char[][] {
        {'8', '2', '7', '1', '5', '4', '3', '9', '6'},
        {'9', '6', '5', '3', '2', '7', '1', '4', '8'},
        {'3', '4', '1', '6', '8', '9', '7', '5', '2'},
        {'5', '9', '3', '4', '6', '8', '2', '7', '1'},
        {'4', '7', '2', '5', '1', '3', '6', '8', '9'},
        {'6', '1', '8', '9', '7', '2', '4', '3', '5'},
        {'7', '8', '6', '2', '3', '5', '9', '1', '4'},
        {'1', '5', '4', '7', '9', '6', '8', '2', '3'},
        {'2', '3', '9', '8', '4', '1', '5', '6', '7'}
    };

    public static void main(String[] args) {
        Sudoku game = new Sudoku(transposedSeed);
        // initial transpose, shuffle & seedChange happens in a constructor
        for (int i = 2; i >= 0; --i) {
            System.out.println("\n\n------ New Board --------\n");
            game.transpose();
            game.shuffle();
            game.printBoard();
            game.seedChanger();
        }
    }
}

Sudoku.java

package T145.sudokugen;

import java.util.Random;

public class Sudoku {

    private final char[][] transposedSeed;

    private char[][] board = new char[9][9];
    private int[] randomizeSudoku = new int[9];
    private char[][] seed = new char[9][9];
    private Random random = new Random();

    public Sudoku(char[][] transposedSeed) {
        this.transposedSeed = transposedSeed;

        transpose();
        shuffle();
        seedChanger();
    }

    public void transpose() {
        for (short i = 0; i < 9; ++i) {
            for (short j = 0; j < 9; ++j) {
                seed[j][i] = transposedSeed[i][j];
            }
        }
    }

    public void seedChanger() {
        for (short i = 0; i < 9; ++i) {
            System.arraycopy(board[i], 0, transposedSeed[i], 0, board.length);
        }
    }

    public void randomSudokuGenerator() {
        short i = 0;

        for (i = 0; i < randomizeSudoku.length; ++i) {
            randomizeSudoku[i] = 9;
        }

        for (i = 0; i < randomizeSudoku.length; ++i) {
            int r = random.nextInt(2);

            for (int i1 = 0; i1 < i; ++i1) {
                int x = randomizeSudoku[i1];

                if (x == r) {
                    if (i < 3) {
                        r = random.nextInt(3);
                    } else if (i < 6) {
                        r = random.nextInt(3) + 3;
                    } else if (i < 9) {
                        r = random.nextInt(3) + 6;
                    }

                    i1 = -1;
                }
            }

            randomizeSudoku[i] = r;
        }
    }

    private void swap(char[][] a, int commonIndex, int first, int second) {
        char swap = a[commonIndex][first];
        a[commonIndex][first] = a[commonIndex][second];
        board[commonIndex][second] = swap;
    }

    public void shuffle() {
        randomSudokuGenerator();

        for (short x = 0; x < 9; ++x) {
            board[0][x] = seed[randomizeSudoku[0]][x];
            board[1][x] = seed[randomizeSudoku[1]][x];
            board[2][x] = seed[randomizeSudoku[2]][x];
            board[3][x] = seed[randomizeSudoku[3]][x];
            board[4][x] = seed[randomizeSudoku[4]][x];
            board[5][x] = seed[randomizeSudoku[5]][x];
            board[6][x] = seed[randomizeSudoku[6]][x];
            board[7][x] = seed[randomizeSudoku[7]][x];
            board[8][x] = seed[randomizeSudoku[8]][x];
        }

        for (short x = 0; x < 9; ++x) {
            if (randomizeSudoku[0] == 0) {
                swap(board, x, 1, 0);
            }

            if (randomizeSudoku[0] == 1) {
                swap(board, x, 2, 0);
            }

            if (randomizeSudoku[0] == 0) {
                swap(board, x, 5, 4);
            }

            if (randomizeSudoku[0] == 1) {
                swap(board, x, 5, 3);
            }

            if (randomizeSudoku[0] == 2) {
                swap(board, x, 8, 6);
            }
        }
    }

    public void printBoard() {
        for (short i = 0; i < 9; ++i) {

            if (i == 0) {
                System.out.print("\t\t\t_______________________________________\n\t row " + (i + 1) + "\t");
            } else {
                System.out.print("\t\t\t|---|---|---||---|---|---||---|---|---|\n\t row " + (i + 1) + "\t");
            }

            for (short j = 0; j < 9; ++j) {
                if (j == 3) {
                    System.out.print("|");
                }

                if (j == 6) {
                    System.out.print("|");
                }

                if (j == 8) {
                    System.out.println("| " + board[i][j] + " |");
                } else {
                    System.out.print("| " + board[i][j] + " ");
                }
            }

            if (i == 2) {
                System.out.println("\t\t\t|---|---|---||---|---|---||---|---|---|");
            }

            if (i == 5) {
                System.out.println("\t\t\t|---|---|---||---|---|---||---|---|---|");
            }

            if (i == 8) {
                System.out.println("\t\t\t---------------------------------------");
                System.out.println("\tcolumns   1   2   3    4   5   6    7   8   9  \n\n\n");
            }
        }
    }
}

A majority of this is just simple re-formatting. You'll notice I use short over int in some cases, and this is just simply b/c shorts have less of a memory imprint. It doesn't really matter b/c of just how much memory modern computers have, but I like to produce code that pays attention to the details.

I haven't modified any of your core logic b/c of my initial reasoning. But to point you in the right direction, think about:

1) Where the modulo operator (%) can be used to clean up some operations.

2) Where you can use other data structures to help optimize performance.

3) Where you can use Java 8+ abilities to optimize performance or at least slim down your code w/ equivalent performance.

\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing my code sir @T145 I have created this code on my own alone so I take full responsibility that I haven't given a structure to it and didn't use OOPS, I will try to make one code using all this concept because when I made this I didn't knew much about OOPS \$\endgroup\$ – Akshay soni May 19 at 2:09
  • \$\begingroup\$ Thanks for your valuable time and support @T145 \$\endgroup\$ – Akshay soni May 19 at 2:09
  • \$\begingroup\$ sir @T145 i further optimized my code a bit and did some of the changes you recommended . \$\endgroup\$ – Akshay soni May 19 at 6:13
  • \$\begingroup\$ s/segway/segue :D \$\endgroup\$ – rossum May 20 at 13:32
0
\$\begingroup\$

As recommended in answers , i updated my code a bit and further optimized it and tried to manage the tasks of methods. Below are some changes i made ;

Here , i put the initial transposing and shuffling into the constructor ,

private SudokuGenerator() {
    this.transpose();
    this.shuffle();
    this.seedChanger();
}

Next i created a method generate() that generates the new sudoku ,

private void generate() {
        System.out.println("\n\n------ New Board --------\n");
        for (int i = 0; i < random.nextInt(5); i++) {
            this.transpose();
            this.shuffle();
            this.seedChanger();
        }
        this.display();
    }

I optimized one of my method where i changed similar task for all index of array by using for loop ,

old code :

 for (short x = 0; x < 9; ++x) {
            board[0][x] = seed[randomizeSudoku[0]][x];
            board[1][x] = seed[randomizeSudoku[1]][x];
            board[2][x] = seed[randomizeSudoku[2]][x];
            board[3][x] = seed[randomizeSudoku[3]][x];
            board[4][x] = seed[randomizeSudoku[4]][x];
            board[5][x] = seed[randomizeSudoku[5]][x];
            board[6][x] = seed[randomizeSudoku[6]][x];
            board[7][x] = seed[randomizeSudoku[7]][x];
            board[8][x] = seed[randomizeSudoku[8]][x];
        }

optimized code :

for (int x = 0; x < 9; x++) {
            for (int i = 0; i < 9; i++) {
                board[i][x] = seed[randomizeSudoku[i]][x];
            }
        }

And converted all <=8 into < 9.

void main now :

 public static void main(String[] args) {
        SudokuGenerator s = new SudokuGenerator();
        s.generate();
    }

Is this better now ? @T145

\$\endgroup\$
  • \$\begingroup\$ Try and execute the code as you have it in this post. \$\endgroup\$ – T145 May 20 at 4:46
  • \$\begingroup\$ you mean my updated code right....? I tried this it works fine. It gives me one new random Sudoku everytime , but I am still creating the object in the psvm of class SudokuGenerator . I know if I make my constructor public I can make an anonymous object in any class and call the method generate() and I can get a random solved Sudoku. @T145 \$\endgroup\$ – Akshay soni May 20 at 5:42
  • \$\begingroup\$ If you were to divide your code into separate classes as I have then this code wouldn't execute. You have your SudokuGenerator's constructor as private, and therefore be inaccessible. Also, your transposedSeed shouldn't always be constant in theory, which is another reason to go for an OOD approach. Don't forget to select an answer. \$\endgroup\$ – T145 May 20 at 13:45
  • \$\begingroup\$ as I said earlier I know I should make my constructor public, and yes making transposed seed not constant is what I am going to think about, thanks. \$\endgroup\$ – Akshay soni May 20 at 14:07

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.