Minesweeper clone using Swing

Question

I'm coding a Minesweeper clone using Java and Swing. For my current knowledge, I manage to keep it working but two things are giving me nightmares. Namely, I have a VERY repetitive code so I suppose there should be a way to write it once only and use twice but I don't know how to do it as the things it does when the conditions are met are highly different.

private void placeBombs(CellOnGrid[][] currentGrid)
{
    boolean wasTheBombPlaced = false;

    for(int i=_howManyMinesOnGrid; i>0; --i)
    {
        while(!wasTheBombPlaced)
        {
            int potentialY = (int) Math.rint(Math.random()*(getGridHeight()-1));
            int potentialX = (int) Math.rint(Math.random()*(getGridWidth()-1));
            if(!currentGrid[potentialY][potentialX].getBomb()) 
            {
                currentGrid[potentialY][potentialX].setBomb();
                wasTheBombPlaced = true;

                if(potentialY>0) {currentGrid[potentialY-1][potentialX].incrementHintNumber();}
                if(potentialY<(getGridHeight()-1)) {currentGrid[potentialY+1][potentialX].incrementHintNumber();}

                if(potentialX>0) {currentGrid[potentialY][potentialX-1].incrementHintNumber();}
                if(potentialX<(getGridWidth()-1)) {currentGrid[potentialY][potentialX+1].incrementHintNumber();}

                if((potentialY>0)&&(potentialX>0)) {currentGrid[potentialY-1][potentialX-1].incrementHintNumber();}
                if((potentialY>0)&&(potentialX<(getGridWidth()-1))) {currentGrid[potentialY-1][potentialX+1].incrementHintNumber();}

                if(potentialY<(getGridHeight()-1)&&(potentialX>0)) {currentGrid[potentialY+1][potentialX-1].incrementHintNumber();}
                if(potentialY<(getGridHeight()-1)&&(potentialX<(getGridWidth()-1))) {currentGrid[potentialY+1][potentialX+1].incrementHintNumber();}
            }
        }
        wasTheBombPlaced = false;
    }

Its purpose is to simply put the bombs randomly and for each bomb placed, check if it's far enough from an edge (or two edges) to increment the corresponding cell's hint number. Also, please note I number my X, Y coordinates from the upper-left corner so (0,0) is the first cell both from the top and from the left. This code, then, just asks 8 questions of the form: "Are you at least one line from the top?" and if it is, then it increments the hint number of the cell above the one we just placed our bomb on.

Problem is, I use the very same questions when user clicks something that is not a bomb. Then, the Action Listener invokes the revealEmpty recursive method which looks like this:

public void revealEmpty(int lineNumber, int columnNumber)                 
{
    --_cellsLeftToBeRevealed;
    buttonsArray[lineNumber][columnNumber].setEnabled(false);
    buttonsArray[lineNumber][columnNumber].showIcon();

    if(buttonsArray[lineNumber][columnNumber].getHintNumber()==0)
    {
        if(lineNumber>0) {
            if(isSuitableForRevealing(lineNumber-1, columnNumber)){
                    revealEmpty(lineNumber-1, columnNumber);}
        }
        if(lineNumber<(getGridHeight()-1)) {
            if(isSuitableForRevealing(lineNumber+1, columnNumber)){
                    revealEmpty(lineNumber+1, columnNumber);}
        }


        if(columnNumber>0) {
            if(isSuitableForRevealing(lineNumber, columnNumber-1)){
                    revealEmpty(lineNumber, columnNumber-1);}
        }
        if(columnNumber<(getGridWidth()-1)) {
            if(isSuitableForRevealing(lineNumber, columnNumber+1)){
                    revealEmpty(lineNumber, columnNumber+1);}
        }


        if((lineNumber>0)&&(columnNumber>0)) {
            if(isSuitableForRevealing(lineNumber-1, columnNumber-1)){
                    revealEmpty(lineNumber-1, columnNumber-1);}
        }
        if((lineNumber>0)&&(columnNumber<(getGridWidth()-1))) {
            if(isSuitableForRevealing(lineNumber-1, columnNumber+1)){
                    revealEmpty(lineNumber-1, columnNumber+1);}
        }


        if(lineNumber<(getGridHeight()-1)&&(columnNumber>0)) {
            if(isSuitableForRevealing(lineNumber+1, columnNumber-1)){
                    revealEmpty(lineNumber+1, columnNumber-1);}
        }
        if(lineNumber<(getGridHeight()-1)&&(columnNumber<(getGridWidth()-1))) {
            if(isSuitableForRevealing(lineNumber+1, columnNumber+1)){
                    revealEmpty(lineNumber+1, columnNumber+1);}
        }
    }

This one reveals what was under the clicked button and, asking the very same questions, goes deeper until there's nothing left to be revealed (the isSuitableForRevealing() just checks if a cell of given coordinates can still be revealed and showIcon() makes the object select an appropriate icon for itself to show while it's disabled).

How could I make this code better and not repeat myself twice with the same ifs?

Also, I know it might be confusing that in placeBombs() I get currentGrid as argument and in revealEmpty() I use some buttonsArray - please note that it's the same array of CellOnGrid[][] objects. I just didn't have the whole code neatly planned from the beginning so I wrote placeBombs() as it is - it will be refactored to not get any arguments as well since always there's only one grid of CellOnGrid objects in the Grid class at given moment so I guess I may just use it directly (it's the class's instance variable) instead of passing it as an argument.

Answer 1

You can increase the readability and reduce the number of checks required in your if block by performing each of the four booleans you get at the start, then checking against them instead of performing it over and over as well as separating out the check then reveal logic into its own method, like so:

private void checkAndReveal(int linePosition, int columnPosition)
{
    if(isSuitableForRevealing(linePosition, columnPosition))
    {
        revealEmpty(linePosition, columnPosition);
    }
}

Then in the method:

if(buttonsArray[lineNumber][columnNumber].getHintNumber() == 0)
{
    boolean notBottomRow = lineNumber < (getGridHeight() - 1);
    boolean notOuterColumn = columnNumber < (getGridWidth() - 1);
    boolean notFirstColumn = columnNumber > 0;
    boolean notTopRow = lineNumber > 0;

    if(notTopRow)
    {
        checkAndReveal(lineNumber - 1, columnNumber);
        if(notFirstColumn)
        {
            checkAndReveal(lineNumber - 1, columnNumber - 1);
        }
        if(notOuterColumn)
        {
            checkAndReveal(lineNumber - 1, columnNumber + 1);
        }
    }
    if(notBottomRow)
    {
        checkAndReveal(lineNumber + 1, columnNumber);
        if(notFirstColumn)
        {
            checkAndReveal(lineNumber + 1, columnNumber - 1);
        }
        if(notOuterColumn)
        {
            checkAndReveal(lineNumber + 1, columnNumber + 1);
        }
    }
    if(notFirstColumn)
    {
        checkAndReveal(lineNumber, columnNumber - 1);
    }
    if(notOuterColumn)
    {
        checkAndReveal(lineNumber, columnNumber + 1);
    }
}

The same can be done for your first piece of code, but I'll leave that up to you.

Answer 2

Did you thought about using a n+2 grid? Just don't place bombs at the border and don't display the other ring. So you can safely check and update all direction?

But maybe you should abstract your grid in two levels, to get rid of the +-1 stuff and implement a iterator to get all neighbours.

Update Getting iterateable neighbours is pretty easy when you introduce a NullObject for nonexisting cells. (This is also helpful at other places.)

public class Grid
{
    //...
    protected Cell getCell(int x, int y) {
        if (x < 1 || x > getWidth())
            return Cell.NULL;
        if (y < 1 || y > getHeight())
            return Cell.NULL;
        return cells[x][y];
    }

    protected boolean placeBomb(int x, int y) {
        if (!getCell(x, y).placeBomb())
            return false;

        for (Cell c : getNeighbours(x, y))
            c.incrementBombCount();
        return true;
    }

    public Set<Cell> getNeighbours(int x, int y) {
            Set<Cell> neighbours = new HashSet<>();
            neighbours.add(getCell(x - 1, y - 1));
            neighbours.add(getCell(x, y - 1));
            neighbours.add(getCell(x + 1, y - 1));
            neighbours.add(getCell(x - 1, y));
            neighbours.add(getCell(x + 1, y));
            neighbours.add(getCell(x - 1, y + 1));
            neighbours.add(getCell(x, y + 1));
            neighbours.add(getCell(x + 1, y + 1));
            neighbours.remove(Cell.NULL);
            return neighbours;
    }

}

And the cell class:

public class Cell {

    public static Cell NULL = new NullCell();

    public static class NullCell extends Cell {
        private NullCell() {
            super(null, -1, -1);
        }

        @Override
        public Set<Cell> getNeighbours() {
            return Collections.emptySet();
        }

        @Override
        public boolean placeBomb() {
            return false;
        }
            //...
    }

    private final Grid grid;
    private final int x;
    private final int y;
    private VisualState state = VisualState.HIDDEN;
    private boolean bomb = false;
    private int bombCount = 0;

    public Cell(Grid grid, int x, int y) {
        this.grid = grid;
        this.x = x;
        this.y = y;
    }

    public Set<Cell> getNeighbours() {
        return grid.getNeighbours(x, y);
    }

    //...
}