Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm coding a minesweeper clone using Java and Swing. For my current knowledge, I manage to keep it working but two things give 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. Without further ado: in my Grid class, when I prepare the grid, I have such code:

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, not like in a regular plane. This code, then, just asks 8 questions like: "Are you at least one line from the top?" and if the answer is yes, 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, recurrence method which goes 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 appriopriate 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.

Could I please ask for some invoice?

share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

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.

share|improve this answer
    
I see. So it seems there's no better easy way to somehow merge the if-ing in both methods, thanks a lot :) –  Straightfw Jan 19 '13 at 19:00
    
I updated my answer do show a better an easy way:) –  mnhg Jan 20 '13 at 8:01
add comment

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);
    }

    //...
}
share|improve this answer
    
Thank you. Wouldn't it mess with the GridLayout, though? I mean - wouldn't the layout reserve space even for the invisible buttons? Also, I'm afraid I'm on a waay too low level to implement some iterators to get the neighbours. –  Straightfw Jan 19 '13 at 18:19
    
Memory? I didn't read the requirement that you want to run it in an embedded system or old dos machine :) Seriously, in most cases readability has an higher value than a premature optimization. –  mnhg Jan 20 '13 at 7:42
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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