Gem Puzzle game

Question

I started learning JavaFX a few days ago and I have just created as an exercise a simple Gem Puzzle game. The puzzle pieces can be moved if we click on them. The visual aspect of the game is very basic. The code is based on a MVC architecture.

It's not totally finished. Still, before I get any further in this exercise (shuffling pieces, checking if the puzzle is solved, creating a solver, improve visual...) I would like to get some feedback about my code from more advanced programmers to know which point I should improve in order to be a better developer. I would be grateful for any remarks and tips. Is the code clean, MVC pattern correctly used, JavaFX used wisely, are code conventions followed, and anything that would help me to improve myself.

The classes are organized as follows:

model : Board, Piece

view : GemMainGUI, Tile, GemBoard

controller : MouseController

MODEL

package games.gem.model;

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

public class Board {

    private final int boundX;
    private final int boundY;
    private List<Piece> pieces;
    private int emptyPositionX;
    private int emptyPositionY;

    public Board(int x, int y) {
        this.boundX = x;
        this.boundY = y;

        this.pieces = new ArrayList<>();

        // Create each pieces with their position in the board
        List<Integer> ids = new ArrayList<>();
        for (int i = 1; i < this.boundX * this.boundY; i++) {
            ids.add(i);
        }
        Collections.shuffle(ids);
        Iterator<Integer> it = ids.iterator();

        for (int i = 0; i < this.boundX; i++) {
            for (int j = 0; j < this.boundY; j++) {
                if (i != this.boundX - 1 || j != this.boundY - 1) this.pieces.add(new Piece(j, i, it.next()));
            }
        }

        this.emptyPositionX = this.boundX - 1;
        this.emptyPositionY = this.boundY - 1;

        this.shuffle();
    }

    public List<Piece> getPieces() {
        return this.pieces;
    }

    public void move(Piece currentPiece) {

        if (((Math.abs(currentPiece.getX() - this.emptyPositionX) <= 1) && !(Math.abs(currentPiece.getY() - this.emptyPositionY) >= 1)) ||
                (!(Math.abs(currentPiece.getX() - this.emptyPositionX) >= 1) && (Math.abs(currentPiece.getY() - this.emptyPositionY) <=1))) {
            int saveX = currentPiece.getX();
            int saveY = currentPiece.getY();

            currentPiece.setX(this.emptyPositionX);
            currentPiece.setY(this.emptyPositionY);

            this.emptyPositionX = saveX;
            this.emptyPositionY = saveY;
        }
    }

    private boolean isSolvable() {
        // Count the number of inversion
        Piece currentPiece;
        int inversion;
        int totalInversion = 0;

        for (int i = 0; i < this.pieces.size() - 1; i++) {
            currentPiece = this.pieces.get(i);
            inversion = 0;
            for (int j = i + 1; j < this.pieces.size(); j++) {
                if (currentPiece.getId() > this.pieces.get(j).getId()) {
                    inversion++;
                }
            }
            totalInversion += inversion;
        }

        if (this.boundX % 2 != 0) {
            return totalInversion % 2 == 0;
        } else {
            if ((this.boundY - this.emptyPositionY) % 2 == 0) {
                return totalInversion % 2 != 0;
            } else {
                return totalInversion % 2 == 0;
            }
        }
    }

    public void shuffle() {
        Collections.shuffle(this.pieces);
        System.out.println(this.pieces);

        if (!this.isSolvable()) {
            Collections.swap(this.pieces, 0, 1);
        }
    }
}

package games.gem.model;

public class Piece {
    private final int id;
    private int x;
    private int y;

    public Piece(int x, int y, int id) {
        this.id = id;
        this.x = x;
        this.y = y;
    }

    public int getId() {
        return this.id;
    }

    public int getX() {
        return this.x;
    }

    public int getY() {
        return this.y;
    }

    public void setX(int x) {
        this.x = x;
    }

    public void setY(int y) {
        this.y = y;
    }

    @Override
    public String toString() {
        return String.valueOf(this.id);
    }
}

VIEW

import games.gem.controller.MouseController;
import games.gem.model.Board;
import javafx.animation.AnimationTimer;
import javafx.application.Application;
import javafx.scene.Group;
import javafx.scene.Scene;
import javafx.scene.canvas.Canvas;
import javafx.scene.canvas.GraphicsContext;
import javafx.stage.Stage;

public class GemMainGUI extends Application {

    public static final int WIDTH = 400;
    public static final int GRID_SIZE = 4;

    private GemBoard container;

    @Override
    public void start(Stage stage) throws Exception {
        Group root = new Group();
        Scene scene = new Scene(root, WIDTH, WIDTH);

        Canvas canvas = new Canvas(WIDTH, WIDTH);
        root.getChildren().add(canvas);
        GraphicsContext gc = canvas.getGraphicsContext2D();

        Board model = new Board(GRID_SIZE, GRID_SIZE);
        this.container = new GemBoard(model);
        scene.setOnMouseClicked(new MouseController(model, this.container));

        stage.setTitle("Gem game");
        stage.setScene(scene);
        this.run(gc);
        stage.show();
    }

    private void run(GraphicsContext gc) {
        new AnimationTimer() {

            @Override
            public void handle(long l) {
                // Erase former drawings
                gc.clearRect(0, 0, WIDTH, WIDTH);

                // Drawing tiles
                GemMainGUI.this.container.draw(gc);

            }
        }.start();
    }

}

package games.gem.view;

import games.gem.model.Board;
import games.gem.model.Piece;
import javafx.scene.canvas.GraphicsContext;

import java.util.ArrayList;
import java.util.List;

public class GemBoard {

    private List<Tile> tiles;

    public GemBoard(Board board) {
        this.tiles = new ArrayList<>();

        for (Piece p : board.getPieces()) {
            tiles.add(new Tile(p));
        }
    }

    public List<Tile> getTiles() {
        return this.tiles;
    }

    public void draw(GraphicsContext gc) {
        for (Tile tile : this.tiles) {
            tile.draw(gc);
        }
    }

}

package games.gem.view;

import games.gem.model.Piece;
import javafx.scene.canvas.GraphicsContext;
import javafx.scene.paint.Color;

public class Tile {

    private Piece piece;
    private static int width = GemMainGUI.WIDTH / GemMainGUI.GRID_SIZE;

    public Tile(Piece piece) {
        this.piece = piece;
    }

    public Piece getPiece() {
        return this.piece;
    }

    public boolean contains(double x, double y) {
        boolean betweenX = this.piece.getX() * width <= x && x <= this.piece.getX() * width + width;
        boolean betweenY = this.piece.getY() * width <= y && y <= this.piece.getY() * width + width;
        return betweenX && betweenY;
    }

    public void draw(GraphicsContext gc) {
        if (this.piece == null) {
            return;
        }
        int x = this.piece.getX() * width;
        int y = this.piece.getY() * width;

        gc.setFill(Color.BLUE);
        gc.fillRect(x, y, width, width);
        gc.setStroke(Color.BLACK);
        gc.strokeRect(x, y, width, width);

        gc.setFill(Color.BLACK);
        gc.fillText(String.valueOf(this.piece.getId()), x + width / 2, y + width / 2);
    }

}

CONTROLLER

package games.gem.controller;

import games.gem.model.Board;
import games.gem.view.GemBoard;
import games.gem.view.Tile;
import javafx.event.EventHandler;
import javafx.scene.input.MouseEvent;

public class MouseController implements EventHandler<MouseEvent> {

    private Board model;
    private GemBoard view;

    public MouseController(Board model, GemBoard view) {
        this.model = model;
        this.view = view;
    }

    @Override
    public void handle(MouseEvent mouseEvent) {
        for (Tile currentTile : this.view.getTiles()) {
            if (currentTile.contains(mouseEvent.getX(), mouseEvent.getY())) {
//                System.out.println(currentTile.getPiece().getId());
                this.model.move(currentTile.getPiece());
            }
        }
    }
}

Answer 1

You should avoid complex boolean expressions in if statements like you used here:

if (((Math.abs(currentPiece.getX() - this.emptyPositionX) <= 1) && !(Math.abs(currentPiece.getY() - this.emptyPositionY) >= 1)) ||
                (!(Math.abs(currentPiece.getX() - this.emptyPositionX) >= 1) && (Math.abs(currentPiece.getY() - this.emptyPositionY) <=1)))

You should have a private boolean method:

if(isSomeStatmentValid())
{
    //do something
}

private boolean isSomeStatmentValid()
{
    (!(Math.abs(currentPiece.getX() - this.emptyPositionX) >= 1)
    &&(Math.abs(currentPiece.getY() - this.emptyPositionY) <= 1))
}

This is more clear to read.

As I said in the comment, I suggest you use GitHub if you don't already use it, and than in the Marketplace page you should subscribe to CodeBeat, Codacy and BetterCodeHub apps for automated code review. It is free of charge for public repositories. It is very helpful.

And you should take look at Java Code Conventions.