Better dependency between controllers and fxml files

[lyna lola]

Hello everyone,
I am a beginner in JavaFX and I 'am working on a project in java based on a game called Yugioh!
Well, I tried to create as much class as possible for better coding and to respect SOLID principales. I want to focus on the duel part, for my question. I have a turnController, duelController, turn, and duel classes. A duel has a list of turns. I wonder if my dependencies are correct or if it is a better way to do that and some good practice rules.

public class Duel {
    private Player player1;
    private Player player2;
    private List<Turn> turns;
    private boolean isPlayer1Turn;
    private Turn currentTurn;

    public Duel(Player duelist, Player opponent)
    {
        this.player1 = duelist;
        this.player2 = opponent;
        this.turns = new ArrayList<Turn>();
        isPlayer1Turn = true;
    }
    //....getter and setter...

    public void play()
    {
        System.out.println("Starting the duel between" + player1.getName() + "and" + player2.getName());
        preparePlayer();
    }

    public void switchTurn() {
          isPlayer1Turn = !isPlayer1Turn;
    }


    public void preparePlayer()
    {
        setPlayerLP();
        shufflePlayersDeck();
        fillPlayerHands();
    }

    public void shufflePlayersDeck()
    {
        player1.getDuelDeck().getMainDeck().shuffle();
        player2.getDuelDeck().getMainDeck().shuffle();
    }

    public void fillPlayerHands()
    {
        player1.setHand(new Hand());
        player2.setHand(new Hand());

        for (int i = 0; i < 5; i++)
        {
            player1.drawCard();
            player2.drawCard();
        }
    }

    public void setPlayerLP()
    {
        player1.setLP(8000);
        player2.setLP(8000);
    }


    public void end()
 {
        System.out.println("END OF DUEL");
    }

    public boolean isDuelOver()
    {
        return player1.getLP()<= 0 || player2.getLP() <= 0;
    }
}

/**
 * A class representing a single turn in a Yu-Gi-Oh! duel.
 * Each turn belongs to a specific Duel and contains information about the current phase and the player taking the turn.
 */
public class Turn {
     private phase phase;
    
     private Duel duel;

//getter and setters
    public Turn(Duel duel){
        this.duel = duel;
    }
  

}

[code=java]
public class TurnController  {
    @FXML
    private Button draw;
    @FXML
    private Button sp;
    @FXML
    private Button main1;
    @FXML
    private Button battle;
    @FXML
    private Button main2;
    @FXML
    private Button end;
    @FXML
    VBox buttons;
    private Turn turn ;
    DuelController duelControl;

    public void setDuelControl(DuelController duelControl) {

        this.duelControl = duelControl;
    }

    public void handleDrawPhase(MouseEvent event) {
        System.out.println("draw phase handler");
        Button button = (Button) event.getSource();
        button.setDisable(true);
        turn.setCurrentPhase(new DrawPhase(turn.getDuel().getCurrentTurn()));
        turn.getCurrentPhase().play();
    }
    public void handleSpPhase(MouseEvent event) {
        System.out.println("Sp phase handler");
        Button button = (Button) event.getSource();
        button.setDisable(true);
        turn.setCurrentPhase(new SpPhase(turn.getDuel().getCurrentTurn()));
        turn.getCurrentPhase().play();
    }
    public void handleMain1Phase(MouseEvent event) {
        System.out.println("Main1 phase handler");
        turn.setCurrentPhase(new MainPhase(Phase.MAIN1, turn));
        turn.getCurrentPhase().play();
    }
    public void handleBattlePhase(Event event) {
        System.out.println("battle phase handler");
        Button button = (Button) event.getSource();
        disbaleButtonsBefore(button);
        turn.setCurrentPhase(new BattlePhase(turn));
        turn.getCurrentPhase().play();
    }
    public void handleMain2Phase(Event event) {
        System.out.println("main2 phase handler");
        Button button = (Button) event.getSource();
        disbaleButtonsBefore(button);
        turn.setCurrentPhase(new MainPhase(Phase.MAIN2, turn));
        turn.getCurrentPhase().play();
    }
    public void handleEndPhase(Event event) {
        System.out.println("end phase handler");
        Button button = (Button) event.getSource();
        button.setDisable(true);
        disbaleButtonsBefore(button);
        turn.setCurrentPhase(new EndPhase(turn));
        duelControl.startTurn();
    }
    public void disbaleButtonsBefore(Button button) {
        int currentIndex = buttons.getChildren().indexOf(button);
        int beforeIndex = currentIndex--;
        button = (Button) buttons.getChildren().get(beforeIndex);
        while (!button.isDisable())
        {
            button.setDisable(true);
            beforeIndex--;
            button = (Button) buttons.getChildren().get(beforeIndex);
        }
    }

    public void resetButtons()
    {
        for (Node button : buttons.getChildren())
        {
            button.setDisable(false);
        }
    }

    public Button getDraw() {
        return draw;
    }
    public Button getSp() {
        return sp;
    }
    public Button getMain1() {
        return main1;
    }
    public void setMain1(Button main1) {
        this.main1 = main1;
    }
    public Button getBattle() {
        return battle;
    }
    public Button getMain2() {
        return main2;
    }
    public Button getEnd() {
        return end;
    }
    public Turn getTurn() {
        return turn;
    }
    public void setTurn(Turn currentTurn) {
        this.turn = currentTurn;
    }

}

public class DuelController implements Initializable
{
    private Duel duel;
    @FXML
    private AnchorPane turn;

    @FXML TurnController turnController;


    private Deck createDeck(String deckName) throws SQLException, IOException
    {
        Deck deck = new Deck(DeckType.MAIN);
        ResultSet deckCardsRes = DataBaseEngine.selectCards(deckName);
        while(deckCardsRes.next()) {deck.getCardList().add(CardFactory.createCard(deckCardsRes));}
        return deck;
    }
    private Duel simulateGame()
    {
        Player player = new Player("Lyna");
        Player opponent = new Player("Lili");

        DeckSet pDeckSet = new DeckSet("deck1");
        DeckSet opponentDeckSet = new DeckSet("deck2");

        player.getDecks().add(pDeckSet);
        opponent.getDecks().add(opponentDeckSet);

        player.setDuelDeck (player.getDecks().get(0));
        opponent.setDuelDeck (opponent.getDecks().get(0));

        try
        {
            player.getDuelDeck().setMainDeck(createDeck("kuriboh"));
            opponent.getDuelDeck().setMainDeck(createDeck("kuriboh"));

        } catch (SQLException e) {
            throw new RuntimeException(e);
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
        return new Duel(player, opponent);
    }
    @Override
    public void initialize(URL url, ResourceBundle resourceBundle)
    {
        turnController.setDuelControl(this);
        duel = simulateGame();
        duel.play();
        startTurn();
    }
    public void startTurn()
    {
        Turn newTurn = new Turn(duel);
        duel.setCurrentTurn(newTurn);
        setTurn();
        turnController.getDraw().fireEvent(new MouseEvent(MouseEvent.MOUSE_CLICKED, 0, 0, 0, 0, null, 0, false, false, false, false, false, false, false, false, false, false, null));
        turnController.getSp().fireEvent(new MouseEvent(MouseEvent.MOUSE_CLICKED, 0, 0, 0, 0, null, 0, false, false, false, false, false, false, false, false, false, false, null));
        turnController.getMain1().fireEvent(new MouseEvent(MouseEvent.MOUSE_CLICKED, 0, 0, 0, 0, null, 0, false, false, false, false, false, false, false, false, false, false, null));
    }
    public Duel getDuel() {return duel;}

    public void setTurn()
    {
        turnController.resetButtons();
        turnController.setTurn(duel.getCurrentTurn());
    }
}

[Lou Hamers]

I don't think there's any terrible sins committed here. A few suggestions..

1)
@FXML fields injected like this
@FXML
VBox buttons;

...can be private:
@FXML
private VBox buttons;

Edit: Also I'm not sure I like the TurnController reference dependency inside DuelController (and vice versa), but I noticed that after I wrote this, so maybe that would be resolvable with better MVC concern separation explained below.

2)
The SOLID stuff is good to be familiar with (those plus all the other "rules" Bob Martin and his type lay on is too much for human brain to stay engaged with at all times)... but personally I would focus more on Model / View / Controller separation. In particular keeping your view rendering code separate from your "business" logic can often help avoid a lot of headaches. There's a lot of resources on MVC and the various other versions of that to read up on.

So with that in mind, it looks like Turn is a clear model class, but "Duel" is more unclear. Is it a data stucture, or something which implements your game logic? You might want to have a simple Duel data model and a separate class named DuelCalculator/DuelResolver/DuelSimulator (naming is always hard; I guess I would pick the last one). The DuelSimulator would do the actual backend calculation of resolving turns in the game, and would take your model classes as inputs for doing so.

One rule of thumb for knowing you have good View and Controller separation is determining whether you're able to run your "backend simulation" without a user interface at all, or with a totally different interface like a console. If you can, that means it'll be much easier to implement automated tests of your backend (i.e. "behind the scenes" logic), when you don't want to involve your UI view stuff.

3)
Probably not crucial for starting out, but it might be good to start with a habit of using a Java logger instead of System.out.println(). Meaning something like SLF4J + Logback or Log4j2 (the latter I'm no longer a fan of after recent security crisis fiascos). Although you could probably get away with just SLF4J's builtin "simple" logging too.

4)
Exception handling is a complex topic that I won't go into here (lots of resources to read about that out there), but you probably want to do something better than just re-throwing checked Exceptions as RuntimeExceptions (and you can also use the above logging to handle those better, if they are something you're able to recover from without crashing the game).


I'm also tempted to complain a bit about JavaFX's "Controller" naming convention/confusion. Perhaps they should've called them simply "View" classes or at least "ViewControllers". But this is not directly related to your code and I think I've put enough here for now.

[lyna lola]

Hello,

Thank you for your answer. I though I was respecting the MVC but it seems not lol. For me my C is for example dualController, V = duel.fxml and M = duel.java. So , in the model , there is no function, only attributes and getter/setter ? and the duelSimulator will do the isDuelOver() etc? How can I link then the duelSimulator with DuelController ? and please, how can I remove the injection in turnController of the duel ?

Thank you for your help and answers

[Lou Hamers]

Sorry I didn't realize you had a response, I must have missed a notification mail.

MVC can be tough to grasp at first, although it sounds simple. To make it worse IMO JavaFX examples tend to blur the lines a little.
There are certainly different schools of thought on all this, but generally your model should mostly just be data structure. Not sure I'd say that's a hard rule but I try to keep methods other than getters and setters outside those classes.

To get to your other questions:

I would place anything you need to resolve the duel or combat you have taking place in a different class (or a package or classes if it's complex enough to warrant breaking other parts out). Something like an isDuelOver() method might make sense in the DuelSimulator class if I'm visualizing it correctly.

For how to couple things together:

There's other ways to do dependency injection with JFX, but to keep things simple initialization setters like you're already doing in a few places might be best for now:

private DuelSimulator duelSimulator;

public void setDuelSimulator(DuelSimulator duelSimulator) {
  this.duelSimulator = duelSimulator;
}

The main scary thing with the Duel/Turn Controller references to one another is that it's a circular dependency. If the DuelController just has a reference to TurnController and not the other way around, that's an improvement, but you might be able to eliminate both references. What does your higher level code which creates the views and "wires" things up look like? Does the DuelController's view contain a TurnController view? You might even be able to just merge the two - the DuelController doesn't look that complicated and much of the logic like startTurn() and createDeck() type things probably would fit better in DuelSimulator, which would make DuelController pretty minimal.
There's some database related stuff up there in createDeck()... I see a layered architecture emerging:

"View Layer" (DuelController if it's at the top of your scene graph)
"Service Layer" (DuelSimulator)
"Data Access Layer" (You've named it DataBaseEngine, sometimes these are named for ex. CardDeckDao)

DAO == data access object, for accessing persistent data storage, anything working with a ResultSet should be here, sending up the results to the service layer.

I also see you got around that dependency injection problem with DatabaseEngine by using a static method. If you move that code to DuelSimulator you still have the same issue, but maybe it's fine for now (I'm trying not to overwhelm with changes). A future option is to create a

private DatabaseEngine db;

reference inside DuelSimulator and let the simulator class initialize (new DatabaseEngine()) it, then no more need for static methods. Your view/controller stuff doesn't need direct access to a database once all that logic is moved to the simulator. So you can remove that coupling entirely and encapsulate the DB stuff entirely away from view stuff.

You can see why Spring became so popular years ago, dependency wiring can be tricky. Unfortunately you can't use Spring with JavaFX controllers very easily, or at least without some 3rd party lib involvement. (Unless you have a client/server architecture but I am guessing this is far more than you need.)

The last three lines in startTurn() are pretty painful, I would definitely eliminate code that fakes a mouse click like that. I could offer suggestions but it depends on where you go from here, if you merge DuelController and TurnController, and so on. Has any of this been helpful? It's a lot to grasp I'm sure.

[lyna lola]

Hello again,
Thank you for your explanation and examples. They are pretty helpful. Well, I wanted to separate duel from turn for the single responsibility principal.
Well on the game, a duel has different turns, and on each turn, there are different phases. for me,  it was so logic to create duel, phase and turn classes. I think I will just have a duel for now, instead of turn and duel. And about the simulation of event on the draw pase for example, it is because this phase is automatic and doesn't require an action from the player. Is there a cleaner way ? or is it okay in my case ?

Here is my architecture

com/example/yugioh/application/App.java
com/example/yugioh/application/Game.java
com/example/yugioh/controllers/BattleFieldController.java
com/example/yugioh/controllers/CardInfosController.java
com/example/yugioh/controllers/CardResultController.java
com/example/yugioh/controllers/DeckBuilderController.java
com/example/yugioh/controllers/DeckController.java
com/example/yugioh/controllers/DeckMenuController.java
com/example/yugioh/controllers/DeckSetController.java
com/example/yugioh/controllers/DuelController.java
com/example/yugioh/controllers/FieldController.java
com/example/yugioh/controllers/MainMenuController.java
com/example/yugioh/controllers/TurnController.java
com/example/yugioh/engines/ApiEngine.java
com/example/yugioh/engines/CardExporterEngine.java
com/example/yugioh/engines/DataBaseEngine.java
com/example/yugioh/engines/DuelSimulatorEngine.java
com/example/yugioh/enums/CardType.java
com/example/yugioh/enums/DeckType.java
com/example/yugioh/enums/EffectType.java
com/example/yugioh/enums/Face.java
com/example/yugioh/enums/Limit.java
com/example/yugioh/enums/MonsterType.java
com/example/yugioh/enums/PhaseEnum.java
com/example/yugioh/enums/Position.java
com/example/yugioh/enums/Speed.java
com/example/yugioh/enums/SpellCardType.java
com/example/yugioh/enums/TrapCardType.java
com/example/yugioh/factory/cardFactory/CardFactory.java
com/example/yugioh/factory/cardFactory/ICardFactory.java
com/example/yugioh/factory/cardFactory/MonsterCardFactory.java
com/example/yugioh/factory/cardFactory/SpellCardFactory.java
com/example/yugioh/factory/cardFactory/TrapCardFactory.java
com/example/yugioh/views/DuelView.java
com/example/yugioh/views/TurnView.java
com/example/yugioh/models/card/Card.java
com/example/yugioh/models/card/CardInfo.java
com/example/yugioh/models/card/FusionMonster.java
com/example/yugioh/models/card/LinkMonster.java
com/example/yugioh/models/card/MonsterCard.java
com/example/yugioh/models/card/NormalMonster.java
com/example/yugioh/models/card/PendulumMonster.java
com/example/yugioh/models/card/RitualMonster.java
com/example/yugioh/models/card/SpellCard.java
com/example/yugioh/models/card/SpellTrapCard.java
com/example/yugioh/models/card/SynchroMonster.java
com/example/yugioh/models/card/TrapCard.java
com/example/yugioh/models/card/XyzMonster.java
com/example/yugioh/models/deck/Deck.java
com/example/yugioh/models/deck/DeckSet.java
com/example/yugioh/models/duel/Duel.java
com/example/yugioh/models/duel/Turn.java
com/example/yugioh/models/field/BanishZone.java
com/example/yugioh/models/field/ExtraDeckZone.java
com/example/yugioh/models/field/Field.java
com/example/yugioh/models/field/FieldZone.java
com/example/yugioh/models/field/GraveyardZone.java
com/example/yugioh/models/field/HandZone.java
com/example/yugioh/models/field/IZone.java
com/example/yugioh/models/field/MainDeckZone.java
com/example/yugioh/models/field/MonsterZone.java
com/example/yugioh/models/field/SpellTrapZone.java
com/example/yugioh/models/field/Zone.java
com/example/yugioh/models/phase/BattlePhase.java
com/example/yugioh/models/phase/DrawPhase.java
com/example/yugioh/models/phase/EndPhase.java
com/example/yugioh/models/phase/IPhase.java
com/example/yugioh/models/phase/MainPhase.java
com/example/yugioh/models/phase/Phase.java
com/example/yugioh/models/phase/SpPhase.java

[Lou Hamers]

I usually decide if another FXML view is warranted depending mostly on how much the view is going to change. There's a lot you can do to make things dynamic by hiding and showing components, or even using Java to create or remove visual components that are very dynamic. More of an art than a science..

These FXML "controllers" to me in a JavaFX app has always felt like just additional View logic and thus part of the view, because it's just too tightly coupled to view components IMO. You need a special test framework (TestFX) to fully test them, which to me is a red flag that they're too coupled to the view to be considered anything else. Which is why I recommended putting as much logic as you can in a plain old Java class (simulator etc) and then having the DuelController send events and user interaction data to that class. Even doing that sometimes those JFX controller classes can get pretty complex if your UI has a lot of features, so the last thing you want is to have them handle stuff that can easily be pulled out.

[Lou Hamers]

Above was written before you edited the list in. You have a lot more there than I imagined!

What are DuelView.java and TurnView.java vs DuelController/TurnController?

If it's just visual (view) updates done on a timer, like animations and such, that should be fine in DuelController. I would use Java Timers for such updates.

Ultimately you're in the best position to make these decisions since you have the big requirements and goals picture. I can't say it's wrong to have both Duel+Turn controller, but it's not terrible to keep them merged until you can see that you clearly need "Turn" stuff broken out.

Edit:
com/example/yugioh/enums/PhaseEnum.java
com/example/yugioh/models/phase/Phase.java

Are both of these enums? The original code makes it look like Phase is also an enum. (I wouldn't put Enum in the name either.)
One more tip which Bob Martin would definitely appreciate - remove old/unused stuff and keep your code clean. I recommend using Git to version control things even if you're working alone. At the very least it's a good backup but it'll also preserve stuff you've deleted in case you decide you actually need it after all. (Also many IDEs have a local file history feature for that, but I wouldn't rely on it too much.)

[Paul Clapham]

Lou Hamers wrote:These FXML "controllers" to me in a JavaFX app has always felt like just additional View logic and thus part of the view, because it's just too tightly coupled to view components IMO.

Based on my Swing experience, it's damn hard to separate the view and controller logic. From the very beginning of coding you find that if you click on a button then it usually changes the rest of the view, and you really have to put on your Separation of Concerns uniform and march around the room before you can put that code into a separate controller tier. I've seen quite a bit of Swing code in web tutorials and that never happens there.

[Lou Hamers]

Paul Clapham wrote:
Based on my Swing experience, it's damn hard to separate the view and controller logic. From the very beginning of coding you find that if you click on a button then it usually changes the rest of the view, and you really have to put on your Separation of Concerns uniform and march around the room before you can put that code into a separate controller tier. I've seen quite a bit of Swing code in web tutorials and that never happens there.

Yeah it's tough sometimes. I just try to do the best I can to keep things that don't really need to be in those JFX controllers in their own space. JavaFX sort of forces the issue by calling these "Controllers" (it's even built into the the FXML spec), but I try to keep what's in there limited to things that need direct access to UI components as much as practical/feasible.

Notably I believe this lack of separation between view and logic is why people decided long ago that Java UIs suck. No, many of the Java UI devs back when Swing was big just messed up, by putting long running logic stuff in the Event Dispatch Thread, freezing up the UI. JavaFX also has an "EDT" of its own, so it's subject to the same potential poor experience. I don't think the OP has any long-running operations planned in this case though, so hopefully staying on the JFX thread will be fine.

[lyna lola]

DuelView and TurnView are only classes to load fxml files, I think it is a clean way to save paths ?

@Value
public class DuelView {
    String FXML_FILE = "/com/example/yugioh/fxml/Duel.fxml";;
    String TITLE = "Duel";
    Stage stage;

    public DuelView() throws IOException {
        FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource(FXML_FILE));
        Parent root = fxmlLoader.load();
        Scene scene = new Scene(root);
        stage = new Stage();
        stage.setTitle(TITLE);
        stage.setScene(scene);
        stage.show();
    }
}

and about the phases, which I think it is also a bad idea, it is an abstract class.

public abstract class Phase implements IPhase {
    private final PhaseEnum phaseName;

    public Phase(PhaseEnum phaseName) {
        this.phaseName = phaseName;
    }

    public PhaseEnum getPhaseName() {
        return phaseName;
    }
}
[code=java]
public class DrawPhase extends Phase {
    public DrawPhase() {
        super(PhaseEnum.DRAW);
    }

    @Override
    public void play() {
        System.out.println("DRAW PHASE");
        //TODO make the player draw 
    }
}

Well, it is a time for a good clean up and more thinking. Well well, it is more challenging than I thought lol... Yes btw, I have a git

[Lou Hamers]

Ah okay. I'm not sure if I mentioned this and wasn't going to because I'm not sure you need it, but there is a library you could use to structure things a little differently and it does dependency injection into your JFX controllers too: https://github.com/AdamBien/afterburner.fx