Exact Replica of Windows Minesweeper in Java

I am a beginner programmer trying to self learn how to write a code. To improve my skills, I'm currently working on a project to create an exact replica of windows minesweeper. It is written in JavaFX and most of the basic code is done. I am able to run the program to play minesweeper without bugs (maybe).

If anybody wants to review my code (though I highly doubt it due to the code size), I would greatly appreciate it.

JavaFX Backbone (Ignore this)

```
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.input.MouseButton;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class Game extends Application {

private NumberDisplay mineCount = new NumberDisplay(DIGITS);
private int numMine;
private TimeDisplay time = new TimeDisplay(DIGITS);
private Board board;
private MainGame mainGame;

public static void main(String[] a){
launch(a);
}

public void start(Stage stage) throws Exception {
board = new Board(9, 9);
mainGame = new MainGame(board, Difficulty.EASY);

updateMineCount();

HBox numberLayout = new HBox(10);
VBox mainLayout = new VBox(10);

numberLayout.getChildren().addAll(time, mineCount);
mainLayout.getChildren().addAll(numberLayout, mainGame);

Scene scene = new Scene(mainLayout);

stage.setScene(scene);
time.start();
stage.show();

mainGame.setOnMouseClicked(e -> {

if (mainGame.isEnd()){

time.stop();

if (mainGame.isWin()){
win();
} else {
lose();
}

} else {

if (e.getButton().equals(MouseButton.SECONDARY)){
updateMineCount();
}

}

});

}

private void updateMineCount(){
numMine = board.getNumMine() - Cell.getNumFlag();

Game

In the bottom of Game, you have this line:

private static final int DIGITS = 3;

This should be up at the top of your class along with the other fields. Other than that, it is a little confusing why that is residing so far away from your other fields.

board = new Board(9, 9);
mainGame = new MainGame(board, Difficulty.EASY);

There is no point in storing board because you never use it again (other than in the next line).

I recommend just inserting new Board into the next line without storing it first in board.

This is what I mean:

mainGame = new MainGame(new Board(9, 9), Difficulty.EASY);

In your lose method of Game, you do the following:

System.out.println("lose" + time.getTime());

That is a good idea to store the time it took the user, then to show it to the user.

However, for some reason, you don't do that in win. Why not? I don't know about you, but I'd want to see how long it took me even if I won the game.

MainGame

This method:

private void assignEvent(Board board)

has a very un-descriptive name. I would call it something more like setFlagAndSelectEvents (or something along those lines).

By the way, this was a very nice method that was easy to follow even with the lack of comments.

Instead of having a field called win and a field called end, I recommend just keeping win but having it default to null.

This will tell you three things now:

• When set to null, the game is not over

• When set to true, the user won the game

• When set to false, the user lost the game

This will not work if you can't set a boolean value to null. However, I don't remember if you can (I could not test it at the moment)

Board

Good job putting random as it's own field.

On this line of initEmptyCell and initMineCount

for (int j = 0; j < cells[0].length; j++){

Did you mean to write:

cells[i].length

These fields:

private static final int EASY_FACTOR = 8;
private static final int MEDIUM_FACTOR = 6;
private static final int HARD_FACTOR = 4;

Should be an enum, rather than separate fields.

It took me a long time to try and figure out what initMineCount was doing.

The reason, this bad method name:

private int getMineCount(int x, int y)

To me, this sounds like "get the amount of mines on the board". I think you should change the name to something that expresses neighboring.

Here is what I came up with:

private int getSurroundingMineCount(int x, int y)

Cell

You are inconsistent in how you check the object's own properties. For example, in select you write:

if (isFlagged){
    [code]
}

And, in flag you write:

if (this.isFlagged()){
    [code]
}

I recommend you choose the first version because that way you don't have to call a method every time.

This is poorly indented:

switch(id){
case MINE: return (String.format("mine, %d", mineCount));
default: return (String.format("empty, %d", mineCount)); 
}

It should look like this:

switch(id){
    case MINE: return (String.format("mine, %d", mineCount));
    default: return (String.format("empty, %d", mineCount)); 
}

All these images:

private static Image unselected = new Image("image/unselected.png");
private static Image mine = new Image("image/mine.png");
private static Image flag = new Image("image/flag.png");
private static Image zero = new Image("image/zero.png");
private static Image one = new Image("image/one.png");
private static Image two = new Image("image/two.png");
private static Image three = new Image("image/three.png");
private static Image four = new Image("image/four.png");
private static Image five = new Image("image/five.png");
private static Image six = new Image("image/six.png");
private static Image seven = new Image("image/seven.png");
private static Image eight = new Image("image/eight.png");

Remain constant throughout the code, and even some other classes need them (Board, for example).

I recommend moving these to their own enum. That way, the values stay constant and other classes can access them.

This is what the enum would look like:

public enum StateImage {
    UNSELECTED(new Image("...")),
    MINE(new Image("...")),
    ...

    private Image image;
    private StateImage(Image image) {
        this.image = image;
    }

    public Image getImage() {
        return this.image
    }
}

Instead of storing the state of being selected and the state of being flagged as booleans, I recommend creating an enum called CellState that has values FLAGGED and SELECTED and UNSELECTED

And, if you want to take it a step further, you can set

-
FLAGGED to 2

-
UNSELECTED to 0

Notice something? These values directly relate to the enums of StateImage so you can easily set and remove images.

Hint: StateImage.FLAG == CellState.FLAGGED

Difficulty

Instead of creating an enum for the difficulty factors like I recommended, you could j

StackExchange Code Review Q#96559, answer score: 10