# Minesweeper BombCounts

I have a method which checks all of its surrounding squares and returns the number of bombs around it. But it's really long code and ugly, so can it be shorten?

final int MINE =10

for (int x = 0; x < counts.length; x++) {
for (int y = 0; y < counts[0].length; y++) {
if (counts[x][y] != MINE) {
int Minesearch = 0;
if (x > 0 && y > 0 && counts[x-1][y-1] == MINE) {//up left
Minesearch++;
}
if (y > 0 && counts[x][y-1] == MINE) {//up
Minesearch++;
}
if (x < counts.length - 1 && y > 0 && counts[x+1][y-1] == MINE) {//up right
Minesearch++;
}
if (x > 0 && counts[x-1][y] == MINE) {//left
Minesearch++;
}
if (x < counts.length - 1 && counts[x+1][y] == MINE) {//right
Minesearch++;
}
if (x > 0 && y < counts[0].length - 1 && counts[x-1][y+1] == MINE) {//down left
Minesearch++;
}
if (y < counts[0].length - 1 && counts[x][y+1] == MINE) {//down
Minesearch++;
}
if (x < counts.length - 1 && y < counts[0].length - 1 && counts[x+1][y+1] == MINE) {//down right
Minesearch++;
}
counts[x][y] = Minesearch;
}
}
}
}

• What is MINE initialized to, or what is its value if it is a constant?
– bob
Commented Aug 28, 2020 at 16:57
• @bob final int MINE = 10; This is it. Commented Aug 29, 2020 at 14:34
• It might be helpful to include that info in the question.
– bob
Commented Aug 31, 2020 at 13:05
• @bob Thanks I'll add MINE initialized Commented Sep 1, 2020 at 16:49

It can be shortened and "beautified" by refactoring the bounds cyhecking and mine checking into internal method:

private boolean isWithinBounds(int x, int y) {
return x >= 0 && y >= 0 && x < width && y < height;
}

private boolean isMine(int x, int y) {
return field[x][y] == MINE;
}


Then the mine count becomes trivial (we can assume that the center of the 3x3 square does not have a mine, otherwise the player would have exploded and ended the game):

for (int x1 = x - 1; x1 <= x + 1; x1++) {
for (int y1 = y - 1; y1 <= y + 1; y1++) {
if (isWithinBounds(x1, y1) && isMine(x1, y1) {
mineCount++;
}
}
}


What we have done is breaking code into methods, each of which implement a small and well defined function. Because each method does exactly one thing, they become easier to understand, maintain and test.

You should check Java naming conventions. Variable names should be in camelCase, startingWithSmallLetter.

Variable and method names should describe the reason why the code exists. E.g. mineSearch is confusing as the variable does not search mines, it just keeps count of them. Thus mineCount is a better alternative.

Counts is also confusing as it contains a value named MINE which obviously is a marker for a cell containing mine but it also contains the surrounding mine counts. I did a minesweeper clone (or actually a minesweeper solver) once and I used an array containing Cell-objects. The Cell object provided methods for querying the status of the cell (stepped on, flagged, unknown) and the number of surrounding mines if it had been stepped on.

• Why isWithinBounds(-1, -1) should return true? It will make isMine fail. The condition should be x >= 0 && y >= 0 && x < mines.length && y < mines[0].length.
– Marc
Commented Aug 28, 2020 at 6:15
• Good point @Marc. I had the condition exactly backwards! Commented Aug 28, 2020 at 6:26
• Also, I would normally write x >= 0 && x < width && y >= 0 && y < height. The same variable together. However, that's a really minor point. Commented Aug 28, 2020 at 17:18
• Why x1/y1? Also, that does check the current field, too, if I'm not mistaken. Commented Aug 28, 2020 at 20:05
• @Bobby The assumption is that mine count is performed only after the user has stepped on the current field. If there is a mine, the game ends and a mine count is not needed. :) Commented Aug 31, 2020 at 5:10

While the Answer of @TorbenPutkonen is correct it is a procedural approach to the problem.

There is nothing wrong with procedural approaches as such, but since Java is a object oriented language we might look out for OO-approaches instead...

I would extract the neighbor check into an enum like this:

enum Direction {
NORTH{
boolean isBomb(inx x, int y, boolean[] field){
if(0 < x)
return BOMB == field(x-1, y);
else
return false;
}
},
NORTH_WEST{
boolean isBomb(inx x, int y, boolean[] field){
if(0 < x && 0 < y)
return BOMB == field(x-1, y-1);
else
return false;
}
},
SOUTH{
boolean isBomb(inx x, int y, boolean[] field){
if(field.length-1 > x)
return BOMB == field(x+1, y);
else
return false;
}
},
SOUTH_EAST{
boolean isBomb(inx x, int y, boolean[] field){
if(field.length-1 > x && field[0].length-1>y)
return BOMB == field(x+1, y+1);
else
return false;
}
}
// other directions following same pattern

abstract boolean isBomb(inx x, int y, boolean[] field);
}


The benefit is that this enum could live in its own file and has a very limited responsibility. That means it is easy to understand what is does, isn't it?

In your calculation method you can simply iterate over the enum constants like this:

for (int x = 0; x < counts.length; x++) {
for (int y = 0; y < counts[0].length; y++) {
int mineCount =0;
for(Direction direction : Direction.values()) {
if (direction.isBomb(x, y, counts) ) {
mineCount++;
}
}
}
}


As a next step I'd apply the "tell, don't ask" principle by changing the method signature:

abstract int getBombValueOf(inx x, int y, boolean[] field);


the implementation in the enum would change like this:

     int getBombValueOf(inx x, int y, boolean[] field){
if(0 < x && BOMB == field(x-1, y))
return 1;
else
return 0;
},


That could be simplified to the "elvis operator":

     int getBombValueOf(inx x, int y, boolean[] field){
return (0 < x && BOMB == field(x-1, y))
? 1
: 0;
},


and the usage would change to this:

for (int x = 0; x < counts.length; x++) {
for (int y = 0; y < counts[0].length; y++) {
int mineCount =0;
for(Direction direction : Direction.values()) {
mineCount +=
direction.getBombValueOf(x, y, counts) );
}
}
}


We could achief the same (exept moving the neighbor calculation to another file) by using a FunctionalInterface and a simple collection:

@FunctionalInterface
interface Direction{
int getBombValueOf(inx x, int y, boolean[] field);
}

private final Collection<Direction> directions = new HashSet<>();

// in constructor
directions.add(new Direction() { // old style anonymous inner class
int getBombValueOf(inx x, int y, boolean[] field){
return (0 < x && BOMB == field(x-1, y))
? 1
: 0;
}
};
directions.add((x, y, field)-> { // Java8 Lambda
return (0 < x && 0 < y &&BOMB == field(x-1, y-1))
? 1
: 0;
};
// more following same pattern

for (int x = 0; x < counts.length; x++) {
for (int y = 0; y < counts[0].length; y++) {
int mineCount =0;
for(Direction direction : directions) {
mineCount +=
direction.getBombValueOf(x, y, counts) );
}
}
}


Of cause we could make much more benefit from OO principles if the game field would not be an array of primitives but a Collection of Objects. But that might be stuff for another answer... ;o)

• Instead of copy-pasting the code in the enums, you could define an xy-offset for each direction and reuse the same code in each value. Personally I don't advocate for adding any functionality to enums. They're intended to be a tool for enumerating value sets and making them a container for code violates that intention and as such, always surprises the user. There are always better ways to implement the functionality. Commented Aug 31, 2020 at 5:16
• @TorbenPutkonen enums are full featured classes. Classes are used to provide different behavior. So I don't see any reason to use enums as dump constants only, especially if in turn they are used to introduce branching by use of a switch block. But I agree that the code in the enums could be cleaned a little more... And just vor the records: my second approach does what you suggest. Commented Sep 2, 2020 at 13:02