Trading Card Game Prototype GUI with JavaFX

Most of my programming experience is with Objective-C, but I have recently started learning Java and JavaFX to build the user interface for a Trading Card Game. This is the most Java I have ever written, and I am completely new to JavaFX as well, so I am sure that there are lots of problems with this code.

I mostly used Scene Builder to layout the scene itself, but I manually edited the FXMLDocument file also to accomplish everything. I am not at all sure that I am doing things the right way here. I have tried to make the code as readable as possible, but many objects are created programmatically and not with Scene Builder so I am not sure that everything will make sense to the reader.

I should note that I tried to use only Panes for containing the information and for rendering it, but I had issues with the scene not updating properly. To solve this problem, I created Lists that contain the Groups that the scene will actually render. This separates the rendering from the creation of the objects being rendered. I think this is a good approach, but I may be wrong.

What I am looking for are best practices for Java, as well as any Java features I may not be using that I should be. Also the readability of the code is always very important to me. Don't worry about being harsh or critical, you won't hurt my feelings.

Here is a sample image of the GUI in action. I should note that it is a prototype for the purpose of testing the game as quickly as possible, so it is ugly:

First things first, this is the class that is the launching point for the program.

JavaFXGame.java

```
package com.cardshifter.fx;

import javafx.application.Application;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.stage.Stage;

//This class just loads the FXML document which initializes its DocumentController

public class JavaFXGame extends Application {

@Override
public void start(Stage stage) throws Exception {

Your JavaFXGame class can use javadoc

/**
* This class just loads the FXML document which initializes its DocumentController
*/
public class JavaFXGame extends Application {

The start method can be shortened and cleaned up:

@Override
public void start(Stage stage) throws Exception {     
    Parent root = FXMLLoader.load(getClass().getResource("FXMLDocument.fxml"));
    Scene scene = new Scene(root);
    stage.setScene(scene);
    stage.show();
}

Even though the specification from the Application class specifies that throws Exception, you can declare it as the following:

public void start(Stage stage) throws IOException {

Specifying the exception more is allowed in an overridden method, it is also allowed to get rid of it completely (although that's not necessary here as that would require a try-catch inside the method). This doesn't break the contract.

You have a comment for main which looks a bit like Javadoc, but please use real javadoc instead:

/**
 * Main method
 * @param args the command line arguments
 */
public static void main(String[] args) {
    launch(args);
}

Taking a look at just the first part of your other class and we can see that it is filled with unnecessary comments and other fluff:

//INITIAL GAME SETUP
private final CardshifterAI opponent = new CompleteIdiot();
//need a forward declaration so that this is  global to the class
private Game game;
//hack to make the buttons work properly
private boolean gameHasStarted = false;
//I think this is a public constructor, this code initializes the Game
@FXML
Pane anchorPane;
public FXMLGameController() throws Exception {
    this.initializeGame();
}

You have several unnecessary comments here (although I understand some of them are because you are new to the language).

private final CardshifterAI opponent = new CompleteIdiot();
private Game game;
private boolean gameHasStarted = false;

@FXML
Pane anchorPane;

public FXMLGameController() throws Exception {
    this.initializeGame();
}

You seem to be very inconsistent in using private on the fields. Remember to use private whenever possible.

It is standard practice in Java to keep all class-variables (a.k.a. fields) at the top of the class together. This makes a typical Java class have the order 1. fields 2. constructor 3. methods. 4. static-methods (where you place the static-methods can differ, but they should always be put together whenever there are any).

On some occasions you seem to be doing this:

if (gameHasStarted == false) {
        // a lot of code...
    }
    // end of method

In such cases I personally prefer to return early to reduce indentation.

Also, you can use !gameHasStarted instead of comparing with == false. ! is the logical not-operator in Java.

if (!gameHasStarted) {
        return;
    }
    // a lot of code...

This also applies to the handleTurnButtonAction method, but there you can simply remove == true as comparing with true is the same as returning the boolean directly.

startGameButtonAction and newGameButtonAction are way too similar to be two separate methods. Consider calling one from the other. Also ask yourself this question: Is startGame button really needed? And/Or is the call to initializeGame in the constructor really needed? Consider making it start a new game automatically on startup.

Either way, there's code duplication there that is easy to get rid of. (Extract function is one way, but as said, consider changing the user experience)

The Java conventions for spacing is like this:

while (currentCard < numCards) {
        ...
        cardGroup.setTranslateX(currentCard * cardWidth * 1.05); // note that a set of parenthesis can be removed here

        Rectangle cardBack = new Rectangle(0, 0, cardWidth, paneHeight);

createOpponentBattlefield, createPlayerBattlefield and createPlayerHand is way too similar. As the game model seems to use a Zone class, perhaps you should make a ZoneView class that is responsible for displaying a Zone.

There is also some duplicate code in markTargets, but I believe that can be handled by using a ZoneView class. Then you could call markTargets on each ZoneView.

Finally, I personally prefer a little bit more space when there is a whole bunch of annotations and fields together. Such as the following:

@FXML
Label opponentCurrentMana;

@FXML
Label opponentTotalMana;

@FXML
Label opponentScrap;

@FXML
Label playerLife;

@FXML
Label playerCurrentMana;

Alternatively, you can write the annotation on the same line as the field type and name.

@FXML Label opponentCurrentMana;
@FXML Label opponentTotalMana;
@FXML Label opponentScrap;
@FXML Label playerLife;
@FXML Label playerCurrentMana;

P.S. Don't forget private.

StackExchange Code Review Q#62341, answer score: 7