3
\$\begingroup\$

I created an interface (IFXMLComponent) that can be added to a FXML controller class.

The FXML controller class that implements this interface will be associated with an FXML file.

The interface both support custom FXML components, in that case the FXML controller class extends a JavaFX node (e.g. javafx.scene.layout.HBox, javafx.scene.control.Button, ...). And it support FXML components, in that case the FXML controller has no parent class and the FXML file has a root node of type JavaFX node e.g. javafx.scene.layout.HBox, javafx.scene.control.Button).

The caveat is that the loadFXML method needs to be the last method in the constructor of the FXML controller class. Because when the FXML file is loaded, the initialize method of the FXML component class is called. And in the initialize method, the child controls are bind to properties, that could be instantiated or initialized in the constructor.

So is this interface the way to go? Or should I create a static method where the FXML controller class is passed as argument.

Static method implementation:

FXMLComponent.java

public static FXMLLoader loadXML(final Class<?> clazz) {
    final FXMLLoader fxmlLoader = ...
}

FXMLChildPane.java

FXMLChildPane(@NamedArg("viewModel") {
    ...
    FXMLComponent.loadXML(FXMLChildPane.class);
    // or 
    FXMLComponent.loadXML(this.getClass());
}

Interface implementation

FXMLChildPane.java

public class FXMLChildPane extends VBox implements IFXMLComponent
{
    @FXML
    private TextField childValueField;

    private final ChildViewModel viewModel;

    public FXMLChildPane()
    {
        this(new ChildViewModel());
        System.out.println("ChildPane: No arg constructor.");
    }

    public FXMLChildPane(@NamedArg("viewModel") final ChildViewModel viewModel)
    {
        System.out.println("ChildPane: constructor with viewModel.");
        this.viewModel = viewModel;
        loadFXML();
    }

    @FXML
    private void initialize()
    {
        this.childValueField.textProperty().bindBidirectional(this.viewModel.childValueProperty());
    }
}

IFXMLComponent.java

public interface IFXMLComponent
{
    /**
     * Load an FXML file, that defines an FXML component, with the FXML loader.
     * The simple name of the class is used to find the associated FXML file. E.g. CustomPane.class is associated with CustomPane.fxml.
     * This method should be the last method called in the constructor.
     * @return FXMLLoader
     */
    default FXMLLoader loadFXML()
    {
        final Callback<Class<?>, Object> controllerFactory = null;
        return this.loadFXML(controllerFactory);
    }

    /**
     * Load an FXML file, that defines an FXML component, with the FXML loader.
     * If the class has no default no argument constructor (or the FXMLLoader should not use the default constructor), a controller factory should be defined.
     * The simple name of the class is used to find the associated FXML file. E.g. CustomPane.class is associated with CustomPane.fxml.
     * This method should be the last method called in the constructor.
     * @param controllerFactory define a controller factory if the controller has no default no argument constructor.
     * @return FXMLLoader
     */
    default FXMLLoader loadFXML(final Callback<Class<?>, Object> controllerFactory)
    {
        // Use simple name of the class for the FXML document. So class CustomPane is associated to CustomPane.fxml
        final String fxmlDocument = new StringBuilder(getClass().getSimpleName()).append(".fxml").toString();
        return this.loadFXML(fxmlDocument);
    }

    /**
     * Load the FXML file, that defines an FXML component, with the FXML loader.
     * This method should be the last method called in the constructor.
     * @param fmxlFile given FXML file that is associated with this FXML component.
     * @return FXMLLoader
     */
    default FXMLLoader loadFXML(final String fmxlFile)
    {
        return loadFXML(fmxlFile, null);
    }

    /**
     * Load the FXML file, that defines an FXML component, with the FXML loader.
     * If the class has no default no argument constructor (or the FXMLLoader should not use the default constructor), a controller factory should be defined.
     * This method should be the last method called in the constructor.
     * @param fmxlFile given FXML file that is associated with this FXML component.
     * @return FXMLLoader
     */
    default FXMLLoader loadFXML(final String fmxlFile, final Callback<Class<?>, Object> controllerFactory)
    {
        return loadFXML(getClass(), fmxlFile, controllerFactory);
    }

    /**
     * Load the FXML file, that defines an FXML component, with the FXML loader.
     * This method should be the last method called in the constructor.
     * @param clazz defines the class loader that is used to find the FXML file and that is used by the FXML loader.
     * @param fmxlFile given FXML file that is associated with this class.
     * @param controllerFactory define a controller factory if the controller has no default no argument constructor.
     * @return FXMLLoader
     */
    default FXMLLoader loadFXML(final Class<?> clazz, final String fmxlFile)
    {
        return this.loadFXML(clazz, fmxlFile, null);
    }

    /**
     * Load the FXML file, that defines an FXML component, with the FXML loader.
     * If the class has no default no argument constructor (or the FXMLLoader should not use the default constructor), a controller factory should be defined.
     * This method checks if the controller defines a custom FXML component or non custom FXML component.
     * This method should be the last method called in the constructor.
     * @param clazz defines the class loader that is used to find the FXML file and that is used by the FXML loader.
     * @param fmxlFile given FXML file that is associated with this class.
     * @param controllerFactory define a controller factory if the controller has no default no argument constructor.
     * @return FXMLLoader
     */
    default FXMLLoader loadFXML(final Class<?> clazz, final String fmxlFile, final Callback<Class<?>, Object> controllerFactory)
    {
        final FXMLLoader fxmlLoader = new FXMLLoader(clazz.getResource(fmxlFile));

        if (Node.class.isAssignableFrom(clazz))
        {
            // Controller extends from JavaFX Node. In this case the controller defines a custom component.
            // Assign the root node and controller to the controller.
            fxmlLoader.setRoot(this);
            fxmlLoader.setController(this);

            // no need to set the controller factory of the FXMLLoader
        }
        else
        {
            // Not a custom component.
            // Do not call the setRoot and setController methods.
            // The FXMLLoader defines these properties based on the root element and fx:controller attribute in the FXML.

            // Use controller factory if defined.
            if (controllerFactory != null)
            {
                fxmlLoader.setControllerFactory(controllerFactory);
            }
        }

        try
        {
            fxmlLoader.load();
            return fxmlLoader;
        }
        catch (Exception e)
        {
            System.err.println("Failed to load FXML file: " + Objects.toString(fmxlFile) + ", for class: "
                    + Objects.toString(clazz.getCanonicalName()) + ". Exception: " + e.getMessage());
            throw new RuntimeException(e);
        }
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Bug:

The loadFXML overload with Callback<Class<?>, Object> as argument does not pass the controllerFactory through. Instead of return this.loadFXML(fxmlDocument) it should say return this.loadFXML(fxmlDocument, controllerFactory).

Formatting:

The javadoc is nearly unformatted and generally contains too long lines. I strongly suggest making use of automatic formatting to get the javadoc into the more readable format:

/**
 * <p>
 * Load an FXML file that defines an FXML component, with the FXML loader.
 * If the class has no default no argument constructor (or the FXMLLoader 
 * should not use the default constructor), a controller factory should be defined.
 * <p>
 * The simple name of the class is used to find the associated FXML file. 
 * e.g. CustomPane.class is associated with CustomPane.fxml.
 * <p>
 * This method should be the last method called in the constructor.
 * 
 * @param controllerFactory 
 *    define a controller factory if the controller doesn't have a no-argument ctor.
 * @return FXMLLoader
 *    The initialized and loaded FXMLLoader
 */

Adding light HTML formatting through <p> usually tremendously improves the reading experience in the javadoc rendering, keeping the lines shorter than your screen width improves the reading experience in the editor directly.

Functionality:

The intention of the code is pretty clear: You're trying to simplify and deduplicate the loading process for FXML components. The question is: why?

One issue I see here is that it might be useful to provide more customization possibilities before actually calling load on the loader. Another issue I have with this is the requirement to call load last in the ctor. What if I want to customize and initialize the FXML-injected fields?

Furthermore it's bad practice to log to System.err instead of using a proper Logger, just the way it is bad practice to leak the this-reference.

Last but not least you're expecting that the implementations of your methods are not changed, which you cannot guarantee on an interface. The whole idea of an interface is that it's changed...

This puts us into a catch-22 situation. You want to allow the controller class to extend a Node (or a Node subclass), but still have your methods basically "immutable". At this point the code is fighting the language. Ultimately the java language this way somewhat allows multiple implementation inheritance... The "actually correct" solution here is to just swallow the bitter pill of duplicated FXML loading code.

In the end, this code is some 40 Lines of effective code with extensive documentation, intended to replace 3, maybe 4 lines in each of the implementing classes. Of course just counting lines this is useful, but in the end you're just hiding an implementation detail into an unintuitive place (namely higher in the hierarchy instead of lower).

I'd personally flat out ignore this code, because it's cost is too high compared to the benefits it provides.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the extended feedback and finding the bug (I never noticed it). I created this interface to avoid the duplicate code, and also to make it easier to setup the FXMLLoader. Setup of the FXMLLoader depends on the type of FXML component (being a custom component or not). You really need the know the fine details of the FXMLLoader in order to do the setup correctly. I wanted to make it easier to use FXML, for developers not knowing the fine details of the FXML loader. The interface is indeed a catch 22. Nothing prevents the user of this interface, to override the methods. \$\endgroup\$ Commented Sep 25, 2017 at 13:22

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.