2D board game: good Model part?

First time writing a big project in OOP. I am quite used to scientific programming but not to OOP, and even less to building GUIs. I am writing a 2D board game: the player can move on a map from tile to tile, meet Helpers and Enemies, and win a Trophy in the end.

I used the MVC pattern and the Model parts consist of the following classes: Board, Tile, Player, Opponent (abstract), Enemy, Helper, Trophy, Position, and HighScoreManager.

Putting them all here would be too much I guess, so here are the "core" files for the Model part:

Board.java

```
package model;

import java.util.Observable;

public class Board extends Observable {

// VARIABLES

private Player player;
private Tile[][] grid = new Tile[WIDTH][HEIGHT];
private Trophy trophy;
private Position activePosition = initialPosition;
private boolean gameFinished = false;
private HighScoreManager highScoreManager = new HighScoreManager();
// constants
static final int WIDTH = 10; // in number of tiles
static final int HEIGHT = 10; // in number of tiles
static final int TOTAL_NUMBER_OF_ENEMIES = 4;
static final int TOTAL_NUMBER_OF_HELPERS = 4;
// user input
int DIFFICULTY_LEVEL = 1;
// initial values
static final int initialXPosition = 0;
static final int initialYPosition = 0;
static final Position initialPosition = new Position(initialXPosition, initialYPosition);
static final int xTrophy = (int) (WIDTH*0.75);
static final int yTrophy = (int) (HEIGHT*0.75);

// CONSTRUCTOR

// METHODS

public void initBoard() {

player = new Player(initialPosition, DIFFICULTY_LEVEL);
trophy = new Trophy();
activePosition = initialPosition;
gameFinished = false;
highScoreManager.setHighScoreValue();

int numberOfHelpers = 0;
int numberOfEnemies = 0;

// create all the tiles
for (int i=0; i<WIDTH; i++) {
for (int j=0; j<HEIGHT;

Things that I liked

• Separation of Code and UI.

• Clear naming of methods and variables. I didn't need any documentation with it, the names were enough.

Things that I didn't like

• Why? Why would you use this everywhere? It is not required and reduces readibility.

• No need to label things like // VARIABLES, // CONSTRUCTOR. If you are using an IDE like Eclipse use Ctrl + O, you can see all the methods, constructors and what not.

Things that you should include in your project

• More description about the game in README.md.

• LICENSE, the code over here is CC-3.0 licensed, but you haven't mentioned any in your GitHub project.

Some Suggestions

• Use of Negation

Use negation to reduce the indentation and increase readibility. (Though it might be a little confusing at first)


Old onKeyPressed()

if (!board.getGameFinished()) { // keep moving only is game not finished
    ...
    if (direction!="") {
        destination = board.computeDestination(direction);
        ...
    }
}

Changed onKeyPressed()

if (board.getGameFinished())
    return;
...
if (direction == "")
    return;
...
destination = board.computeDestination(direction);
board.makeMove(destination);

• Use of Switch-Case statement

Avoid using MAGIC NUMBERS.


Usage of numbers like -1, 0, 1. What do these numbers mean? Maybe you can write comment and would be able to understand. But after 2-3 months you wouldn't be able to figure out their meaning.

switch (box.getChoice()) { // take action depending on user's answer to 
    case -1: //close
        break;
    case 0: //new game
        board.initBoard(); 
        break;
    case 1: //high scores
        String highScore = board.getHighScoreManager().getHighScore();
        gui.showHighScore(highScore);
        break;
    case 2: //close
        gui.askExitConfirmation();
        break;
    default:
        ;
}

Instead, make them private static final variables and use. Or better yet make an enum out of them.

public enum EndOfGame {
    CLOSE_1(-1), NEW_GAME(0), HIGH_SCORES(1), CLOSE_2(2);
    private int value;
    private EndOfGame(int value) {
        this.value = value;
    }
    public int getValue() {
        return value;
    }
    public static EndOfGame fromValue(int value) {
        for (EndOfGame endOfGame : EndOfGame.values()) {
            if (endOfGame.getValue() == value)
                return endOfGame;
        }
        return null;
    }
}

and use it for switch-case as -

switch (EndOfGame.fromValue(box.getChoice())) {
case CLOSE_1:
    break;
case NEW_GAME:
    board.initBoard();
    break;
case HIGH_SCORES:
    String highScore = board.getHighScoreManager().getHighScore();
    gui.showHighScore(highScore);
    break;
case CLOSE_2:
    gui.askExitConfirmation();
    break;
default:
    break;
}

• Comments and Indentation

Ctrl + Shitf + F is your friend. Use it often, it will format your code for you.


Don't write code like this with comments describing every variable

public EndOfGameDialogBox(JPanel parentPane) {
    choice = JOptionPane.showOptionDialog(  parentPane, //parent pane
        message,
        title,
        JOptionPane.YES_NO_CANCEL_OPTION, //type of options
        JOptionPane.QUESTION_MESSAGE, //type of message
        null, //icon
        options, //list of buttons
        options[0]); //default focus on first button
}

Instead, attach javadoc to your project, and you will be able to see the description of all the method arguments.

• UI code on UI Thread

Go through the description about running the code on EDT as described here.

StackExchange Code Review Q#75456, answer score: 3