Minesweeper implementation

Question

Please review my implementation of the Minesweeper game Kata. Any and all feedback will be much appreciated. I would love to hear your feedback on readability, simplicity, performance, and any code smells. I even welcome nitpicking. Overall I want to know is the code clean (I know there is always room for improvement).

The rules of the application can be found here: Minesweeper. I have modified it a bit I'm not reading from a file.

public class MineSweeper {
    public char[][] getSweptMatrix(char[][] matrix) {
        if(matrix == null) throw new IllegalArgumentException();
        buildSweepMatrix(matrix);
        return matrix;
    }

    private void buildSweepMatrix(char[][] matrix) {
        for (int i = 0; i < matrix.length; i++) {
            for (int j = 0; j < matrix[0].length; j++) {
                int count = checkAdjacentsForBombs(matrix, i, j);
                if (matrix[i][j] != '*') matrix[i][j] = (char) (count + '0');
            }
        }
    }

    private int checkAdjacentsForBombs(char[][] matrix, int i, int j) {
        int count = 0;
        count += checkNorthForBomb(matrix, i, j);
        count += checkNorthEastForBomb(matrix, i, j);
        count += checkEastForBomb(matrix, i, j);
        count += checkSouthEastForBomb(matrix, i, j);
        count += checkSouthForBomb(matrix, i, j);
        count += checkSouthWestForBomb(matrix, i, j);
        count += checkWestForBomb(matrix, i, j);
        count += checkNorthWestForBomb(matrix, i, j);
        return count;
    }

    private int checkNorthForBomb(char[][] matrix, int i, int j) {
        return i - 1 >= 0 && matrix[i - 1][j] == '*' ? 1 : 0;
    }

    private int checkNorthEastForBomb(char[][] matrix, int i, int j) {
        return j + 1 < matrix[0].length && i - 1 >= 0 && matrix[i - 1][j + 1] == '*' ? 1 : 0;
    }

    private int checkEastForBomb(char[][] matrix, int i, int j) {
        return j + 1 < matrix[0].length && matrix[i][j + 1] == '*' ? 1 : 0;
    }

    private int checkSouthEastForBomb(char[][] matrix, int i, int j) {
        return j + 1 < matrix[0].length && i + 1 < matrix.length && matrix[i + 1][j + 1] == '*' ? 1 : 0;
    }

    private int checkSouthForBomb(char[][] matrix, int i, int j) {
        return i + 1 < matrix.length && matrix[i + 1][j] == '*' ? 1 : 0;
    }

    private int checkSouthWestForBomb(char[][] matrix, int i, int j) {
        return j - 1 >= 0 && i + 1 < matrix.length && matrix[i + 1][j - 1] == '*' ? 1 : 0;
    }

    private int checkWestForBomb(char[][] matrix, int i, int j) {
        return j - 1 >= 0 && matrix[i][j - 1] == '*' ? 1 : 0;
    }

    private int checkNorthWestForBomb(char[][] matrix, int i, int j) {
        return j - 1 >= 0 && i - 1 >= 0 && matrix[i - 1][j - 1] == '*' ? 1 : 0;
    }

Diego Martinoia · Accepted Answer · 2016-01-19 14:00:08Z

It's mostly minor issues, but since you want nitpicking... here we go:

In your public method you check the sanity of the input for null. Assuming you want to check for sanity, then you should check also that none of the sub-arrays is null, and that they all have the same size. But that becomes a bit tedious to do in a "constructor", so you'd better extract it into a "checkArgument" method.
Stop using i and j with the matrix. Row and Col force you to reason more about their meaning, and will prevent you from silly mistakes where you switch them (though this is my personal preference)
Your method is called "int checkAdjacentsForBombs", but if it's an int it would be better named "int countAdjacentsBombs"
While I see their convenience, I don't like your "checkNorth"... style of returning ints. I'd rather have it being "isCellABomb(char[][], int row, int col) and then you can generalize all methods in a single one and do:
```
if (isCellABomb(matrix, row-1, col)) count++;

if (isCellABomb(matrix, row-1, col+1)) count++; ....
```

This will allow you to reuse it also in the buildSweepMatrix method check

You pass the matrix around everywhere, bc it's in the signature of your methods. But those are all private, so you may ease it up a bit by storing the matrix in the class and sharing it, since no one else will access those methods, though it's debatable.
You should really extract your bomb marker ('*') to a final static const BOMB_SYMBOL, to ease readability and maintainability (so if tomorrow you mark bombs by % instead, you need to change in a single place

You first do the "costly" operation of counting the bombs, and then checking if it was needed here:

int count = checkAdjacentsForBombs(matrix, i, j);

if (matrix[i][j] != '*') matrix[i][j] = (char) (count + '0');

Try doing the opposite so you only compute stuff when needed.

Great feedback absolutely appreciate it. Can't believe I missed points 2 and 6. — Marquis Blount
– Marquis Blount, Commented Jan 19, 2016 at 0:45

Simon Forsberg · Accepted Answer · 2016-01-18 19:44:39Z

I just want to comment about this:

count += checkNorthForBomb(matrix, i, j);
count += checkNorthEastForBomb(matrix, i, j);
count += checkEastForBomb(matrix, i, j);
count += checkSouthEastForBomb(matrix, i, j);
count += checkSouthForBomb(matrix, i, j);
count += checkSouthWestForBomb(matrix, i, j);
count += checkWestForBomb(matrix, i, j);
count += checkNorthWestForBomb(matrix, i, j);

You're really just doing the same thing there, but for different x/y coordinates. (i and j might be the standard for row/column in Mathematical matrices, but I always prefer x and y, especially in code).

What you should do is to make use of the pattern.

for (int y = i - 1; y <= i + 1; y++) {
    for (int x = j - 1; x <= j + 1; x++) {
        if (!inRange(x, y)) { // Make sure that the position is inside the Matrix
            continue;
        }
        if (x == 0 && y == 0) { // Skip the center
            continue;
        }
        count += isBomb(matrix[y][x]) ? 1 : 0;
    }
}

Generally when using 2D grids, I find a inRange method to be extremly useful.

In Minesweeper, I prefer a isBomb method rather than a matrix[y][x] == BOMB_SYMBOL check, as checking if it is a bomb is a common thing to do, and if you want to change matrix[y][x] to be a matrix of MinesweeperField, then it's much easier to perform that change (although then you should make isBomb a method in MinesweeperField)

Stack Exchange Network

Minesweeper implementation

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Minesweeper implementation

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions