15
\$\begingroup\$

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: game image 01

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 {     

        //stage.setTitle("title");

        Parent root = FXMLLoader.load(getClass().getResource("FXMLDocument.fxml"));

        Scene scene = new Scene(root);

        stage.setScene(scene);
        //stage.centerOnScreen();
        stage.show();
    }

     // @param args the command line arguments

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

This is the meat of the program, where the user interface is controlled and commands are sent to the Game:

FXMLGameController.java

package com.cardshifter.fx;

import java.io.File;
import java.io.FileInputStream;
import java.io.InputStream;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;
import java.util.ResourceBundle;

import javafx.event.ActionEvent; 
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.Group;
import javafx.scene.Node;
import javafx.scene.control.Label;
import javafx.scene.layout.Pane;
import javafx.scene.paint.Color;
import javafx.scene.shape.QuadCurve;
import javafx.scene.shape.Rectangle;

import org.luaj.vm2.lib.jse.CoerceLuaToJava;

import com.cardshifter.ai.CardshifterAI;
import com.cardshifter.ai.CompleteIdiot;
import com.cardshifter.core.Card;
import com.cardshifter.core.Game;
import com.cardshifter.core.Player;
import com.cardshifter.core.Targetable;
import com.cardshifter.core.Zone;
import com.cardshifter.core.actions.TargetAction;
import com.cardshifter.core.actions.UsableAction;
import com.cardshifter.core.console.CommandLineOptions;

public class FXMLGameController implements Initializable {

    //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();
    }
    private void initializeGame() throws Exception {
        CommandLineOptions options = new CommandLineOptions();
        InputStream file = options.getScript() == null ? Game.class.getResourceAsStream("start.lua") : new FileInputStream(new File(options.getScript()));
        game = new Game(file, options.getRandom());
    }
    //GAME START and NEW GAME
    @FXML
    private Label startGameLabel;
    @FXML
    private void startGameButtonAction(ActionEvent event) {
        if (gameHasStarted == false) {
            startGameLabel.setText("Starting Game");
            game.getEvents().startGame(game);
            gameHasStarted = true;
            turnLabel.setText(String.format("Turn Number %d", game.getTurnNumber()));

            this.playerBattlefieldData = new ArrayList<>();
            this.opponentBattlefieldData = new ArrayList<>();
            this.playerHandData = new ArrayList<>();
            this.opponentHandData = new ArrayList<>();

            this.createData();
            this.render();
        }
    }
    @FXML
    private void newGameButtonAction(ActionEvent event) throws Exception {
        this.initializeGame();

        startGameLabel.setText("Starting Game");
        game.getEvents().startGame(game);
        gameHasStarted = true;
        turnLabel.setText(String.format("Turn Number %d", game.getTurnNumber()));

        this.playerBattlefieldData = new ArrayList<>();
        this.opponentBattlefieldData = new ArrayList<>();
        this.playerHandData = new ArrayList<>();
        this.opponentHandData = new ArrayList<>();

        this.createData();
        this.render();
    }

    //TODO: Create a fixed time step and render in that
    //RENDER - eventually change this to an Update method
    public void render() {
        //this is a hack to make the buttons disappear when the choice is made
        //will not work with a fixed time step
        Node choiceBoxPane = anchorPane.lookup("#choiceBoxPane");
        anchorPane.getChildren().remove(choiceBoxPane);

        this.renderHands();
        this.renderBattlefields();

        this.updateGameLabels();
    }

    //RENDER HANDS
    @FXML
    private Pane opponentHandPane;
    @FXML
    private Pane playerHandPane;
    private void renderHands() {
        this.renderOpponentHand();
        this.renderPlayerHand();
    }
    private void renderOpponentHand() {
        opponentHandPane.getChildren().clear();
        opponentHandPane.getChildren().addAll(opponentHandData);
    }
    private void renderPlayerHand() {
        playerHandPane.getChildren().clear();
        playerHandPane.getChildren().addAll(playerHandData);
    }

    //RENDER BATTLEFIELDS
    @FXML
    Pane opponentBattlefieldPane;
    @FXML
    Pane playerBattlefieldPane;
    private void renderBattlefields() {
        this.renderOpponentBattlefield();
        this.renderPlayerBattlefield();
    }
    private void renderOpponentBattlefield() {
        opponentBattlefieldPane.getChildren().clear();
        opponentBattlefieldPane.getChildren().addAll(opponentBattlefieldData);
    }
    private void renderPlayerBattlefield() {
        playerBattlefieldPane.getChildren().clear();
        playerBattlefieldPane.getChildren().addAll(playerBattlefieldData);
    }

    //Called at the end of turn, and when a card is played
    public void createData() {
        this.createHands();
        this.createBattlefields();
    }

    //CREATE HANDS
    private void createHands() {
        this.createOpponentHand();
        this.createPlayerHand();
    }
    //CREATE OPPONENT CARD BACKS
    private List<Group> opponentHandData;
    private void createOpponentHand() {
        opponentHandData.clear();

        //Opponent cards are rendered differently because the faces are not visible
        double paneHeight = opponentHandPane.getHeight();
        double paneWidth = opponentHandPane.getWidth();

        int numCards = this.getOpponentCardCount();
        int maxCards = Math.max(numCards, 8);
        double cardWidth = paneWidth / maxCards;

        int currentCard = 0;
        while(currentCard < numCards) {
            Group cardGroup = new Group();
            cardGroup.setTranslateX(currentCard * (cardWidth*1.05)); //1.05 so there is space between cards
            opponentHandData.add(cardGroup);

            Rectangle cardBack = new Rectangle(0,0,cardWidth,paneHeight);
            cardBack.setFill(Color.AQUAMARINE);
            cardGroup.getChildren().add(cardBack);

            currentCard++;
        }
    }
    private int getOpponentCardCount() {
        Player player = game.getLastPlayer(); 
        Zone hand = (Zone)CoerceLuaToJava.coerce(player.data.get("hand"), Zone.class);
        List<Card> cardsInHand = hand.getCards();
        return cardsInHand.size();
    }
    //CREATE PLAYER HAND
    private List<Group> playerHandData;
    private void createPlayerHand() {
        playerHandData.clear();

        List<Card> cardsInHand = this.getCurrentPlayerHand();
        int numCards = cardsInHand.size();

        int cardIndex = 0;
        for (Card card : cardsInHand) {
            CardNode cardNode = new CardNode(playerHandPane, numCards, "testName", card, this);
            Group cardGroup = cardNode.getCardGroup();
            cardGroup.setAutoSizeChildren(true); //NEW
            cardGroup.setId(String.format("card%d", card.getId()));
            cardGroup.setTranslateX(cardIndex * cardNode.getWidth());
            playerHandData.add(cardGroup);

            cardIndex++;
        }
    }
    private List<Card> getCurrentPlayerHand() {
        Player player = game.getFirstPlayer(); 
        Zone hand = (Zone)CoerceLuaToJava.coerce(player.data.get("hand"), Zone.class);
        return hand.getCards();
    }

    //CREATE BATTLEFIELDS
    private void createBattlefields() {
        this.createOpponentBattlefield();
        this.createPlayerBattlefield();
    }
    //CREATE OPPONENT BATTLEFIELD
    private List<Group> opponentBattlefieldData;
    private void createOpponentBattlefield() {
        opponentBattlefieldData.clear();

        List<Card> cardsInBattlefield = this.getBattlefield(game.getLastPlayer());
        int numCards = cardsInBattlefield.size();

        int cardIndex = 0;
        for (Card card : cardsInBattlefield) {
            CardNodeBattlefield cardNode = new CardNodeBattlefield(opponentBattlefieldPane, numCards, "testName", card, this, false);
            Group cardGroup = cardNode.getCardGroup();
            cardGroup.setAutoSizeChildren(true); //NEW
            cardGroup.setId(String.format("card%d", card.getId()));
            cardGroup.setTranslateX(cardIndex * cardNode.getWidth());
            opponentBattlefieldData.add(cardGroup);

            cardIndex++;
        } 
    }
    //CREATE PLAYER BATTLEFIELD
    private List<Group> playerBattlefieldData;
    private void createPlayerBattlefield() {
        playerBattlefieldData.clear();

        List<Card> cardsInBattlefield = this.getBattlefield(game.getFirstPlayer());
        int numCards = cardsInBattlefield.size();

        int cardIndex = 0;
        for (Card card : cardsInBattlefield) {
            CardNodeBattlefield cardNode = new CardNodeBattlefield(playerBattlefieldPane, numCards, "testName", card, this, true);
            Group cardGroup = cardNode.getCardGroup();
            cardGroup.setAutoSizeChildren(true); //NEW
            cardGroup.setId(String.format("card%d", card.getId()));
            cardGroup.setTranslateX(cardIndex * cardNode.getWidth());
            playerBattlefieldData.add(cardGroup);

            cardIndex++;
        } 
    }
    private List<Card> getBattlefield(Player player) {
        Zone battlefield = (Zone)CoerceLuaToJava.coerce(player.data.get("battlefield"), Zone.class);
        return battlefield.getCards();
    }

    //END TURN LABEL
    @FXML
    private Label turnLabel;
    //ADVANCE TURNS
    @FXML
    private void handleTurnButtonAction(ActionEvent event) {
        if (gameHasStarted == true) {
            game.nextTurn();

            //This is the AI doing the turn action
            while (game.getCurrentPlayer() == game.getLastPlayer()) {
                UsableAction action = opponent.getAction(game.getCurrentPlayer());
                if (action == null) {
                    System.out.println("Warning: Opponent did not properly end turn");
                    break;
                }
                action.perform();
            }

            turnLabel.setText(String.format("Turn Number %d", game.getTurnNumber()));

            //reload data at the start of a new turn
            this.createData();
            this.render();
        }
    }

    //CHOICE BOX PANE
    public void buildChoiceBoxPane(Card card, List<UsableAction> actionList) {
        Pane choiceBoxPane = new Pane();
        choiceBoxPane.setPrefHeight(367);
        choiceBoxPane.setPrefWidth(550);
        choiceBoxPane.setTranslateX(326);
        choiceBoxPane.setTranslateY(150);
        choiceBoxPane.setId("choiceBoxPane");

        choiceBoxPane.getChildren().clear();

        int numChoices  = actionList.size();
        double paneHeight = choiceBoxPane.getPrefHeight();
        double paneWidth = choiceBoxPane.getPrefWidth();
        double choiceBoxWidth = paneWidth / numChoices;

        int actionIndex = 0;
        for(UsableAction action : actionList) {
            ChoiceBoxNode choiceBox = new ChoiceBoxNode(choiceBoxWidth, paneHeight, "testName", action, this);
            Group choiceBoxGroup = choiceBox.getChoiceBoxGroup();
            choiceBoxGroup.setTranslateX(actionIndex * choiceBoxWidth);
            choiceBoxPane.getChildren().add(choiceBox.getChoiceBoxGroup());

            actionIndex++;
        }

        anchorPane.getChildren().add(choiceBoxPane);
    }

    //TARGETING
    public TargetAction nextAction;
    public void performNextAction(Targetable target) {
        nextAction.setTarget(target);
        nextAction.perform();
        this.createData();
        this.render();
    }
    public void markTargets(List<Card> targets) {
        List<Node> cardsInPlayerBattlefield = playerBattlefieldPane.getChildren();
        List<Node> cardsInOpponentBattlefield = opponentBattlefieldPane.getChildren();
        for (Card target : targets) {
            for(Node node : cardsInPlayerBattlefield) {
                if(node.getId().equals(String.format("card%d",target.getId())) == true) {
                    CardNodeBattlefield actionNode = (CardNodeBattlefield)node;
                    actionNode.createTargetButton();
                }
            }
            for(Node node : cardsInOpponentBattlefield) {
                if(node.getId().equals(String.format("card%d",target.getId())) == true) {
                    CardNodeBattlefield actionNode = (CardNodeBattlefield)node;
                    actionNode.createTargetButton();
                }
            }
        }
    }

    //GAME STATE LABELS
    @FXML
    Label opponentLife;
    @FXML
    Label opponentCurrentMana;
    @FXML
    Label opponentTotalMana;
    @FXML
    Label opponentScrap;
    @FXML
    Label playerLife;
    @FXML
    Label playerCurrentMana;
    @FXML
    Label playerTotalMana;
    @FXML
    Label playerScrap;
    private void updateGameLabels() {
        Player opponent = game.getLastPlayer();
        opponentLife.setText(String.format("%d",opponent.data.get("life").toint()));
        opponentCurrentMana.setText(String.format("%d",opponent.data.get("mana").toint()));
        opponentTotalMana.setText(String.format("%d",opponent.data.get("manaMax").toint()));
        opponentScrap.setText(String.format("%d",opponent.data.get("scrap").toint()));

        Player player = game.getFirstPlayer();
        playerLife.setText(String.format("%d", player.data.get("life").toint()));
        playerCurrentMana.setText(String.format("%d", player.data.get("mana").toint()));
        playerTotalMana.setText(String.format("%d", player.data.get("manaMax").toint()));
        playerScrap.setText(String.format("%d", player.data.get("scrap").toint()));
    }

    //BOILERPLATE
    @Override
    public void initialize(URL url, ResourceBundle rb) {
        // TODO
    }       

    //NOT YET USED
    @FXML
    private QuadCurve handGuide;
}

Finally, this is the FXML file that controls the layout for all the objects that are not created programmatically:

FXMLDocument.fxml

<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.text.*?>
<?import javafx.scene.shape.*?>
<?import java.lang.*?>
<?import java.util.*?>
<?import javafx.scene.*?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<AnchorPane fx:id="anchorPane" maxHeight="600" maxWidth="1200" minHeight="600" minWidth="1200" prefHeight="600.0" prefWidth="1200.0" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="com.cardshifter.fx.FXMLGameController">
    <children>

        <!--  Buttons  -->
        <Button fx:id="button" layoutX="1098.0" layoutY="530.0" onAction="#startGameButtonAction" text="Start Game" />
        <Label fx:id="startGameLabel" layoutX="1098.0" layoutY="564.0" minHeight="16" minWidth="69" />

        <Button layoutX="1105.0" layoutY="467.0" mnemonicParsing="false" onAction="#handleTurnButtonAction" text="End Turn" />
        <Label fx:id="turnLabel" layoutX="1091.0" layoutY="500.0" text="Turn Number" />

        <Button fx:id="newGameButton" layoutX="17.0" layoutY="24.0" mnemonicParsing="false" onAction="#newGameButtonAction" text="New Game" />

        <!--  Hands  -->
        <Pane fx:id="playerHandPane" layoutX="14.0" layoutY="393.0" prefHeight="200.0" prefWidth="1066.0">
            <children>
                <QuadCurve fx:id="handGuide" controlY="-100.0" endX="100.0" fill="DODGERBLUE" layoutX="480.0" layoutY="200.0" opacity="0.0" startX="-100.0" stroke="BLACK" strokeType="INSIDE" />
            </children>
        </Pane>

        <Pane fx:id="opponentHandPane" layoutX="153.0" layoutY="6.0" prefHeight="61.0" prefWidth="895.0" />

        <!--  Battlefields  -->
        <Pane fx:id="playerBattlefieldPane" layoutX="47.0" layoutY="237.0" prefHeight="98.0" prefWidth="1001.0" />
        <Pane fx:id="opponentBattlefieldPane" layoutX="47.0" layoutY="107.0" prefHeight="98.0" prefWidth="1001.0" />

        <!--  Game State Labels  -->
        <Label layoutX="1111.0" layoutY="29.0" text="Opponent" />
        <Label layoutX="1133.0" layoutY="51.0" text="Life" />
        <Label layoutX="1101.0" layoutY="91.0" text="Current Mana" />
        <Label layoutX="1107.0" layoutY="131.0" text="Total Mana" />
        <Label fx:id="opponentLife" layoutX="1133.0" layoutY="67.0" prefHeight="16.0" prefWidth="35.0" text="#" textAlignment="CENTER" />
        <Label fx:id="opponentCurrentMana" layoutX="1140.0" layoutY="107.0" text="#" />
        <Label fx:id="opponentTotalMana" layoutX="1140.0" layoutY="148.0" text="#" />
        <Label layoutX="1126.0" layoutY="263.0" text="Player" />
        <Label layoutX="1133.0" layoutY="292.0" text="Life" />
        <Label fx:id="playerLife" layoutX="1138.0" layoutY="308.0" text="#" />
        <Label layoutX="1103.0" layoutY="327.0" text="Current Mana" />
        <Label fx:id="playerCurrentMana" layoutX="1140.0" layoutY="343.0" text="#" />
        <Label layoutX="1111.0" layoutY="366.0" text="Total Mana" />
        <Label fx:id="playerTotalMana" layoutX="1141.0" layoutY="385.0" text="#" />
        <Label layoutX="1127.0" layoutY="171.0" text="Scrap" />
        <Label layoutX="1129.0" layoutY="407.0" text="Scrap" />
        <Label fx:id="playerScrap" layoutX="1141.0" layoutY="423.0" text="#" />
        <Label fx:id="opponentScrap" layoutX="1140.0" layoutY="187.0" text="#" />

    </children>
</AnchorPane>
\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

Command line parsing

I suppose this has some unimplemented functionality:

CommandLineOptions options = new CommandLineOptions();
InputStream file = options.getScript() == null ? Game.class.getResourceAsStream("start.lua") : new FileInputStream(new  File(options.getScript()));
game = new Game(file, options.getRandom());

I would guess that options.getScript() is always null, unless CommandLineOptions has some magic in it.

The second line is too long. Having to scroll to the far right makes this hard to read.

Constant strings like "start.lua" should not be buried in the code, better to move them to the top into static constants. But actually, this bit doesn't belong to the game class.

Separation of responsibilities

Is it really the game's responsibility to deal with IO errors during startup? Not really. It's the responsibility of the launcher of the game. Move the all the launching (command line parsing, configuration) related stuff outside to a dedicated helper class.

Duplication

You really have too much duplicated logic. For example:

private void renderOpponentHand() {
    opponentHandPane.getChildren().clear();
    opponentHandPane.getChildren().addAll(opponentHandData);
}
private void renderPlayerHand() {
    playerHandPane.getChildren().clear();
    playerHandPane.getChildren().addAll(playerHandData);
}

and

private void renderOpponentBattlefield() {
    opponentBattlefieldPane.getChildren().clear();
    opponentBattlefieldPane.getChildren().addAll(opponentBattlefieldData);
}
private void renderPlayerBattlefield() {
    playerBattlefieldPane.getChildren().clear();
    playerBattlefieldPane.getChildren().addAll(playerBattlefieldData);
}

These call for a helper:

private void replacePaneData(Pane pane, List<Group> data) {
    pane.getChildren().clear();
    pane.getChildren().addAll(data);
}

This is just an example. Take a careful look through the entire code and make sure you extract all the duplicated elements to helper methods.

Declaration order

At first glance I thought this code won't compile, because early on, I saw these initializations:

this.playerBattlefieldData = new ArrayList<>();
this.opponentBattlefieldData = new ArrayList<>();
this.playerHandData = new ArrayList<>();
this.opponentHandData = new ArrayList<>();

... but not the variable declarations... I had to search through your code, and these variables are declared at various different places. This is confusing, and against the standard. This is the recommended declaration order:

  1. Class (static) variables. First the public class variables, then the protected, then package level (no access modifier), and then the private.
  2. Instance variables. First the public class variables, then the protected, then package level (no access modifier), and then the private.
  3. Constructors
  4. Methods

For more details, see: Checkstyle declaration order docs.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Excellent answer overall. The CommandLineOptions thing is because bazola copied my code that was console-based, where I used a 3rd party library to populate the contents from the String[] args. Bazola just needs to learn that he shouldn't do everything the same as I do :) \$\endgroup\$ Commented Sep 17, 2014 at 12:03

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.