Minesweeper Code, Review and Advice

Question

I created a minesweeper code but i need help going over it. I would like if someone could go over it and point out anything (repeating code, code being called multiple time when no needed, etc).

package Minesweeper;

import javax.swing.*;
import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

@SuppressWarnings("serial")
public class BoardBuild extends JFrame implements ActionListener {
    static BoardBuild board = new BoardBuild();
    static JPanel p1, p2, p3;
    int miss = 0;

    // top label
    JLabel lbl1 = new JLabel("Minesweeper");

    public static final int BOARD_HEIGHT = 10;
    public static final int BOARD_WIDTH = 10;
    public static final int NUMBER_OF_CELLS = BOARD_HEIGHT * BOARD_WIDTH;
    public static final int BOMB_VALUE = 99; 
    public static final int NO_MINE = 0; 

    // all 100 of the board pieces to click
    JButton[][] btn = new JButton[BOARD_WIDTH][BOARD_HEIGHT];

    int mines[][] = new int[BOARD_WIDTH][BOARD_HEIGHT];// board piece values
    private GenerateMines mineGen;
    JButton btnReset = new JButton("Reset");

    public BoardBuild() {
        p1 = new JPanel();
        p1.add(lbl1, BorderLayout.CENTER);

        p2 = new JPanel();
        p2.setLayout(new GridLayout(BOARD_WIDTH, BOARD_HEIGHT));

        for (int x = 0; x < BOARD_HEIGHT; x++) {
            for (int y = 0; y < BOARD_WIDTH; y++) {
                btn[x][y] = new JButton("");
                btn[x][y].addActionListener(this);
                p2.add(btn[x][y]);
            }
        }
        getMines();
        p3 = new JPanel();
        btnReset.addActionListener(this);
        p3.add(btnReset, BorderLayout.CENTER);
    }

    public void getMines() {
        mineGen = new GenerateMines();
        System.out.println("New Mine Values Generated");

        // sets indicator according to how many bombs around
        for (int x = 0; x < BOARD_HEIGHT; x++) { 
            for (int y = 0; y < BOARD_WIDTH; y++) {
                btn[x][y].setText("");
                mines[x][y] = mineGen.getMinePos(x, y);
                if (mines[x][y] >= BOMB_VALUE) 
                    btn[x][y].setForeground(Color.RED);
                else {
                    btn[x][y].setBackground(null);
                    if (mines[x][y] == 1)
                        btn[x][y].setForeground(Color.BLUE);
                    else if (mines[x][y] == 2)
                        btn[x][y].setForeground(Color.GREEN);
                    else if (mines[x][y] == 3)
                        btn[x][y].setForeground(Color.YELLOW);
                    else if (mines[x][y] == 4)
                        btn[x][y].setForeground(Color.ORANGE);
                    else if (mines[x][y] == 5)
                        btn[x][y].setForeground(Color.RED);
                }
            }
        }
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        for (int x = 0; x < BOARD_HEIGHT; x++) {
            for (int y = 0; y < BOARD_WIDTH; y++) {
                if (e.getSource() == btn[x][y]) {
                    btn[x][y].removeActionListener(this);
                    if (mines[x][y] >= BOMB_VALUE) {
                        showBoard();
                        return;
                    } else if (mines[x][y] != NO_MINE) {
                        btn[x][y].setText("" + mines[x][y]);
                        btn[x][y].setBackground(Color.GRAY);
                        miss++;
                    } else if (mines[x][y] == NO_MINE)
                        floodFill(x, y);
                    checkWin();
                    System.out.println(miss);
                }
            }
        }
        if (e.getSource() == btnReset) {
            miss = 0;
            System.out.println(miss);
            getMines();
            for (int x = 0; x < BOARD_HEIGHT; x++) {
                for (int y = 0; y < BOARD_WIDTH; y++) {
                    btn[x][y].addActionListener(this);
                    btn[x][y].setBackground(null);
                }
            }
        }
    }

    public void floodFill(int x, int y) {
        if (x >= 0 && x <= 9 && y >= 0 && y <= 9) {
            for (int z = 1; z < 6; z++) {
                if (mines[x][y] == z) {
                    miss++;
                    System.out.println(miss);
                    btn[x][y].setText(z + "");
                    btn[x][y].setBackground(Color.GRAY);
                    return;
                }
            }
            if (mines[x][y] == 0 && btn[x][y].getBackground() != Color.GRAY) {
                miss++;
                System.out.println(miss);
                btn[x][y].setBackground(Color.GRAY);
                btn[x][y].removeActionListener(this);
                floodFill(x - 1, y);
                floodFill(x + 1, y);
                floodFill(x, y - 1);
                floodFill(x, y + 1);
            } else {
                return;
            }
        }
    }

    public void showBoard() {
        System.out.println("BOOOM");
        JOptionPane.showMessageDialog(this, "Unfortunately, You Lose", "",
                JOptionPane.PLAIN_MESSAGE);
        for (int x = 0; x < BOARD_HEIGHT; x++) {
            for (int y = 0; y < BOARD_WIDTH; y++) {
                btn[x][y].setBackground(Color.GRAY);
                if (mines[x][y] != NO_MINE) {
                    if (mines[x][y] >= BOMB_VALUE)
                        btn[x][y].setText("B");
                    else
                        btn[x][y].setText("" + mines[x][y]);
                }
            }
        }
        miss = 0;
    }

    public void checkWin() {
        if (miss == 90) {
            System.out.println("WIN");
            JOptionPane.showMessageDialog(this, "Congratulations, You Win!!",
                    "", JOptionPane.PLAIN_MESSAGE);
        }
    }

    public static void main(String[] args) {
        Dimension dimension = Toolkit.getDefaultToolkit().getScreenSize();
        int x = (int) ((dimension.getWidth() - 450) / 2);
        int y = (int) ((dimension.getHeight() - 500) / 2);

        board.setLocation(x, y);
        board.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        board.add(p1, BorderLayout.NORTH);
        board.add(p2, BorderLayout.CENTER);
        board.add(p3, BorderLayout.SOUTH);
        board.setSize(450, 500);
        board.setVisible(true);
        board.setResizable(true);
    }
}

This it the main class and does a lot of other things.

package Minesweeper;

public class GenerateMines {
    // all 100 of the board pieces values (0=no bomb near 1=1 bomb near, etc)
    int[][] board = new int[10][10];

    public GenerateMines() {
        int temp;
        int row, column;

        final int NO_MINE = 0;
        final int BOMB_VALUE = 99;
        final int NUM_OF_BLOCKS = 100;
        final int NUM_OF_MINES = 10;

        for (int j = 0; j < NUM_OF_MINES; j++) {
            do {
                temp = (int) (Math.random() * NUM_OF_BLOCKS);
                row = temp / 10;
                column = temp % 10;
            } while (board[row][column] == BOMB_VALUE);
            board[row][column] = BOMB_VALUE;
        }

        for (int x = 0; x < 10; x++) {
            for (int y = 0; y < 10; y++) {
                if (board[x][y] >= BOMB_VALUE) {
                    if (y != 9)// East
                        board[x][y + 1] += 1;
                    if (y != 0)// West
                        board[x][y - 1] += 1;
                    if (x != 0)// North
                        board[x - 1][y] = board[x - 1][y] + 1;
                    if (x != 9)// South
                        board[x + 1][y] = board[x + 1][y] + 1;
                    if (y != 9 && x != 0)// N.E.
                        board[x - 1][y + 1] += 1;
                    if (x != 0 && y != 0)// N.W.
                        board[x - 1][y - 1] += 1;
                    if (x != 9 && y != 0)// S.W.
                        board[x + 1][y - 1] += 1;
                    if (x != 9 && y != 9)// S.E.
                        board[x + 1][y + 1] += 1;
                }
            }
        }
    }

    public int getMinePos(int r, int c) {// sends value of each piece to main
        return (board[r][c]);
    }
}

This is the class which generates the mine positions and the values of the "blocks" around it.

I would appreciate if someone could look over this and give me areas in which i need to fix (Line, Advice on what to do there).

Answer 1

In Java, primitive numeric types all default to their equivalent of 0, therefore this block can be removed entirely from GenerateMines:

for (int x = 0; x < 10; x++) {
    for (int y = 0; y < 10; y++) {
        board[x][y] = 0;
    }
}

Additionally, this Boolean[][] filled = new Boolean[10][10]; is only ever filled as false, and then never read. Therefore these lines can be removed:

Boolean[][] filled = new Boolean[10][10];

and filled[x][y] = false;

And a call to btn[x][y] != null will produce the same result as checking filled[x][y] if you are using it somewhere outside the code you provided.

The actionPerformed() method in BoardBuild should have an @Override annotation as it implements the method ActionListener.actionPerformed().

Your large if block in getMines() that sets the colour of the JButtons: the value of mines[x][y] does not change inside the block, therefore only one can be true in one iteration, and should therefore be an if/else-if block, or a switch statement to avoid checking the condition after it has already found one that is true (in a non-worst case scenario).

The:

else {
    return;
}

block in BoardBuild.floodFill() is unnecessary, as it is the last line in the method and would therefore happen on its own when it gets to the subsequent brace anyway.

In the BoardBuild() constructor the call to getMines(); is possibly dangerous as the method getMines() is overridable in a way which would stop the class from constructing properly. The simplest solution is to make the getMines() method private (if you don't need to use it outside of the class) or final if you do.

Although they're more stylistic choices, I like to add this. when accessing member variables so you can tell the difference between them and local variables without syntax highlighting, and according to the Java naming conventions, variables should start with a lowercase letter, so BoardBuild Board should be BoardBuild board unless you're going to make it final when it'd be BoardBuild BOARD.

Additionally, these variables in GenerateMines:

int temp;
int row, column;

Are only used in the constructor, but stored indefinitely because you made them member variables. They should be local to the constructor. The same is true of GenerateMines mineGen; in BoardBuild which is only used in getMines(). Only make them member variables if necessary to save on memory.

Answer 2

The getMines method could probably be named more descriptively. A method that returns void with a name starting with "get" is misleading.

You have many "magic numbers" in your code, especially 10 and 100. This makes your code less clear, and harder to change. Consider instead creating constants for board height, board width, and number of cells:

public static final int BOARD_HEIGHT = 10;
public static final int BOARD_WIDTH = 10;
public static final int NUMBER_OF_CELLS = BOARD_HEIGHT * BOARD_WIDTH;

So a snippet of code like the below would change to

for (int x = 0; x < BOARD_WIDTH; x++) {
            for (int y = 0; y < BOARD_HEIGHT; y++) {
                btn[x][y].setBackground(Color.GRAY);
                if (mines[x][y] != 0) {

Or a declaration would change to

int board[][] = new int[BOARD_WIDTH][BOARD_HEIGHT]

I'm also not fond of representing the bombs as a magic number. You could create a cell class:

public class Cell { 
    private int x;
    private int y;
    private boolean isBomb;
    private int adjacentBombCount;
    //TODO: Constructors, setters, etc
}

Then your code would be much clearer:

if (cell.isBomb()){

On the other hand, if you want to leave it coded as int, you should at least define a constant for the bomb.

/*
 *  The value to set if the cell contains a bomb
 */
public static final int BOMB_VALUE = 99; 

Answer 3

The array mine is never used in class GenerateMines. Consider removing it.

Answer 4

mines[x][y] == 0

seams to mean 'no mine'

I would make a constant

final static int NO_MINES=0;

=>

mines[x][y] == NO_MINES

Answer 5

My addition from the algorithm perspective - usually, in minesweeper you should generate the mines after the first user click. Why? You just don't want the user to find a mine by the first click.