4
\$\begingroup\$

I really hate copy and pasting my code from one method to another. It signals something in my brain, "This could be done either more efficiently or more readable". This is used in a minesweeper program I am implementing with a JavaFx GUI (that's not very relevant, but it might help to know). Hopefully this is enough context!

@Contract(pure = true)
private int setMinX(int x){ return (x == 0 ? 0:(x-1)); }
@Contract(pure = true)
private int setMinY(int y){ return (y == 0 ? 0:(y-1)); }
@Contract(pure = true)
private int setMaxX(int x){ return (x == this.width-1 ? this.width-1 : x+1); }
@Contract(pure = true)
private int setMaxY(int y){ return (y == this.height-1 ? this.height-1 : y+1);}

private void adjacents(int x, int y){
    //calculate adjacents for the just placed mine
    int minX = setMinX(x);
    int minY = setMinY(y);
    int maxX = setMaxX(x);
    int maxY = setMaxY(y);

    if (this.mineField[minX][minY] != -1){ this.mineField[minX][minY] += 1; } //left top corner
    if (this.mineField[maxX][minY] != -1){ this.mineField[maxX][minY] += 1; } //right top corner
    if (this.mineField[maxX][maxY] != -1){ this.mineField[maxX][maxY] += 1; } // bottom right corner
    if (this.mineField[minX][maxY] != -1){ this.mineField[minX][maxY] += 1; } // bottom left corner

    if(x > 0 && x < this.width-1){
        if (this.mineField[x][minY] != -1){ this.mineField[x][minY] += 1; } // top-middle
        if (this.mineField[x][maxY] != -1){ this.mineField[x][maxY] += 1; } //bottom-middle
    }

    if(y > 0 && y < this.height-1){
        if (this.mineField[minX][y] != -1){ this.mineField[minX][y] += 1; } // left-middle
        if (this.mineField[maxX][y] != -1){ this.mineField[maxX][y] += 1; } //right-middle
    }
}

public void floodClear(int x, int y) {

    int minX = setMinX(x);
    int minY = setMinY(y);
    int maxX = setMaxX(x);
    int maxY = setMaxY(y);

    if (this.mineField[x][y] >= 0 && !this.liveGame[x][y]) {
        //IMPLEMENT WITH GUI
        //show(x,y);
        if (this.mineField[minX][minY] == 0) {
            floodClear(minX, minY);
        }
        if (this.mineField[maxX][minY] == 0) {
            floodClear(maxX, minY);
        }
        if (this.mineField[maxX][maxY] == 0) {
            floodClear(maxX, maxY);
        }
        if (this.mineField[minX][maxY] == 0) {
            floodClear(minX, maxY);
        }

        if (x > 0 && x < this.width - 1) {
            if (this.mineField[x][minY] == 0) {
                floodClear(x, minY);
            } // top-middle
            if (this.mineField[x][maxY] == 0) {
                floodClear(x, maxY);
            } //bottom-middle
        }

        if (y > 0 && y < this.height - 1) {
            if (this.mineField[minX][y] == 0) {
                floodClear(minX, y);
            } // left-middle
            if (this.mineField[maxX][y] == 0) {
                floodClear(maxX, y);
            }  //right-middle
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Just out of curiousity, what are those @Contract annotations that you have added? \$\endgroup\$ – Simon Forsberg Jun 26 '16 at 18:32
  • \$\begingroup\$ Actually, this sums it up pretty well: "specifies if this method is pure, i.e. has no visible side effects. This may be used for more precise data flow analysis, and to check that the method's return value is actually used in the call place." \$\endgroup\$ – JaeJae55 Jun 26 '16 at 21:00
  • 1
    \$\begingroup\$ It's a bit weird to call the method setMinY when it seems more like a getter. \$\endgroup\$ – Simon Forsberg Jun 27 '16 at 7:52
  • \$\begingroup\$ You're right, unfortunately changed this moments after posting (: \$\endgroup\$ – JaeJae55 Jun 28 '16 at 20:24
2
\$\begingroup\$

Iterating through adjacents

There are two main approaches to take while iterating through the neighbors in Minesweeper.

The first and most common option is to use a nested for loop:

for (int y = -1; y <= 1; y++) {
    for (int x = -1; x <= 1; x++) {
        if (x == 0 && y == 0) {
            continue; // We don't want to consider the field itself as a neighbor
        }
        increaseAdjacentMineCounter(mineX + x, mineY + y);
    }
}

The second approach, and the one that I normally use and like better, is to use an array to define which fields should be used as your neighbors.

int[][] neighbors = {{ -1, -1 }, { -1, 0 }, { -1, 1 }, { 0, -1 },
    { 0, 1 }, { 1, -1 }, { 1, 0 }, { 1, 1 }};
for (int[] neighbor : neighbors) {
    int x = neighbor[0];
    int y = neighbor[1];
    increaseAdjacentMineCounter(mineX + x, mineY + y);
}

The reason for why I like the second approach better is because it becomes more data-oriented. You can change the way you consider neighbors without changing your code. Changing the way your neighbors are considered can be a bit entertaining but very confusing :)

\$\endgroup\$
1
\$\begingroup\$

this.mineField[minX][minY] != -1

This could be a method hasMineAt.

You could rewrite the entire method as a list of "increase adjacent mine counter":

private void increaseAdjacentMineCounter(int locationX, int locationY)
{
    if(withinBounds(locationX, locationY) && !hasMineAt(locationX, locationY)){
        this.mineField[locationX][locationY] += 1;
    }
}

Then your method could consist of 8 calls to increaseAdjacentMineCounter:

increaseAdjacentMineCounter(mineX - 1, mineY - 1);
increaseAdjacentMineCounter(mineX - 1, mineY);
increaseAdjacentMineCounter(mineX - 1, mineY + 1);

increaseAdjacentMineCounter(mineX, mineY - 1);

increaseAdjacentMineCounter(mineX, mineY + 1);

increaseAdjacentMineCounter(mineX + 1, mineY - 1);
increaseAdjacentMineCounter(mineX + 1, mineY);
increaseAdjacentMineCounter(mineX + 1, mineY + 1);

Maybe there's a way to make iterating that list a bit easier. I don't know.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.