Managing scene switching in JavaFX via FXML (Performance Question)

Question

I'm playing arround with JavaFX and various scenes that are loaded by FXML. So I got the idea to write a manager that handles scene switching.

So far everything works, but I'm unsure if this is a good implementation.

import java.io.IOException;
import javafx.fxml.FXMLLoader;
import javafx.scene.Scene;
import javafx.scene.layout.Pane;
import javafx.stage.Stage;

public class SceneManager {
    private static final String[] fxmlFiles = {"../gui/MainWindow.fxml", "../gui/NewGameWindow.fxml"};
    private static SceneManager instance = null;
    private static Stage rootStage = null;

    private FXMLLoader[] loadedFxml;
    private Pane[] loadedPanes;
    private Scene[] scenes;

    public enum States {
        MAIN_MENU, NEW_GAME;
    }

    private SceneManager() {
        try {
            this.loadedFxml = new FXMLLoader[States.values().length];
            this.loadedPanes = new Pane[States.values().length];
            this.scenes = new Scene[States.values().length];

            for(int i = 0; i < fxmlFiles.length; i++) {
                loadedFxml[i] = new FXMLLoader(getClass().getResource(fxmlFiles[i]));
                loadedPanes[i] = loadedFxml[i].load();
                scenes[i] = new Scene(loadedPanes[i]);
            }

            rootStage.setScene(scenes[0]);
            rootStage.setResizable(false);
            rootStage.show();
        } catch(IOException e) {
            e.printStackTrace();
        }
    }

    public static SceneManager getInstance() {
        if(instance == null) {
            instance = new SceneManager();
        }
        return instance;
    }

    public static void setUp(Stage stage) {
        SceneManager.rootStage = stage;
    }

    public void switchScene(States state) {
        rootStage.setScene(scenes[state.ordinal()]);
    }
}

So what I plan to do is, load the FXML via the loader, assign it to a pane, create all scenes.

Then I set a scene as its starting scene and do the rest via the getInstance().switchScene() method in the controller.

It works well but I'm unsure if this is a good approach.

Answer 1

The implementation is imho pretty bad for several reasons:

The singleton pattern is not properly implemented

The singleton pattern is used to access an instance containing the relevant data/functionality via a static method, but this instance should contain all the relevant data as instance fields.

rootStage and setUp are static though.

You store data in fields that is never needed again

You never read from loadedFxml or loadedPanes outside of the loop. You could instead create local variables in the loop body.

Everything is loaded at once

For several small scenes this may not make much difference, but as you add more and more scenes this will increase startup time. Consider lazily loading the scenes.

Data for scenes is kept in different data structures

Not much of an issue, but it makes the class a bit harder to maintain. The enum stores one part of the data used to create/access the scenes fxmlFiles contains the other half. Every time you add/remove a scene you need to update both parts of your code. In cases like this it would be preferable to store the url data in the enum itself:

public enum States {
    MAIN_MENU("../gui/MainWindow.fxml"), NEW_GAME("../gui/NewGameWindow.fxml");
    private final url;

    States(String url) {
        this.url = url;
    }
}

for(States state : States.values()) {
    FXMLLoader loader = new FXMLLoader(getClass().getResource(state.url));
    ...
}

Note that you use .. in your urls here which ceases to work, if you package your program as a jar.

But using a enum in the first place is a questionable decision. This way you won't be able to add/remove scenes without recompiling.

Using static doesn't seem necessary at all

It's good to avoid the use of static at all if possible. (see Why are static variables considered evil?).

If my assumption that you only use the SceneManager class from scenes loaded by it (and for displaying the initial scene) is correct, it's not hard to pass the SceneManager instance to the controllers of the scenes to avoid the need of using SceneManager.getInstance in those classes (see Passing Parameters JavaFX FXML):

superclass for controllers

public class BaseController {
    protected SceneManager sceneManager;
    void setSceneManager(SceneManager sceneManager) { // if SceneManager and BaseController are in different packages, change visibility
        this.sceneManager = sceneManager;
    }
}

FXMLLoader loader = ...
Pane pane = loader.load();
BaseController controller = loader.getController();
controller.setSceneManager(this);

---

Using the url as identifiers for the scenes for simplicity, you could improve the implementation:

public class SceneManager {
    
    private final Stage rootStage;

    public SceneManager(Stage rootStage) {
        if (rootStage == null) {
            throw new IllegalArgumentException();
        }
        this.rootStage = rootStage;
    }

    private final Map<String, Scene> scenes = new HashMap<>();

    public void switchScene(String url) {
        Scene scene = scenes.computeIfAbsent(url, u -> {
            FXMLLoader loader = new FXMLLoader(getClass().getResource(u));
            try {
                Pane p = loader.load();
                BaseController controller = loader.getController();
                controller.setSceneManager(this);
                return new Scene(p);
            } catch (IOException ex) {
                throw new RuntimeException(ex);
            }
        });
        rootStage.setScene(scene);
    }
}

This allows you to

• create different managers for different stages
• load scenes when they are needed first
• dynamically add more scenes
• prevents state where switchScene is called but the stage is null
• simplifies access to the SceneManager in controller classes to sceneManager.switchScene
• SceneManager is possibly available for garbage collection before the program completes since there is no static reference to it.