HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Sudoku with Android

Submitted by: @import:stackexchange-codereview··
0
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:

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 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.