2
\$\begingroup\$

Here is my take on Minesweeper in Java. Any feedback is welcome regarding readability, design or anything really.

Cell.java

public class Cell {

    public static final int BOMB = -1;

    boolean revealed = false;
    int row, col, val;

    public Cell(int... coordinates) {
        this.row = coordinates[0];
        this.col = coordinates[1];
    }

    boolean isBomb() {
        return val == BOMB;
    }

    boolean isEmpty() {
        return val == 0;
    }

}

Board.java

class Board {

    enum BoardResponse {
        OK, BOMB, ALL_CELLS_REVEALED
    }

    Cell[][] rows;

    // Returns a new Board with a few bombs and the values around the bombs.
    public static Board newBoard() {
        Board board = new Board();
        board.rows = new Cell[7][7];  // todo: Accept parameters instead of 7,7

        for (int i = 0; i < board.rows.length; i++)
            for (int j = 0; j < board.rows[i].length; j++)
                board.rows[i][j] = new Cell(i, j);

        board.rows[1][1].val = Cell.BOMB;  // todo: randomise bomb placement
        board.rows[3][3].val = Cell.BOMB;  // make sure bombs not adjacent
        board.rows[5][5].val = Cell.BOMB;

        // Fill bombs surroundings with values.
        for (Cell[] row : board.rows)
            for (Cell cell : row)
                if (cell.isBomb())
                    board.surroundingCells(cell).forEach(c -> c.val = c.val + 1);

        return board;
    }

    // Accepts a row and a column, modifies the state as needed and returns 
    // whether game is finished, a bomb has been clicked or game continues.
    BoardResponse flipCell(int... coordinates) {
        Cell clicked = rows[coordinates[0]][coordinates[1]];
        clicked.revealed = true;

        // Player lost
        if (clicked.isBomb())
            return BoardResponse.BOMB;

        // Check if game finished, i.e. player won..
        // For the game to finish all cells
        // (except the ones holding bombs) must be revealed.
        boolean allCellsRevealed = true;
        isAllCellsRevealed:
        for (Cell[] row : rows)
            for (Cell cell : row)
                if (!cell.revealed && !cell.isBomb()) {
                    allCellsRevealed = false;
                    break isAllCellsRevealed;
                }

        if (allCellsRevealed)
            return BoardResponse.ALL_CELLS_REVEALED;

        // If the cell clicked on is an empty cell,
        // reveal all surrounding cells recursively until valued cells.
        if (clicked.isEmpty()) {
            Queue<Cell> queue = new LinkedList<>();
            surroundingCells(clicked)
                    .stream().filter(c -> !c.revealed).forEach(queue::add);
            while (!queue.isEmpty()) {
                Cell cell = queue.remove();
                cell.revealed = true;
                if (cell.isEmpty()) {
                    Set<Cell> cells = surroundingCells(cell);
                    cells.stream().filter(c -> !c.revealed).forEach(queue::add);
                }
            }
        }

        return BoardResponse.OK;
    }

    // Given a single cell in the board, returns all surrounding cells in a Set.
    private Set<Cell> surroundingCells(Cell cell) {
        Set<Cell> surroundingCells = new HashSet<>();

        // Cells in upper row
        for (int i = cell.col - 1; i < cell.col + 2; i++)
            if (inBounds(cell.row - 1, i))
                surroundingCells.add(rows[cell.row - 1][i]);

        // Cells in lower row
        for (int i = cell.col - 1; i < cell.col + 2; i++)
            if (inBounds(cell.row + 1, i))
                surroundingCells.add(rows[cell.row + 1][i]);

        // Cell to left
        if (inBounds(cell.row, cell.col - 1))
            surroundingCells.add(rows[cell.row][cell.col - 1]);

        // Cell to right
        if (inBounds(cell.row, cell.col + 1))
            surroundingCells.add(rows[cell.row][cell.col + 1]);

        return surroundingCells;
    }

    // Helper method to surroundingCells.
    // Tries to access the cell in given coordinates handling OutOfBoundException.
    private boolean inBounds(int... coordinates) {
        try {
            Cell cell = rows[coordinates[0]][coordinates[1]];
            return true;
        } catch (ArrayIndexOutOfBoundsException e) {
            return false;
        }
    }

}

And here is a command line client I have:

public class MinesweeperCli {
    public static void main(String[] args) {

        Scanner in = new Scanner(System.in);

        Board board = Board.newBoard();
        Board.BoardResponse boardResponse;
        do {
            int row = in.nextInt();
            int col = in.nextInt();
            boardResponse = board.flipCell(row, col);
            printBoard(board);
            if (boardResponse == Board.BoardResponse.BOMB) {
                System.out.println("Bomb!");
                break;
            }
        } while (boardResponse != Board.BoardResponse.ALL_CELLS_REVEALED);

        in.close();
    }

    public static void printBoard(Board board) {
        StringBuilder sb = new StringBuilder();

        for (Cell[] row : board.rows) {
            for (Cell cell : row)
                sb.append(cellRep(cell)).append(" ");
            sb.append("\n");
        }

        System.out.println(sb.toString());
    }

    public static String cellRep(Cell cell) {
        if (!cell.revealed)
            return "-";

        if (cell.isBomb())
            return "*";

        if (cell.isEmpty())
            return ".";

        return valueOf(cell.val);
    }

}

A Sample Run

0
0
1 - - - - - - 
- - - - - - - 
- - - - - - - 
- - - - - - - 
- - - - - - - 
- - - - - - - 
- - - - - - - 

6
0
1 - - - - - - 
- - - - - - - 
1 1 2 - - - - 
. . 1 - - - - 
. . 1 1 2 - - 
. . . . 1 - - 
. . . . 1 - - 

1
1
1 - - - - - - 
- * - - - - - 
1 1 2 - - - - 
. . 1 - - - - 
. . 1 1 2 - - 
. . . . 1 - - 
. . . . 1 - -

Bomb!
\$\endgroup\$
5
\$\begingroup\$
  • Unabbreviated names would be nicer.
  • Immutable properties (row, column) should be made final.
  • Package private directly accessible - especially mutable - fields are not liked.
  • ... have their uses, but not here.
  • {} in generally also are Always used.

So:

public class Cell {

    public static final int BOMB = -1;

    final int row;
    final col;

    boolean revealed;
    int value;

    public Cell(int row, int col) {
        this.row = row;
        this.col = col;
    }

Critics:

  • cellRep belongs more to class Cell. You could make it a char method, in order to have just one char, for the board representation.
  • printBoard belongs more to Board.
  • You might consider the board size as constructor Parameters too.

Design criticism:

row and col are redundant.

There are also a couple of pitfalls you avoided, so in general the code is not bad.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I am planning on creating a UI with Swing so I am not sure if printBoard belongs to Board. Same for cellRep. Thank you for your valuable response. \$\endgroup\$ – Koray Tugay Sep 2 at 13:44
  • \$\begingroup\$ A String toString() would be nice for logging though. But okay. \$\endgroup\$ – Joop Eggen Sep 2 at 13:46
  • \$\begingroup\$ What “pitfalls” were avoided? \$\endgroup\$ – Peter Sep 2 at 15:09
  • 2
    \$\begingroup\$ @Peter using StringBuilder, using inBounds. Shows some insights. Despite not keeping to main-stream coding conventions. \$\endgroup\$ – Joop Eggen Sep 2 at 15:32
  • \$\begingroup\$ @JoopEggen I see and makes sense. A toString ignoring isRevealed and only printing the values would be nice. \$\endgroup\$ – Koray Tugay Sep 3 at 11:20
1
\$\begingroup\$

It seems unclear what responsibilities the Cell class is supposed to have. You obtain a Cell from the Board and then query the cell for information about whether it is flipped or not but to flip a cell or get the surrounding cells you have to make a request to Board.

To me it would make sense that the operations that are done on a Cell would be done through the Cell in question. The cell would provide information about whether it is flipped or not. Whether it has been flagged or not. It's status after flipping (empty, number of surrounding mines, bomb).

The board would provide information about the state of the game and access to cells.

Even though the model is internal to your CLI/Swing UI, you should still maintain separation of concerns. Access to object's fields should be done through accessor methods, not directly, and the fields should be private. Once you start modifying object's internal state from other objects you introduce yourself to a food fight in an Italian restaurant. That spaghetti gets really messy.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your valuable time and review. Access to object's fields should be done through accessor methods I really do not care much about this when just playing with code myself. Regarding You obtain a Cell from the Board and then query the cell for information about whether it is flipped or not but to flip a cell or get the surrounding cells you have to make a request to Board., how would you do it? How can a single cell know about its surroundings? \$\endgroup\$ – Koray Tugay Sep 4 at 10:59
  • \$\begingroup\$ Please don't post if you're not interested in feedback. We're doing this for free and it just wastes our time. \$\endgroup\$ – TorbenPutkonen Sep 4 at 15:55
  • \$\begingroup\$ I am not telling I am not interested in feedback. I actually asked you a specific question on your feedback, if you realised. I do not have to accept all the feedback, do I? \$\endgroup\$ – Koray Tugay Sep 4 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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