patternjavaMinor
Minesweeper implementation
Viewed 0 times
implementationminesweeperstackoverflow
Problem
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.
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 = 0 && matrix[i - 1][j] == '*' ? 1 : 0;
}
private int checkNorthEastForBomb(char[][] matrix, int i, int j) {
return j + 1 = 0 && matrix[i - 1][j + 1] == '*' ? 1 : 0;
}
private int checkEastForBomb(char[][] matrix, int i, int j) {
return j + 1 = 0 && i + 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;
}Solution
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:
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:
Try doing the opposite so you only compute stuff when needed.
-
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.
Code Snippets
if (isCellABomb(matrix, row-1, col)) count++;
if (isCellABomb(matrix, row-1, col+1)) count++; ....int count = checkAdjacentsForBombs(matrix, i, j);
if (matrix[i][j] != '*') matrix[i][j] = (char) (count + '0');Context
StackExchange Code Review Q#117114, answer score: 5
Revisions (0)
No revisions yet.