Cfrtjryk

I wrote a basic Minesweeper game for practice. It consist 3 classes:

• Main - only as launcher


• Cell - controling behavior of single cell


• Board - coordinating behavior of cells.

The hardest part for me, was a determining the value of single cells (how many mines is around it), and firing a chain reaction, if some cell was empty(value = 0) - it check closest cells, if they have value, it just reveal it, if they are empty it repeats process on them. So

I wonder if my solution is reasonable (it works, but it look scary), and I would be great, if someone would review it.

These are two methods in Board Class: setCellValues() and scanForEmptyCells(). However I will be gratefull for any commants and advices, also concenring OOP, code structure and style.

I post basic working GUI version, if someone would like to run it.

Main Class:

public class Main {

    public static void main(String args){

        Board board = new Board();

        board.setBoard();

    }

}

Board Class:

import javax.swing.*;

import java.awt.*;

import java.util.ArrayList;



public class Board {

    private Cell cells;

    private int cellID = 0;

    private int side = 8;

    private int limit = side-2;



    public void setBoard(){

        JFrame frame = new JFrame();

        frame.add(addCells());



        plantMines();

        setCellValues();



        frame.pack();

        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

        frame.setVisible(true);

    }



    public JPanel addCells(){

        JPanel panel = new JPanel(new GridLayout(side,side));

        cells = new Cell[side][side];

        for(int i = 0; i< side; i++){

            for(int j = 0; j<side; j++){

                cells[i][j] = new Cell(this);

                cells[i][j].setId(getID());

                panel.add(cells[i][j].getButton());

            }

        }

        return panel;

    }



    public void plantMines(){

        ArrayList<Integer> loc = generateMinesLocation(10);

        for(int i : loc){

            getCell(i).setValue(-1);

        }

    }

    /*Choose rendom places for mines*/

    public ArrayList<Integer> generateMinesLocation(int q){

        ArrayList<Integer> loc = new ArrayList<Integer>();

        int random;

        for(int i = 0; i<q;){

            random = (int)(Math.random()* (side*side));

            if(!loc.contains(random)){

                loc.add(random);

                i++;

            }

        }

        return loc;

    }

    // MOST IMPORTANT PART/////////////////////////////////////////////////////

    /*This method count number of mines around particular cell and set its value*/

    public void setCellValues(){

        for(int i = 0; i<side; i++){

            for(int j = 0; j<side; j++){

                 if(cells[i][j].getValue() != -1){

                     if(j>=1 && cells[i][j-1].getValue() == -1) cells[i][j].incrementValue();

                     if(j<= limit && cells[i][j+1].getValue() == -1) cells[i][j].incrementValue();

                     if(i>=1 && cells[i-1][j].getValue() == -1) cells[i][j].incrementValue();

                     if(i<= limit && cells[i+1][j].getValue() == -1) cells[i][j].incrementValue();

                     if(i>=1 && j>= 1 && cells[i-1][j-1].getValue() == -1) cells[i][j].incrementValue();

                     if(i<= limit && j<= limit && cells[i+1][j+1].getValue() == -1) cells[i][j].incrementValue();

                     if(i>=1 && j<= limit && cells[i-1][j+1].getValue() == -1) cells[i][j].incrementValue();

                     if(i<= limit && j>= 1 && cells[i+1][j-1].getValue() == -1) cells[i][j].incrementValue();

                 }

            }

        }

    }

    /*This method starts chain reaction. When user click on particular cell, if cell is empty (value = 0) this

    method look for other empty cells next to activated one. If finds one, it call checkCell and in effect,

    start next scan on its closest area.

     */

    public void scanForEmptyCells(){

        for(int i = 0; i<side; i++){

            for(int j = 0; j<side; j++){

                if(!cells[i][j].isNotChecked()){

                    if(j>=1 && cells[i][j-1].isEmpty()) cells[i][j-1].checkCell();

                    if(j<= limit && cells[i][j+1].isEmpty()) cells[i][j+1].checkCell();

                    if(i>=1 && cells[i-1][j].isEmpty()) cells[i-1][j].checkCell();

                    if(i<= limit && cells[i+1][j].isEmpty()) cells[i+1][j].checkCell();

                    if(i>=1 && j>= 1 && cells[i-1][j-1].isEmpty()) cells[i-1][j-1].checkCell();

                    if(i<= limit && j<= limit && cells[i+1][j+1].isEmpty()) cells[i+1][j+1].checkCell();

                    if(i>=1 && j<= limit && cells[i-1][j+1].isEmpty()) cells[i-1][j+1].checkCell();

                    if(i<= limit && j>= 1 && cells[i+1][j-1].isEmpty()) cells[i+1][j-1].checkCell();

                }

            }

        }

    }

    //////////////////////////////////////////////////////////////////////////////////

    public int getID(){

        int id = cellID;

        cellID++;

        return id;

    }



    public Cell getCell(int id){

        for(Cell a : cells){

            for(Cell b : a){

                if(b.getId() == id) return b;



            }

        }

        return null;

    }



    public void fail(){

        for(Cell a : cells){

            for(Cell b : a){

                b.reveal();

            }

        }

    }

}

Cell Clas:

import javax.swing.*;

import java.awt.*;

import java.awt.event.ActionEvent;

import java.awt.event.ActionListener;



public class Cell implements ActionListener{

    private JButton button;

    private Board board;

    private int value;

    private int id;

    private boolean notChecked;



    public Cell(Board board){

        button = new JButton();

        button.addActionListener(this);

        button.setPreferredSize(new Dimension(20,20));

        button.setMargin(new Insets(0,0,0,0));

        this.board = board;

        notChecked = true;

    }



    public JButton getButton() {

        return button;

    }



    public int getValue() {

        return value;

    }



    public int getId() {

        return id;

    }



    public void setId(int id) {

        this.id = id;

    }



    public void setValue(int value) {

        this.value = value;

    }



    public void displayValue(){

        if(value==-1){

            button.setText("u2600");

            button.setBackground(Color.RED);

        }else if(value!=0){

            button.setText(String.valueOf(value));

        }

    }



    public void checkCell(){

        button.setEnabled(false);

        displayValue();

        notChecked = false;

        if(value == 0) board.scanForEmptyCells();

        if(value == -1) board.fail();

    }



    public void incrementValue(){

        value++;

    }



    public boolean isNotChecked(){

        return notChecked;

    }



    public boolean isEmpty(){

        return isNotChecked() && value==0;

    }



    public void reveal(){

        displayValue();

        button.setEnabled(false);

    }



    @Override

    public void actionPerformed(ActionEvent e) {

        checkCell();

    }



}

Some simple pointers to get things started...

Magic numbers

You have hard-coded your side, limit (how is it used, and why does it default to side - 2?), the number of mines, the dimensions of your buttons and the meaning of a Cell's value to -1 when it is a mine... these should either be extracted out as constants, or documented clearly to show how they are being used.

GUI and usability (UX)

Lumping four UX-related suggestions together:

1. Your game is missing the fun of minesweeper: a timer and the ability to flag mines!


2. Also, it will be nice if you had at least a reset button, if not a button to change the number of mines. :)


3. Oh yeah, your expansion logic doesn't seem to expand to also include the numbered cells... It only expands the empty ones, leaving the user to have to manually click on the surrounding numbered cells. This diminishes the playing quality somewhat.


4. Following from the earlier section, your Dimensions(20, 20) can be a bit small for high-DPI displays, it will be nice if you either went with higher values, or look for non-pixel-based scaling alternatives... Here's a related StackOverflow link I found that may be of interest:

Side-note: The older Minesweeper for Windows avoids triggering a mine on the first click, it'll be... nice(?) if you can replicate that too. ;)

Other minor code stuff

• Variable declarations should be done with the interface, i.e. List<Integer> loc = ... instead of ArrayList<Integer> loc = ...
  


• 
  Board.getID() can just be return cellID++, since that is a post-increment operator. That means (roughly) after the variable is called, the value before the increment is returned to the caller, but the value after the increment is written to the variable for the next usage. It is not that hard to understand as long as you realize the difference between pre- and post-increment operators.


• This could be just my personal preference, but I'm not fond of the idea of a somewhat complicated ActionListener implementation that knows a Board and also generates JButtons... I have a feeling you might be able to have a generic listener that acts on a pair of (x,y) button coordinates.


• And that nest of if statements... looks ripe for refactoring, but maybe I'll take a more in-depth look the next time.

Code Review: Part Deux (fortified with a bit of Java 8 magic features)

Starting your board

If you can rename setBoard() as getBoard() and change its return type as a JFrame, the following approach is what I believe the more conventional form of starting a Swing application:

public static void main(String args) {

    SwingUtilities.invokeLater(() -> new Board().getBoard());

}

Separating the creation of GUI elements and Cells

addCells() is doing both the creating of GUI elements and the initialization of your side * side Cell-array. It will be better if (the renamed) getBoard(), which calls addCells(), is only concerned with creating the GUI layer, and that instantiating your Board class actually initializes the Cells and the mines for us. This means that any Board instances are properly initialized and ready to be played, which is arguably more 'object'-like. Applying these points will result in the following code:

public Board() {

    cells = new Cell[SIDE][SIDE];

    IntStream.range(0, SIDE).forEach(i -> {

        IntStream.range(0, SIDE).forEach(j -> cells[i][j] = new Cell(this));

    });

    init();

}



private void init() {

    plantMines();

    setCellValues();

}

• I have taken the liberty of turning side into a static final SIDE field.


• Will explain init() later...

Iterating through the cells array

Based on my current understanding, a simpler way of iterating through our 2D array is to create a helper method that accepts a Consumer<Cell> operation in Java 8.

private void forEach(Consumer<Cell> consumer) {

    Stream.of(cells).forEach(row -> Stream.of(row).forEach(consumer));

}

This allows us to further simplify most of our code afterwards. The first method to be simplified is our addCells() method, provided the separation of concerns described in the earlier section is done.

private JPanel addCells() {

    JPanel panel = new JPanel(new GridLayout(SIDE, SIDE));

    forEach(cell -> panel.add(cell.getButton()));

    return panel;

}

Cell implementation and coupling

As mentioned in my original answer, I am not fond of Cell implementing the ActionListener interface, and I could be biased. My take on this is to give each JButton generated by the Cell a listener that calls its checkCell() method instead.

Cell(Board board) {

    this.board = board;

    button = new JButton();

    button.addActionListener(listener -> { this.checkCell(); });

    ...

}

As pointed out by @Simon, it'll be better have methods that directly represents the exact actions we want on a Cell, i.e. to setMine(), or check isMine(), or the more straightforward isChecked() as opposed to isNotChecked(). Here they are...

int setMine() {

    if (!isMine()) {

        setValue(-1);

        return 1;

    }

    return 0;

}



void checkCell() {

    reveal(null);

    if (isMine() || board.isDone()) {

        board.reveal(isMine() ? Color.RED : Color.GREEN);

    } else if (value == 0) {

        board.scanForEmptyCells();

    }

}



boolean isChecked() {

    return checked;

}



boolean isEmpty() {

    return !isChecked() && value == 0;

}



boolean isMine() {

    return value == -1;

}

The other observation I had is that your Cell class seems to be quite coupled with the Board class, and this is evident from the number of non-private methods in them. A quick test shows:

• All methods in Cell are accessed by Board except
  
  
  
  
  • setValue(int)
  
  
  • displayValue(Color)
  
  
  
  


• The following methods in Board are accessed by Cell
  
  
  
  
  • scanForEmptyCells()
  
  
  • reveal(Color)
  
  
  • isDone()
  
  
  
  

These suggests that there is a bit of 'to-and-fro-ing' between Board and Cell methods, which can be improved upon.

Game over?

A slight enhancement shown above is the usage of Color to choose how we want to shade the background of a revealed cell. This is useful in the end-game scenario, where we assist the user in showing that all the cells left unchecked (since we don't have the ability to flag) are indeed all the mines.

In Board:

void reveal(Color color) {

    forEach(cell -> cell.reveal(color));

}



boolean isDone() {

    int result = new int[1];

    forEach(cell -> { if (cell.isEmpty()) { result[0]++; }});

    return result[0] == MINES;

}

The int result is just to get past the limitation that our operation can only be done on a mutable container.

In Cell:

void reveal(Color color) {

    displayValue(color);

    checked = true;

    button.setEnabled(!checked);

}



private void displayValue(Color color) {

    if (isMine()) {

        button.setText("u2600");

        button.setBackground(color);

    } else if (value != 0) {

        button.setText(String.valueOf(value));

    }

}

Planting mines

Again, @Simon has some good pointers about your generateMinesLocation() method, but I think it can be further improved... by removing it entirely. Why?

• It is the only place where a cell ID is used.


• You don't really need a cell ID as long as you can provide your x, y or row, column coordinates.


• Nothing beats a straightforward reference in a 2D array.

Therefore, all you need is your plantMines():

private void plantMines() {

    Random random = new Random();

    int counter = 0;

    while (counter != MINES) {

        counter += cells[random.nextInt(SIDE)][random.nextInt(SIDE)].setMine();

    }

}

Cell.setMine() returns 1 if the mine is set, else 0. We can then make use of the return value to increment the total number of mines we have until the desired number of MINES (extracted out as a static final field).

Banishing the if nest

Now onto the exciting part... What's a reasonable way of generating a 3x3 matrix centered around x, y? We'll need the valid preceding and succeeding values, i.e. x-1 ... x+1 and y-1 ... y+1, and then every permutation of them except x,y itself:

1. 
   

   A method to get valid preceding and succeeding values:

   
   
   

   private IntStream sidesOf(int value) {
   
       return IntStream.rangeClosed(value - 1, value + 1).filter(
   
               x -> x >= 0 && x < SIDE);
   
   }
   
   

   
   


2. 
   

   Permute horizontally and vertically, and then exclude x,y:

   
   
   

   private Set<Cell> getSurroundingCells(int x, int y) {
   
       Set<Cell> result = new HashSet<>();
   
       sidesOf(x).forEach(a -> {
   
           sidesOf(y).forEach(b -> result.add(cells[a][b]));
   
       });
   
       result.remove(cells[x][y]);
   
       return result;
   
   }
   
   

   
   

The nest of if statements is essentially describing:

• If the current cell (1 of 8) matches a specific criteria
  
  
  
  
  • Perform an operation either on that cell, or the cell at x,y
    
  
  
  
  

As such, with our getSurroundingCells() method, we can implement that as a pair of filter-and-forEach steps:

getSurroundingCells(x, y).stream().filter(predicate).forEach(consumer);

Final post-code-refactoring for the two main methods:

/**

 * This method count number of mines around particular cell and set its value

 */

private void setCellValues() {

    IntStream.range(0, SIDE).forEach(x -> {

        IntStream.range(0, SIDE).forEach(y -> {

            if (!cells[x][y].isMine()) {

                getSurroundingCells(x, y).stream().filter(Cell::isMine)

                    .forEach(z -> cells[x][y].incrementValue());

            }

        });

    });

}



/**

 * This method starts chain reaction. When user click on particular cell, if cell is

 * empty (value = 0) this method look for other empty cells next to activated one.

 * If finds one, it call checkCell and in effect, start next scan on its closest

 * area.

 */

void scanForEmptyCells() {

    IntStream.range(0, SIDE).forEach(x -> {

        IntStream.range(0, SIDE).forEach(y -> {

            if (cells[x][y].isChecked()) {

                getSurroundingCells(x, y).stream().filter(Cell::isEmpty)

                    .forEach(Cell::checkCell);

            }

        });

    });

}

Bonus! Resetting a Board

Minesweeper addicts* playing your application may revel in the clutter-free interface and possibly even the simplicity of non-flagging and having to maintain a mental map of the mines, but they'll probably be dismayed that starting a new game involves restarting the application... How hard will it be to implement a reset feature?

In Board:

private JFrame getBoard() {

    JFrame frame = new JFrame();

    frame.getContentPane().setLayout(new BorderLayout());

    frame.add(addCells(), BorderLayout.NORTH);

    frame.add(addControlPanel(), BorderLayout.SOUTH);

    ...

}



private JPanel addControlPanel() {

    JPanel panel = new JPanel();

    JButton reset = new JButton("Reset");

    reset.addActionListener(listener -> reset());

    panel.add(reset);

    return panel;

}



void reset() {

    forEach(cell -> cell.reset());

    init();

}

Just in case you forgot, init() does the plantMines() and setCellValues() for us.

In Cell:

void reset() {

    setValue(0);

    button.setText("");

    button.setBackground(null);

    checked = false;

    button.setEnabled(!checked);

}

* - I will neither confirm nor deny if this includes myself.

Have fun!

Use the Random class:

Random rand = new Random();

...

random = rand.nextInt(side * side);

i and j, what is row and what is column?

for(int i = 0; i<side; i++){

    for(int j = 0; j<side; j++){

         if(cells[i][j].getValue() != -1){

I have no clue by looking at these lines what is x and what is y. I strongly suggest you rename your variables to x and y, or row and col instead.

getCell is slow

You are currently looping through the cells until you find one that matches the id. This is slow. As this is only called from generateMinesLocation, you can instead have generateMinesLocation return a List<Cell>. Or even better: Set<Cell> to make the loc.contains lookup much faster, because:

• The order doesn't matter


• There should be no duplicates

These two conditions makes it a perfect for Set<Cell> loc = new HashSet<>();

This will eliminate the need for a id on a Cell.

value == -1 means that it is a mine

Once upon a time, I also used a special value to indicate that a cell was a mine. Later I realized my mistake. It is really much better to use an additional boolean for the property of whether or not it is a mine. And you should definitely include a boolean isMine() function.

Not only does this make the code cleaner, it also allows a future mode where a Cell can be both a number and a mine at the same time. (Trust me, the possibilities are endless!)

notChecked

public boolean isNotChecked(){

    return notChecked;

}

Save yourself some trouble and rename this to checked and isChecked(). Or even better: clicked and isClicked().

Writing if (!cell.isClicked()) is much more preferable than if (cell.isNotClicked())