patternjavaMinor
Minesweeper Code
Viewed 0 times
codeminesweeperstackoverflow
Problem
I created some code for Minesweeper 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 times when not needed, etc).
This it the main class that also does a lot of other things:
```
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 = 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 ac
This it the main class that also does a lot of other things:
```
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 = 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 ac
Solution
In Java, primitive numeric types all default to their equivalent of
Additionally, this
and
And a call to
The
Your large
The:
block in
In the
Although they're more stylistic choices, I like to add
Additionally, these variables in GenerateMines:
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
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.Code Snippets
for (int x = 0; x < 10; x++) {
for (int y = 0; y < 10; y++) {
board[x][y] = 0;
}
}Boolean[][] filled = new Boolean[10][10];else {
return;
}int temp;
int row, column;Context
StackExchange Code Review Q#20017, answer score: 7
Revisions (0)
No revisions yet.