13
\$\begingroup\$

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();
        mineCount.setNumber(numMine);
        mineCount.update();
    }

    private void win(){
        System.out.println("win");
    }

    private void lose(){
        System.out.println("lose" + time.getTime());
    }

    private static final int DIGITS = 3;


}

MainGame Class (Main Class that Contains Most of the Game Logic)

import javafx.scene.image.ImageView;
import javafx.scene.input.MouseButton;
import javafx.scene.layout.GridPane;



public class MainGame extends GridPane{

    private ImageView[][] cell;
    private boolean win;
    private boolean end;

    public MainGame(Board board, Difficulty difficulty){

        board.init(difficulty);

        cell = new ImageView[board.getYSize()][board.getXSize()];

        for (int i = 0; i < board.getYSize(); i++){
            for (int j = 0; j < board.getXSize(); j++){

                cell[i][j] = new ImageView(board.getCell(j, i).getUnselectedImage());   
                cell[i][j].setFitHeight(CELL_SIZE);
                cell[i][j].setFitWidth(CELL_SIZE);
                GridPane.setRowIndex(cell[i][j], i + 1);
                GridPane.setColumnIndex(cell[i][j], j + 1);
                this.getChildren().add(cell[i][j]);

            }
        }

        assignEvent(board);

    }

    private void assignEvent(Board board){

        for (ImageView[] cellRow: this.getCell()){
            for (ImageView cell: cellRow){
                cell.setOnMouseClicked(e -> {

                    int[] index = getClickedIndex(cell, board);
                    int x = index[0];
                    int y = index[1];

                    if (e.getButton().equals(MouseButton.SECONDARY)){

                        if (!(board.getCell(x, y).isSelected())){
                            flag(x, y, board);
                        }


                    } else {

                        if (!(board.getCell(x,y).isFlagged())){

                            selectCell(x, y, x, y, board);

                            if (board.getBoardSize() - board.getNumMine() == Cell.getNumSelectedCell()){
                                win();
                            }

                        }                       

                    }

                });
            }
        }

    }

    private void flag(int x, int y, Board board){

        board.getCell(x, y).flag();

        if (board.getCell(x, y).isFlagged()){
            cell[y][x].setImage(board.getCell(x, y).getFlagImage());
        } else {
            cell[y][x].setImage(board.getCell(x, y).getUnselectedImage());
        }

    }

    private void selectCell(int firstX, int firstY, int x, int y, Board board){

        this.cell[y][x].setImage(board.getCell(x, y).getSelectedImage());
        board.getCell(x, y).select();

        if (board.getCell(x,y).getID().equals(CellValue.MINE) && x == firstX && y == firstY){

            lose(board);

        } else if (board.getCell(x,y).getMineCount() == 0){
                selectSurroundingCell(firstX, firstY, x, y, board);         

        }



    }

    private void selectSurroundingCell(int firstX, int firstY, int x, int y, Board board){

        for (int i = (y - 1); i <= (y + 1); i++){
            for (int j = (x - 1); j <= (x + 1); j++){

                try {

                    if (board.getCell(j, i).isSelected()){
                        continue;
                    }

                    if (i == y && j == x){
                        continue;   
                    }

                    selectCell(firstX, firstY, j, i, board);

                } catch (IndexOutOfBoundsException ex){
                    continue;
                }



            }
        }   

    }

    private int[] getClickedIndex(ImageView cell, Board board){

        int[] index = new int[2];

        for (int i = 0; i < board.getYSize(); i++){
            for (int j = 0; j < board.getXSize(); j++){



                if (cell.equals(this.cell[i][j])){
                    index[0] = j;
                    index[1] = i;
                }

            }
        }

        return index;

    }

    private void win(){
        end = true;
        win = true;
    }

    private void lose(Board board){
        displayAll(board);
        end = true;
        win = false;
    }

    public boolean isWin(){
        return win;
    }

    public boolean isEnd(){
        return end;
    }

    private void displayAll(Board board){

        for (int i = 0; i < board.getYSize(); i++){
            for (int j = 0; j < board.getXSize(); j++){

                if (!(board.getCell(j, i).isSelected())){
                    this.cell[i][j].setImage(board.getCell(j, i).getSelectedImage());
                }

            }   
        }
    }


    public ImageView getCell(int x, int y){
        return cell[y][x];
    }

    public ImageView[][] getCell(){
        return cell;
    }

    public static final int CELL_SIZE = 20;

}

Board Class (The Class that Contains the Minesweeper Board)

import java.util.Random;

public class Board {

    private Cell[][] cells;
    private Random random = new Random();
    private int numMine;

    public Board(int xSize, int ySize){
        cells = new Cell[xSize][ySize];

    }

    public void init(Difficulty difficulty){

        initEmptyCell();
        numMine = initNumMine(difficulty);
        initMine();
        initMineCount();

    }

    public void init(int numMine) throws TooMuchMineException{

        if (numMine >= ((cells.length - 1) * (cells[0].length - 1))){
            throw new TooMuchMineException();
        }

        initEmptyCell();
        this.numMine = numMine;
        initMine();
        initMineCount();
    }

    private void initEmptyCell(){

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

    private int initNumMine(Difficulty difficulty){

        switch(difficulty){
            case EASY: return getBoardSize() / EASY_FACTOR; 
            case MEDIUM: return getBoardSize() / MEDIUM_FACTOR; 
            case HARD: return getBoardSize() / HARD_FACTOR; 
            default: return 0;
        }
    }

    private void initMine(){

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

            while(true){
                Cell randomCell = cells[random.nextInt(cells.length)][random.nextInt(cells[0].length)];

                if (!(randomCell.getID().equals(CellValue.MINE))){
                    randomCell.setMine();
                    break;
                }

            }

        }
    }

    private void initMineCount(){

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

                if (cells[i][j].getID().equals(CellValue.MINE)){
                    continue;
                }

                int mineCount = 0;

                mineCount = getMineCount(j, i);

                cells[i][j].setMineCount(mineCount);

            }
        }

    }

    public Cell getCell(int x, int y){
        return cells[y][x];
    }

    public Cell[][] getCell(){
        return cells;
    }

    private int getMineCount(int x, int y){

        int mineCount = 0;

        for (int i = (y - 1); i <= (y + 1); i++){
            for (int j = (x - 1); j <= (x + 1); j++){

                if (i == y && j == x) continue;

                try {

                    if (cells[i][j].getID().equals(CellValue.MINE)){
                        mineCount++;
                    }

                } catch (IndexOutOfBoundsException ex){
                    continue;
                }

            }
        }

        return mineCount;


    }

    public int getBoardSize(){
        return getYSize() * this.getXSize();
    }

    public int getXSize(){
        return cells[0].length;
    }

    public int getYSize(){
        return cells.length;
    }

    public int getNumMine(){
        return numMine;
    }


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



}

Cell Class (The Class that Contains the code for a single Minesweeper Cell)

import javafx.scene.image.Image;


public class Cell {

    private CellValue id;
    private int mineCount;
    private boolean isSelected = false;
    private boolean isFlagged = false;

    private static int numFlag = 0;
    private static int numSelectedCell = 0; 

    public Cell(){
        this(CellValue.EMPTY);
    }

    public Cell(CellValue id){
        this.id = id;
    }

    public void setMine(){
        id = CellValue.MINE;
    }

    public CellValue getID(){
        return id;
    }

    public void select(){
        isSelected = true;

        if (isFlagged){
            flag();
        }

        numSelectedCell++;
    }

    public void flag(){
        isFlagged = !isFlagged;

        if (this.isFlagged()){
            numFlag++;
        } else {
            numFlag--;
        }

    }

    public boolean isSelected(){
        return isSelected;
    }

    public boolean isFlagged(){
        return isFlagged;
    }

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

    public void setMineCount(int mineCount){
        this.mineCount = mineCount;
    }

    public int getMineCount(){
        return mineCount;
    }

    public Image getUnselectedImage(){
        return unselected;
    }

    public Image getFlagImage(){
        return flag;
    }

    public Image getSelectedImage(){

        if (id.equals(CellValue.MINE)){
            return getMineImage();
        }

        switch (mineCount){

        case 0: return zero;
        case 1: return one;
        case 2: return two;
        case 3: return three;
        case 4: return four;
        case 5: return five;
        case 6: return six;
        case 7: return seven;
        case 8: return eight;

        default : return null;

        }
    }

    public Image getMineImage(){
        return mine;
    }

    public static int getNumSelectedCell(){
        return numSelectedCell;
    }

    public static int getNumFlag(){
        return numFlag;
    }

    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");

}

CellValue Class(Enum Class for Cell: EMPTY / MINE)

public enum CellValue {

    EMPTY, MINE;

}

Difficulty Class(Enum Class: EASY/MEDIUM/HARD)

public enum Difficulty {

    EASY, MEDIUM, HARD;

}

NumberDisplay Class (Another javafx class that create a number display, ignore this)

import java.util.ArrayList;

import javafx.scene.image.Image;
import javafx.scene.image.ImageView;
import javafx.scene.layout.HBox;


public class NumberDisplay extends HBox {

    private int number;
    private ArrayList<ImageView> ivs;

    public NumberDisplay(){
        this(0);
    }

    public NumberDisplay(int digit){
        super();
        number = 0;
        ivs = new ArrayList<ImageView>();

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

            ivs.add(new ImageView());
            ivs.get(i).setFitHeight(DISPLAY_HEIGHT);

            ivs.get(i).setFitWidth(DISPLAY_WIDTH);
            this.getChildren().add(ivs.get(i));
        }

        this.update();

    }

    public void setNumber(int number){
        this.number = number;
    }

    public int getNumber(){
        return number;
    }

    public void update(){

        int[] digits = parseNumber();

        if (number < 0){

            for (int i = 0; i < ivs.size() - 1; i++){
                setImage(ivs.get(ivs.size() - 1 - i), digits[i]);

            }

            ivs.get(0).setImage(negative);

        } else {

            for (int i = 0; i < ivs.size(); i++){
                setImage(ivs.get(ivs.size() - 1 - i), digits[i]);

            }

        }



    }

    private int[] parseNumber(){

        int[] digits = new int[ivs.size()];

        if (number >= Math.pow(10, ivs.size())){

            for (int i = 0; i < digits.length; i++){
                digits[i] = 9;
            }

        } else if (number <= -Math.pow(10, ivs.size() - 1)) {

            for (int i = 0; i < digits.length; i++){
                digits[i] = 9;
            }

        } else {

            for (int i = 0; i < digits.length; i++){
                digits[i] = (number % ((int)(Math.pow(10, i + 1)))) / (int)(Math.pow(10, i));
            }

        }

        return digits;

    }

    private void setImage(ImageView iv, int digit){

        switch (digit){

        case 1: case -1: iv.setImage(one); break;
        case 2: case -2: iv.setImage(two); break;
        case 3: case -3: iv.setImage(three); break;
        case 4: case -4: iv.setImage(four); break;
        case 5: case -5: iv.setImage(five); break;
        case 6: case -6: iv.setImage(six); break;
        case 7: case -7: iv.setImage(seven); break;
        case 8: case -8:  iv.setImage(eight); break;
        case 9: case -9: iv.setImage(nine); break;
        case 0: iv.setImage(zero); break;
        default: iv.setImage(zero); break;

        }

    }

    private static Image zero = new Image("image/digitalnumber/zero.png");
    private static Image one = new Image("image/digitalnumber/one.png");
    private static Image two = new Image("image/digitalnumber/two.png");
    private static Image three = new Image("image/digitalnumber/three.png");
    private static Image four = new Image("image/digitalnumber/four.png");
    private static Image five = new Image("image/digitalnumber/five.png");
    private static Image six = new Image("image/digitalnumber/six.png");
    private static Image seven = new Image("image/digitalnumber/seven.png");
    private static Image eight = new Image("image/digitalnumber/eight.png");
    private static Image nine = new Image("image/digitalnumber/nine.png");
    private static Image negative = new Image("image/digitalnumber/negative.png");

    private static final int DISPLAY_HEIGHT = 30;
    private static final int DISPLAY_WIDTH = 20;

}

TimeDisplay CLass (Again, another javafx class that creates a timer)

import javafx.animation.Animation;
import javafx.animation.KeyFrame;
import javafx.animation.Timeline;
import javafx.util.Duration;


public class TimeDisplay extends NumberDisplay {

    Timeline timeline;

    public TimeDisplay(int digit){
        super(digit);
        this.update();
    }

    public void start(){

        timeline = new Timeline(new KeyFrame(
                Duration.millis(1000),
                ae -> addSecond()));
        timeline.setCycleCount(Animation.INDEFINITE);
        timeline.play();
    }

    public void stop(){
        timeline.stop();
    }

    public void reset(){
        this.setNumber(0);
    }

    public int getTime(){
        return this.getNumber();
    }

    private void addSecond(){
        this.setNumber(this.getNumber() + 1);
        this.update();
    }

}

TooMuchMineExceptionClass (Custom Exception Class for My Code)

public class TooMuchMineException extends RuntimeException {

    public TooMuchMineException(){
        super();
    }

    public TooMuchMineException(String msg){
        super(msg);
    }

    public TooMuchMineException(String msg, Throwable cause){
        super(msg, cause);
    }

    public TooMuchMineException(Throwable cause){
        super(cause);
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ I'm working up a review so don't despair. I will say this though. Look into design patterns. Easiest one to clean up a few things would be the Factory method. I'll cover that in my review though. \$\endgroup\$ Commented Jul 11, 2015 at 20:21
  • \$\begingroup\$ see my edit. I added a unit test and another tip. \$\endgroup\$ Commented Jul 14, 2015 at 12:54

2 Answers 2

10
\$\begingroup\$

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 just set the values EASY, MEDIUM, and HARD to the difficulty factors.


NumberDisplay

There are multiple things wrong with these lines:

ivs.add(new ImageView());
ivs.get(i).setFitHeight(DISPLAY_HEIGHT);

ivs.get(i).setFitWidth(DISPLAY_WIDTH);
this.getChildren().add(ivs.get(i));
  1. The constant ivs.get(i)

Isn't this a little redundant and inefficient? Wouldn't it be easier to just store ivs.get(i) in a variable, and then to re-put it back into the array when you are done?

  1. ivs.get(i) == ivs.add(...)??

This may just be me, but when you add a new ImageView to the array, and then you get(i), wouldn't you be retrieving the ImageView that you just put in?

If so, don't add the ImageView to the array until you are done calling all those functions on it. Then, you can add it to the array.


The same recommendation I wrote about the other pictures applies to the ones at the end of this file.


Misc.

Write some JavaDoc.

You wrote a lot of code, and that code could get really hard to look back on unless you have documentation describing each and every method.

And, by having JavaDoc, it will be easier for other people to review your code.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ I strongly disagree with the comment about storing the result of board.getYSize(). The proposed change makes the code harder to read and most likely will not improve performance at all. Provided that the compiler can prove that the return value is loop-invariant (it can), then the JIT will do that optimization for you. And probably some other things. \$\endgroup\$
    – Emily L.
    Commented Jul 14, 2015 at 13:06
  • 1
    \$\begingroup\$ The suggestion to use an array to store the contents of one,two... in the section "Cell" is also inefficient. The switch is dense and will be compiled into a jump table by the JIT. Essentially performing the same optimization you did with one caveat, it will preserve the behaviour of returning null for undefined values rather than throwing. Using an array forces a memory allocation (although fast in java) and puts unnecessary strain on the GC where the switch does not. Unless you make the array static global but you did not. \$\endgroup\$
    – Emily L.
    Commented Jul 14, 2015 at 13:14
  • \$\begingroup\$ @EmilyL. Thanks for those tips! I had no idea that the JIT did all that optimization. I have changed my answer. \$\endgroup\$
    – SirPython
    Commented Jul 14, 2015 at 13:19
  • 2
    \$\begingroup\$ You're welcome. Compilers are pretty good today, generally you should just express your intent as clearly as possible, use the correct algorithm and think about data structures. The compiler will take care of the details. That kind of optimizations should only ever be performed after profiling measurements support the decision before and after and there is a need for them (i.e. code is too slow). \$\endgroup\$
    – Emily L.
    Commented Jul 14, 2015 at 13:25
0
\$\begingroup\$

A tip I learned from C# winforms programs is to have the entry point do as little as possible to get the program up and running. I believe it should be a core fundamental to all high level concept classes. Main is no exception, in fact it is at the highest level of your program and therefor should do nothing more than do the smallest things to get your program started. An example of what I have in mind would be more like this.

@Override
public void start(Stage stage) throws Exception{
    MainGame game = GameFactory.create(new Board(9,9), Difficulty.EASY);
    Scene scene = new Scene(GameWindowLayoutFactory.create(game));
    stage.setScene(scene);
    stage.show();
}

This code above takes very little brain power to know what it does, which is perfect for the highest level of your code. If a new person comes in and wants to fix something with the UI, then they would start drilling into the GameWindowLayoutFactory. If it's the game logic, then they start drilling into GameFactory.

This points me to my comment that I suggested to you earlier about looking into design patterns. They are easy to understand, but take some practice and understanding to know when and how to use them effectively. The factory pattern (IMO) can very easily be used too much and make it harder to understand your code. That said I would personally limit factories to be used primarily for high level code, or for when you want to have more of a Fluent feel to your code. Now even if you decided to not go with the Factories in this scenario I would recommend using more methods (with good names to them) so that it can still be read easily. I've encapsulated your start method so that it looks like this now (along with using SirPython's suggestion of in-lining when it makes sense to, and having to make a method in MainGame to get the remaining mines)

@Override
public void start(Stage stage) throws Exception {
    NumberDisplay mineCount = new NumberDisplay(DIGITS);

    MainGame mainGame = createMainGame(mineCount);

    Parent mainLayout = createLayout(mainGame, mineCount);
    startGame(stage, mainLayout);
}

Moving on to MainGame, I hate to see deep nested methods. I would rather see a method inside at most the second nest. That would turn something like this

private void assignEvent(Board board){

    for (ImageView[] cellRow: this.getCell()){
        for (ImageView cell: cellRow){
            cell.setOnMouseClicked(e -> {

                int[] index = getClickedIndex(cell, board);
                int x = index[0];
                int y = index[1];

                if (e.getButton().equals(MouseButton.SECONDARY)){

                    if (!(board.getCell(x, y).isSelected())){
                        flag(x, y, board);
                    }


                } else {

                    if (!(board.getCell(x,y).isFlagged())){

                        selectCell(x, y, x, y, board);

                        if (board.getBoardSize() - board.getNumMine() == Cell.getNumSelectedCell()){
                            win();
                        }

                    }

                }

            });
        }
    }

}

into something like this

private void assignEvent(Board board){
    for (ImageView[] cellRow: this.getCell()){
        assignEventToCellRow(board, cellRow);
    }

}

private void assignEventToCellRow(Board board, ImageView[] cellRow) {
    for (ImageView cell: cellRow){
        cell.setOnMouseClicked(getMouseEventEventHandler(board, cell));
    }
}

private EventHandler<MouseEvent> getMouseEventEventHandler(Board board, ImageView cell) {
    return e -> {
        //removed for brevity
    };
}

this method below makes me angry every time I read it

private void selectSurroundingCell(int firstX, int firstY, int x, int y, Board board){

    for (int i = (y - 1); i <= (y + 1); i++){
        for (int j = (x - 1); j <= (x + 1); j++){

            try {

                if (board.getCell(j, i).isSelected()){
                    continue;
                }

                if (i == y && j == x){
                    continue;
                }

                selectCell(firstX, firstY, j, i, board);

            } catch (IndexOutOfBoundsException ex){
                continue;
            }



        }
    }

}

WHY?!? Why would you catch a index out of bounds exception. There is no one but to blame but the programmer if that is thrown. which leads me to my next tops for discussion. Using unit tests. When you have something this "complex" having unit tests in place is a must. There are scores of papers and blogs and books out there as to why and how so I'm not going to elaborate on that. I will however show a few examples. A tip I've learned about unit tests is that they should be neat, small, and fast. To know what to write a test for take a step back and analyze what you want to write. Can it be pulled out into a class easily so it can be tested in isolation. In this case yes! However because of how this is written pulling out things into a proper class can be tricky. (because it was so tricky I moved on to something a little easier to show what I mean) NumberDisplay (and by extension TimeDisplay) is a good example of a class that does too much and can easily delegate its work to something else. What's more you can take that work and put it in a unit test. NumberDisplay does a lot of math to just display 3 numbers, to what is essentially just a number format. We can use that to our advantage thinking of it like that. Make a NumberDisplayDigitFormatter that extends NumberFormat it will have a matching test in the test folder.

NumberDisplayDigitFormatter

public class NumberDisplayDigitFormatter extends NumberFormat {
    @Override
    public StringBuffer format(double number, StringBuffer toAppendTo, FieldPosition pos) {
        return null;
    }

    @Override
    public StringBuffer format(long number, StringBuffer toAppendTo, FieldPosition pos) {
        return null;
    }

    @Override
    public Number parse(String source, ParsePosition parsePosition) {
        return null;
    }
}

NumberDisplayDigitFormatterTest

public class NumberDisplayDigitFormatterTest {

    @Test
    public void testFormat() throws Exception {

    }
}

Each line in my assert process is where I had to run the test, watch it fail, edit the NumberFormatter, and run the test to watch it pass. Final out come is this (test first)

public class NumberDisplayDigitFormatterTest {

    @Test
    public void testFormat() throws Exception {
        NumberDisplayDigitFormatter formatter = new NumberDisplayDigitFormatter();

        assertEquals(formatter.format(1), "001");
        assertEquals(formatter.format(1234), "234");
        assertEquals(formatter.format(-1), "-01");
        assertEquals(formatter.format(-123), "-23");
    }
}

public class NumberDisplayDigitFormatter extends NumberFormat {
    @Override
    public StringBuffer format(double number, StringBuffer toAppendTo, FieldPosition pos) {
        throw new IllegalStateException("Does not support formatting decimal numbers");
    }

    @Override
    public StringBuffer format(long number, StringBuffer toAppendTo, FieldPosition pos) {
        String format = "000" + Math.abs(number);
        format = format.substring(format.length()-3);
        toAppendTo.append(format);
        if(number < 0)
            toAppendTo.setCharAt(0, '-');

        return toAppendTo;
    }

    @Override
    public Number parse(String source, ParsePosition parsePosition) {
        throw new IllegalStateException("Does not support parsing numbers");
    }
}

the test runs in about 300ms, and it took me about 2 minutes to make that test. 300ms may seem like a lot, but that includes compile time, setup time and execution time. Actual time to format that is very minimal.

So now I have a string that is formatted the way you want NumberDisplay to show it. You would pass in your numberformatter in the constructor, and use setNumber which would run the number through the formatter and put up the appropriate images as needed. (I have that code on my personal laptop so maybe it could be a exercise for you) Speaking of which, before I couldn't get this working without first having to change all your images to use getClass().getResource("/image/digitalnumber/asdf.png"). I believe it is good practice to use that instead of hardcoded string literals. I even pulled out all the images into a abstract ImageResources class which loaded statically.

public abstract class ImageResources {
    public static DigitalNumberImageResources clock = new DigitalNumberImageResources();
    public static MineNumberImageResources mines = new MineNumberImageResources();

    static Image loadImage(String folder, String imageName){
        return new Image(ImageResources.class
                .getClass()
                .getResource(folder + imageName)
                .toExternalForm());
    }

    static class DigitalNumberImageResources{
        private static final String folder = "/images/digitalnumber/";
        public static Image one = loadImage(folder, "one.png");
        //...
        public static Image nine = loadImage(folder, "nine.png");
    }

    static class MineNumberImageResources {
        private static final String folder = "/images/mines/";
        public static Image one = loadImage(folder, "one.png");
        //...
        public static Image eight = loadImage(folder, "eight.png");
    }
}

and to use it it was just a matter of saying ImageResources.mines.one

So that's what I have right now. Sorry I didn't get a chance to show you the other mine class I just ran out of time.

\$\endgroup\$
2
  • \$\begingroup\$ I am using index out of bounds exception to take into account the corner cell (I know, I am abusing exception). But this seems to be the easiest way to handle this problem. Basically, I ordered each cell to look at its surrounding. However, the corner cells don't have any surrounding, and thus if their surrounding is "out of bound", then I would just continue the loop. What would be your best suggestion tackle this problem without abusing exception handling? \$\endgroup\$
    – dkurniawan
    Commented Jul 13, 2015 at 0:09
  • 1
    \$\begingroup\$ When I get a chance to show you my unit test I will show you my solution to it. However two scenarios come to mind. One is to make an extra column and row and make a empty non visible cell, or use boundary checks. Most people would choose boundary checks. \$\endgroup\$ Commented Jul 13, 2015 at 0:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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