A Minesweeper implementation in Java

Question

As I'm working with a proprietary programming language in my daily job, my java skills unfortunately missed out. In order to change this I am currently working on a minesweeper clone. So far, I have finished the basic implementation (business logic and command line view). The next step will be a graphical user interface.

Now I am looking for tips from experienced java developers in order to further improve the code and myself.

How can I make the code more readable and simplistic?
Are there any code smells?
Can I use more design pattern to get more flexibility?

I would appreciate your comments and any kind of positive and negative feedback.

The full and up-to-date code is published on GitHub.

Here are some original code extracts:

Cell.java

public class Cell implements CellRO {

    private boolean isVisited;
    private boolean isMine;
    private boolean markedAsBomb;
    private int countOfNeighbourMines;

    public int getCountOfNeighbourMines() {
        return countOfNeighbourMines;
    }

    public void setCountOfNeighbourMines(final int count) {
        countOfNeighbourMines = count;
    }

    public Cell() {
        isVisited = false;
        isMine = false;
        markedAsBomb = false;
        countOfNeighbourMines = 0;
    }

    public boolean isVisited() {
        return isVisited;
    }

    public boolean isMine() {
        return isMine;
    }

    public boolean isMarkedAsBomb() {
        return markedAsBomb;
    }

    public void visit() {
        if (isVisited) {
            throw new IllegalStateException("A cell can only be visited once!");
        }
        isVisited = true;
    }

    public void setMine() {
        isMine = true;
    }

    public void changeMarkedAsBomb() {
        markedAsBomb = !markedAsBomb;
    }

    public void removeMine() {
        isMine = false;
    }
}

Cellbuilder.java

import model.cell.Cell;
import java.util.Random;
import java.util.concurrent.ThreadLocalRandom;

public final class CellBuilder {

    private final int countOfRows;
    private final int countOfColumns;
    private final int countOfMines;

    public CellBuilder(final int rowCount, final int colCount, final int minesCount) {
        countOfRows = rowCount;
        countOfColumns = colCount;
        countOfMines = Math.max(0, Math.min(minesCount, countOfRows * countOfColumns));
    }

    public Cell[][] buildBoard() {
        Cell[][] board = new Cell[this.countOfRows][this.countOfColumns];
        populateBoardWithCells(board);
        populateBoardWithMines(board);
        return board;
    }

    private void populateBoardWithCells(final Cell[][] board) {
        for (int row = 0; row < this.countOfRows; row++) {
            for (int col = 0; col < this.countOfColumns; col++) {
                board[row][col] = new Cell();
            }
        }
    }

    private void populateBoardWithMines(final Cell[][] board) {
        int mines = 0;
        for (int row = 0; row < this.countOfRows; row++) {
            for (int col = 0; col < this.countOfColumns; col++) {
                if (mines < countOfMines) {
                    board[row][col].setMine();
                    mines++;
                }
            }
        }
        shuffleBoard(board);
    }

    private void shuffleBoard(final Cell[][] board) {
        // Fisher–Yates algorithm
        Random ran = new Random();

        for (int row = board.length - 1; row > 0; row--) {
            for (int col = board[row].length - 1; col > 0; col--) {
                int rowRandom = ran.nextInt(row + 1);
                int colRandom = ran.nextInt(col + 1);

                Cell temp = board[row][col];
                board[row][col] = board[rowRandom][colRandom];
                board[rowRandom][colRandom] = temp;

            }
        }
    }

    public int getCountOfMines() {
        return this.countOfMines;
    }
}

Board.java

import model.cell.Cell;
import model.cell.NullCell;
import util.GameDifficulty;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;

public final class Board {

    public static final int MINIMUM_ROWS = 4;
    public static final int MINIMUM_COLUMNS = 4;

    private final Cell[][] field;
    private final int rowCount;
    private final int colCount;
    private final int countOfMines;

    /*
    Default constructor will use default values
     */
    public Board() {
        this(MINIMUM_ROWS, MINIMUM_COLUMNS, GameDifficulty.EASY);
    }


    public Board(final int countOfRows, final int countOfColumns,
                 final GameDifficulty difficulty) {
        rowCount = Math.max(countOfRows, MINIMUM_ROWS);
        colCount = Math.max(countOfColumns, MINIMUM_COLUMNS);
        countOfMines = getCountOfMinesByDifficulty(difficulty);

        CellBuilder builder = new CellBuilder(rowCount, colCount, countOfMines);
        field = builder.buildBoard();
        setNeighbourCount();
    }

    public Board(final Cell[][] fieldOfCells, final int mineCount) {
        this.rowCount = fieldOfCells.length;
        this.colCount = fieldOfCells[0].length;
        this.countOfMines = mineCount;
        this.field = fieldOfCells;
    }

    private int getCountOfMinesByDifficulty(final GameDifficulty difficulty) {
        return (int) (difficulty.getPercentage() * getCountOfCells());
    }

    public Cell getCell(final int row, final int col) {
        if (!isValidPosition(row, col)) {
            return NullCell.getInstance();
        }
        return field[row][col];
    }

    private void setNeighbourCount() {
        for (int row = 0; row < rowCount; row++) {
            for (int col = 0; col < colCount; col++) {
                Cell cell = getCell(row, col);
                cell.setCountOfNeighbourMines(getCountOfNeighbourMines(cell));
            }
        }
    }

    private boolean isValidPosition(final int row, final int col) {
        if (row < 0 || row >= rowCount) {
            return false;
        }

        if (col < 0 || col >= colCount) {
            return false;
        }

        return field[row][col] != null;
    }

    public int getRowCount() {
        return this.rowCount;
    }

    public int getColCount() {
        return this.colCount;
    }

    public List<Cell> getNeighbourCells(final Cell cell) {
        for (int row = 0; row < rowCount; row++) {
            for (int col = 0; col < colCount; col++) {
                if (getCell(row, col) == cell) {
                    return getNeighbourCells(row, col);
                }
            }

        }
        return Collections.emptyList();
    }

    private List<Cell> getNeighbourCells(final int row, final int col) {
        List<Cell> neighbours = new ArrayList<>();
        if (isValidPosition(row - 1, col - 1)) {
            neighbours.add(field[row - 1][col - 1]);
        }
        if (isValidPosition(row - 1, col)) {
            neighbours.add(field[row - 1][col]);
        }
        if (isValidPosition(row - 1, col + 1)) {
            neighbours.add(field[row - 1][col + 1]);
        }
        if (isValidPosition(row, col - 1)) {
            neighbours.add(field[row][col - 1]);
        }
        if (isValidPosition(row, col + 1)) {
            neighbours.add(field[row][col + 1]);
        }
        if (isValidPosition(row + 1, col - 1)) {
            neighbours.add(field[row + 1][col - 1]);
        }
        if (isValidPosition(row + 1, col)) {
            neighbours.add(field[row + 1][col]);
        }
        if (isValidPosition(row + 1, col + 1)) {
            neighbours.add(field[row + 1][col + 1]);
        }
        return neighbours;
    }

    public int getCountOfNeighbourMines(final Cell cell) {
        int countOfNeighbourMines = 0;
        List<Cell> neighbours = getNeighbourCells(cell);
        for (Cell neighbour : neighbours) {
            if (neighbour.isMine()) {
                countOfNeighbourMines++;
            }
        }
        return countOfNeighbourMines;
    }

    public void moveMineToRandomCell(final int row, final int col) {
        Cell cell = getCell(row, col);
        if (!cell.isMine()) {
            return;
        }
        int rowNew = row;
        int colNew = col;
        Random ran = new Random();
        while (rowNew == row || colNew == col || getCell(rowNew, colNew).isMine()) {

            rowNew = ran.nextInt(rowCount);
            colNew = ran.nextInt(colCount);
        }
        cell.removeMine();
        getCell(rowNew, colNew).setMine();
    }

    public int getCountOfMines() {
        return countOfMines;
    }

    private int getCountOfCells() {
        return rowCount * colCount;
    }
}

GameStateRunning.java

import model.GameModelImpl;
import model.cell.Cell;

import java.util.List;

public class GameStateRunning implements GameModelState {

    @Override
    public void openCell(final GameModelImpl context, final int row, final int col) {
        Cell cell = context.getCell(row, col);
        if (cell.isMine()) {
            context.setState(new GameStateLost());
            return;
        }

        if (cell.isVisited() || context.getBoard().getCountOfNeighbourMines(cell) == 0) {
            openCellR(context, cell);
        } else {
            cell.visit();
        }

        if (gameIsWon(context)) {
            context.setState(new GameStateWon());
        }
    }

    @Override
    public void changeMarkedAsBomb(final GameModelImpl context, final int row, final int col) {
        Cell cell = context.getCell(row, col);
        cell.changeMarkedAsBomb();
    }

    @Override
    public void visitAllAndRemoveMarks(final GameModelImpl context) {
        // Operation not allowed in this state!
    }

    private void openCellR(final GameModelImpl context, final Cell cell) {
        List<Cell> neighbours = context.getBoard().getNeighbourCells(cell);
        for (Cell neighbour : neighbours) {
            if (neighbour.isVisited()) {
                continue;
            }

            if (cell.isMine() && !cell.isMarkedAsBomb()) {
                context.setState(new GameStateLost());
                return;
            } else {
                neighbour.visit();

                if (context.getBoard().getCountOfNeighbourMines(neighbour) == 0) {
                    openCellR(context, neighbour);
                }
            }
        }
    }

    private boolean gameIsWon(final GameModelImpl context) {
        for (int row = 0; row < context.getBoard().getRowCount(); row++) {
            for (int col = 0; col < context.getBoard().getColCount(); col++) {
                if (!context.getBoard().getCell(row, col).isMine() && !context.getBoard().getCell(row, col).isVisited()) {
                    return false;
                }
            }
        }
        return true;
    }
}

You should rollback your latest edit, "do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code.". — Nevay
– Nevay, Commented Sep 3, 2017 at 11:50
TDD is a process and a tool to develop a program. But this site is for existing working code. Therefore it is meaningless which way it has been written and TDD should not be mentioned in the title (unless you also want to discuss your unit tests) - But don't get me wrong! I appreciate that you go for TDD! — Timothy Truckle
– Timothy Truckle, Commented Sep 5, 2017 at 19:22

slowy · Accepted Answer · 2017-09-03 16:33:49Z

It's quite decent.

Some smaller things:

@Override: Always use the @Override annotation. First of all, it tells others, that this method is overwritten. And if you do an error, e.g. use a wrong method signature, the compiler will jump in.
Ordering of elements within a class: I don't think there's a convention, but in Cell, you start with member vars, public getter/setter, constructor, and public methods again.
visit: Not entirely sure, if visit means 'clicked the cell'?
mine: I don't get that either. First I thought that's the state, when the user marks it as mine. But you also have a method changeMarkedAsBomb.
The CellBuilder is misleading, because there's a pattern called Builder. Maybe in your case a 'static factory' would be better.
Okay, that's nitpicking: If I read a variable name called count..., I assume it's for counting something. Maybe numberOf or amountOf would suit better.
As janos mentioned, to populate the cells and then shuffle it is a bit fancy. Maybe the 2d array isn't the best data structure. A Set or a Map would make a few things easier.
Cell[][] field; I'd name it cells
rowCount = Math.max(countOfRows, MINIMUM_ROWS); I think if the row or column size exceeds the max values, it should throw an Exception, because a user could create a game using 3*3 and he would receive a 4*4 board.
Board constructors: I don't think the third constructor is necessary. I'd even say, it must not be there. Because if I want to use it, I have to create the cells myself. I think the creation of the cells is an implementation detail which should be hidden. Other than that: The second constructor should call the third constructor, because of code duplication. In the third constructor, you do not have the row/colCount assertion, so I'm able to pass a 1*1 field.
return rowCount * colCount; This shouldn't change during the game. So rowCount, colCount and cellCount can be final.

Hope this helps,

slowy

Thanks for your comments, slowy! I already changed my code considering your hints. Now it's more tidy. — Oliver530
– Oliver530, Commented Sep 3, 2017 at 20:46

janos · Accepted Answer · 2017-09-03 07:01:15Z

Performance

The method getNeighbourCells(Cell) and all the methods that call it will perform poorly. This method scans the entire board until it finds the matching cell. There is another version of this method that takes (int, int) parameters. That should be used instead, everywhere, and the other eliminated. (After a quick scan in the code, this seems possible with minor changes.) If you really need a way to find the neighbors by Cell rather than coordinates, then I suggest to consider a different design that would make such lookups more efficient, for example caching in a hash map.

Selecting random cells to place mines

Picking a random cell from the grid and skipping if it's already a mine is a bit ugly. Theoretically, it might never terminate. If the caller mistakenly tries to place more mines then cells, then it will never terminate. If there is a relatively high rate of mines, it will be slow. Long story short, it's a bit problematic.

You might be interested in an alternative that's more elegant, at the expense of extra memory (double the size of the board): create an array with all the cells, shuffle the array, and then pick the first N elements of the array.

Java usage

By "usage" I mean some common practical patterns that can be applied to bring some technical benefits, for example:

Instead of returning new ArrayList<>(), if that value will never be modified, then it's better to return Collections.emptyList(), which will save you unnecessary object construction.
I don't see a reason to use ThreadLocalRandom instead of Random. I suggest to use the latter.

If you are interested in useful usage patterns in Java, I recommend Joshua Bloch's book Effective Java, it's got a ton of those.

Thanks a lot for your review, janos! To improve the random mine generation I implemented a modified Fisher–Yates algorithm. Now it should be much more efficient. The next step will be a caching mechanism as you suggested. The other tips are also useful. Thanks a lot! — Oliver530
– Oliver530, Commented Sep 3, 2017 at 8:32

Roland Illig · Accepted Answer · 2017-09-03 05:53:11Z

3

Overall, your code is well-structured and very readable.

I don't like the concept of a NullCell, since it hides programming mistakes. I prefer an exception to be thrown in such a case. In the end, your UI will prevent that any of these exception could be thrown.

You should remove the class CellBuilder, since its name is wrong (it doesn't build individual cells, but arrays of them). The code should move into the Board class. By that, the two places where the random number generator is used are closer to each other.

Instead of the current setMine I prefer the classical variant that only sets the field unconditionally and returns void. There's only one place where this extra complexity is actually used.

In some cases you used different grammar for the same concept, e.g. countOfRows and rowCount. Be more consistent here.

You could try whether your variable names can be shorter. I would rename rowRandom to just row, and numberOfRows to just rows.

A Cell should not know anything about its neighbors. This kind of information belongs to the board and should probably be computed instead of stored. Or, if you store it, write tests that when the player clicks on a mine with the very first click, all neighbor counts are properly updated.

Oh, and a last tip: don't add design patterns before you actually need them. They just bloat the code. One example is your CellBuilder, which currently separates code that belongs together.

answered Sep 3, 2017 at 5:53

Roland Illig

21.9k2 gold badges36 silver badges84 bronze badges

\$\begingroup\$ Thanks for your feedback, Roland! I have removed the NullObject pattern and shorten/unified the variable names. Now the code should be more readable. The CellBuilder is now a static BoardBuilder. Would you integrate the code into the Board class? I divided the functionality because of better UnitTesting possibilities and because of the principles Separation of concerns and Method Object. Looking forward to hearing from you. \$\endgroup\$

Oliver530
– Oliver530

2017-09-03 11:32:20 +00:00
Commented Sep 3, 2017 at 11:32
\$\begingroup\$ Yes, I would integrate the code into the board class because then your unit tests can easily work with a realistic board. By the way, you need better tests. Your shuffle code won't shuffle in row 0. Therefore it's easier to shuffle a one-dimensional array first and then map it onto the two-dimensional board. \$\endgroup\$

Roland Illig
– Roland Illig

2017-09-03 15:35:47 +00:00
Commented Sep 3, 2017 at 15:35
1

\$\begingroup\$ "A Cell should not know anything about its neighbors." This is a point to be discussed. I'd say there should be no board class (or object) at all and the "board" should build up through the neighbor relationship of the individual cells. This approach has some neat advantages, eg.: you need only deal with corners and edges during initialization, not during "play time" and automatic uncovering is a (recursive) call of an uncovered cell to its known neighbors if non of this neighbors holds a bomb... \$\endgroup\$

Timothy Truckle
– Timothy Truckle

2017-09-05 19:09:56 +00:00
Commented Sep 5, 2017 at 19:09

Add a comment |

Timothy Truckle · Accepted Answer · 2017-09-05 19:48:10Z

Thanks for sharing your code!

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Don't surprise your readers

As already mentioned the CellBuilder class has a method buildBoard() which is indeed surprising and shows that either the class or the method should have a different name.

Don't use abbreviations

Your Cell class implements an interface called CellRO. What does RO stand for?

Also you have a method openCellR(). I can only guess that R stands for recursive...

Interface design

Your class GameStateRunning implements the interface GameModelState and has an empty implementation of the method visitAllAndRemoveMarks(). This is an indication that this method should not be member of this interface. The need to put this method into this interface may indicate a design problem somewhere else in your program.

Code duplication

The method getNeighbourCells() could be shortened if you replace the duplicated lines with loops like this:

private List<Cell> getNeighbourCells(final int row, final int col) {
    List<Cell> neighbours = new ArrayList<>();
    for(int rowOffset = -1; rowOffset < 2; rowOffset++)
      for(int colOffset = -1; colOffset < 2; colOffset++)
        if (rowOffset != 0 && colOffset != 0) // skip cell itself
          if (isValidPosition(row + rowOffset , col + colOffset)) {
             neighbours.add(field[row + rowOffset][col  + colOffset]);
          }
    return neighbours;
}

Stack Exchange Network

A Minesweeper implementation in Java

4 Answers 4

Performance

Selecting random cells to place mines

Java usage

Naming

Don't surprise your readers

Don't use abbreviations

Interface design

Code duplication

You must log in to answer this question.

Hot Network Questions

A Minesweeper implementation in Java

4 Answers 4

Performance

Selecting random cells to place mines

Java usage

Naming

Don't surprise your readers

Don't use abbreviations

Interface design

Code duplication

You must log in to answer this question.

Related

Hot Network Questions