patternjavaMinor
Sudoku with Android
Viewed 0 times
withsudokuandroid
Problem
I have been programming for a long time, but nobody has given me any feedback if my code is good or not. Please review it, and give me feedback where I could be better.
The source code is also on GitHub.
GameEngine.java:
MainActivity.java
SudokuChecker.java
```
package com.marcellelek.sudoku;
public class SudokuChecker {
private static SudokuChecker instance;
private SudokuChecker(){}
public static SudokuChecker getInstance(){
if( i
The source code is also on GitHub.
GameEngine.java:
package com.marcellelek.sudoku;
import com.marcellelek.sudoku.view.sudokugrid.GameGrid;
import android.content.Context;
public class GameEngine {
private static GameEngine instance;
private GameGrid grid = null;
private int selectedPosX = -1, selectedPosY = -1;
private GameEngine(){}
public static GameEngine getInstance(){
if( instance == null ){
instance = new GameEngine();
}
return instance;
}
public void createGrid( Context context ){
int[][] Sudoku = SudokuGenerator.getInstance().generateGrid();
Sudoku = SudokuGenerator.getInstance().removeElements(Sudoku);
grid = new GameGrid(context);
grid.setGrid(Sudoku);
}
public GameGrid getGrid(){
return grid;
}
public void setSelectedPosition( int x , int y ){
selectedPosX = x;
selectedPosY = y;
}
public void setNumber( int number ){
if( selectedPosX != -1 && selectedPosY != -1 ){
grid.setItem(selectedPosX,selectedPosY,number);
}
grid.checkGame();
}
}MainActivity.java
package com.marcellelek.sudoku;
import android.app.Activity;
import android.os.Bundle;
public class MainActivity extends Activity {
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
GameEngine.getInstance().createGrid(this);
}
private void printSudoku(int Sudoku[][]) {
for (int y = 0; y < 9; y++) {
for (int x = 0; x < 9; x++) {
System.out.print(Sudoku[x][y] + "|");
}
System.out.println();
}
}SudokuChecker.java
```
package com.marcellelek.sudoku;
public class SudokuChecker {
private static SudokuChecker instance;
private SudokuChecker(){}
public static SudokuChecker getInstance(){
if( i
Solution
Singletons
You have made your
Using only a single instance is perfectly okay, but enforcing the use of only a single instance is bad.
I'd also recommend you to read an answer to So Singletons are bad, then what? at the Stack Exchange site Software Engineering.
Especially your
Validating the board
Your current algorithm for checking if there's a duplicate in a row or column or region is currently of time complexity \$O(n^2)\$. For each item you compare it with (almost) each other item. This is a bit slow.
Instead you can use a
Example rewrite of your
Stylistic stuff
Please lookup Google's Java Style Guide and follow it. Use the automatic formatting feature of your IDE. Name the parameters to your methods according to the conventions.
You have made your
GameEngine and SudokuChecker as singletons, meaning that you are preventing multiple instances from being created. Why? To me it feels like you just want the easy way out, so that you can anywhere write GameEngine.getInstance(). I would recommend you to use Dependency Injection instead and avoid using singletons when you don't absolutely have to. (Hint: You pretty much never have to).Using only a single instance is perfectly okay, but enforcing the use of only a single instance is bad.
I'd also recommend you to read an answer to So Singletons are bad, then what? at the Stack Exchange site Software Engineering.
Especially your
SudokuChecker and SudokuGenerator should definitely not be singletons. Just ask yourself: Would there be a problem with having two instances of SudokuGenerator? What if you wanted to generate 10 boards and then choose one that you liked best?Validating the board
Your current algorithm for checking if there's a duplicate in a row or column or region is currently of time complexity \$O(n^2)\$. For each item you compare it with (almost) each other item. This is a bit slow.
Instead you can use a
Set to keep track of the numbers you have seen.Example rewrite of your
checkHorizontal method:private boolean checkHorizontal(int[][] sudoku) {
for (int y = 0 ; y found = new HashSet();
for (int xPos = 0 ; xPos < 9 ; xPos++) {
int number = sudoku[xPos][y];
if (number == 0) {
return false;
}
if (!found.add(number)) {
// this will happen if the number was already added
return false;
}
}
}
return true;
}Stylistic stuff
Please lookup Google's Java Style Guide and follow it. Use the automatic formatting feature of your IDE. Name the parameters to your methods according to the conventions.
Code Snippets
private boolean checkHorizontal(int[][] sudoku) {
for (int y = 0 ; y < 9 ; y++) {
Set<Integer> found = new HashSet<Integer>();
for (int xPos = 0 ; xPos < 9 ; xPos++) {
int number = sudoku[xPos][y];
if (number == 0) {
return false;
}
if (!found.add(number)) {
// this will happen if the number was already added
return false;
}
}
}
return true;
}Context
StackExchange Code Review Q#124652, answer score: 6
Revisions (0)
No revisions yet.