Follow the Rubberduck - Part 3: JavaFX MVC and MVP Matrjoshka

Question

After the last post had some minor improvements for me and the beta release was actually used for some things, I decided that the GUI was too ugly to continue like this and rewrote the whole GUI in JavaFX.

The Matrjoshka - Usage

This posed some challenges for MVC / MVP, because of some peculiarities with JavaFX Views and Controllers. The final result of that rewrite is a matrjoshka-puppet-like structure, where the MVP-View of the Application is itself an MVC-based "application".
Unfortunately I have found no other way to properly abstract away the interactions with my Controllers and as of now my Main-Method looks like this:

public static void main(String[] args) {
    if (args.length > 1) {
        // don't even bother!
        System.out.println(ARGUMENT_MISMATCH);
        return;
    }
    launch(args);
}

@Override
public void start(Stage primaryStage) throws Exception {
    Stage rcStage = new Stage(StageStyle.UTILITY);
    rcStage.initOwner(primaryStage);
    ResxChooser rc = new JFXResxChooserView(rcStage, getClass().getResource("/ResxChooser.fxml"));
    Parameters params = getParameters();
    if (params.getUnnamed().size() != 0) { // should be 1..
        final Path resxFile = Paths.get(params.getUnnamed().get(0));
        rc.setFileset(resxFile);
    }
    Stage translationStage = new Stage(StageStyle.UTILITY);
    translationStage.initOwner(primaryStage);
    TranslationView tv = new JFXTranslationView(translationStage, getClass().getResource("/TranslationView.fxml"));
    OverviewView v = new JFXOverviewView(primaryStage, getClass().getResource("/OverviewView.fxml"));

    OverviewModel m = new OverviewModel();
    Dialog d = new JFXDialog();
    OverviewPresenter p = new OverviewPresenter(m, v, tv, rc, d);
    // Wire up all the crap
    DependencyRoot.inject(m, v, p, tv, rc);

    Platform.runLater(p::show);
    Platform.runLater(p::fileChoosing);
}

As you can see the OverviewPresenter is basically the Octo-Kraken, having a reference to every significant instance. For now I'm not too worried about that, since all interactions are event-based and the refactoring to clean this is simplistic. It just hasn't been necessary yet.

What I'd much more like a review on is the way I set up my Views.

The Matrjoshka - Setup

As mentioned above this feels like a Matrjoshka-Puppet. The basic gist of it is, that JavaFX expects me to specify a fx:controller in my fxml-files that the UI elements are injected into using black magic @FXML-Annnotations. Since that means I cannot control instantiation of that controller, let alone inject dependencies into it without resorting to a DI-Framework (which seems too heavy handed for my 5-odd dependencies) I wanted to abstract the FXML-Bound Controller into a class I can control.

The FXML is the following:

<?import javafx.geometry.Insets?>
<?import javafx.scene.control.Button?>
<?import javafx.scene.control.ButtonBar?>
<?import javafx.scene.control.TableView?>
<?import javafx.scene.layout.BorderPane?>

<BorderPane xmlns:fx="http://javafx.com/fxml"
            fx:controller="de.vogel612.helper.ui.jfx.JFXOverviewController"
            minHeight="300" minWidth="600">
    <padding>
        <Insets top="15" bottom="20" right="15" left="15"/>
    </padding>
    <top>
        <ButtonBar minHeight="30">
            <buttons>
                <Button fx:id="save" text="Save" minWidth="60" minHeight="20"/>
                <Button fx:id="chooseLang" text="Choose Languages" minWidth="60" minHeight="20"/>
            </buttons>
        </ButtonBar>
    </top>
    <center>
        <TableView fx:id="table"/>
    </center>
</BorderPane>

Very simplistic. Just a table and two buttons. This gets bound to the JFXOverviewController that I cannot control:

public class JFXOverviewController extends OverviewViewCommon implements Initializable {

    @FXML
    private Button save;

    @FXML
    private Button chooseLang;

    @FXML
    private TableView<TranslationPair> table;

    public JFXOverviewController() {

    }

    @Override
    public void initialize() {
        // empty?
    }

    @Override
    public void show() {
        throw new UnsupportedOperationException();
    }

    @Override
    public void rebuildWith(List<Translation> left, List<Translation> right) {
        if (left.isEmpty() || right.isEmpty()) {
            return;
        }
        table.setItems(buildObservableList(left, right));
        table.getColumns().get(0).setText(left.get(0).getLocale());
        table.getColumns().get(1).setText(right.get(0).getLocale());
    }

    private static ObservableList<TranslationPair> buildObservableList(List<Translation> left,
      List<Translation> right) {
        List<TranslationPair> result = new ArrayList<>();
        final int limit = Math.min(left.size(), right.size());
        for (int i = 0; i < limit; i++) {
            result.add(new TranslationPair(left.get(i), right.get(i)));
        }
        return FXCollections.observableList(result);
    }


    @Override
    public void hide() {
        throw new UnsupportedOperationException();
    }

    @Override
    public void initialize(URL url, ResourceBundle resourceBundle) {
        Objects.requireNonNull(save, "save was not FXML-injected correctly");
        Objects.requireNonNull(table, "table was not FXML-injected correctly");
        Objects.requireNonNull(chooseLang, "chooseLang was not FXML-injected correctly");

        save.setOnAction(evt -> saveRequestListeners.forEach(Runnable::run));
        chooseLang.setOnAction(evt -> langChoiceRequestListeners.forEach(Runnable::run));
        Callback<TableColumn<TranslationPair,String>, TableCell<TranslationPair, String>> cellRenderer =
          column -> {
              TableCell<TranslationPair, String> cell = new TableCell<TranslationPair, String>() {
                  @Override
                  protected void updateItem(String item, boolean empty) {
                      super.updateItem(item, empty);
                      setText(item);
                      TranslationPair rowValue = (TranslationPair) getTableRow().getItem();
                      if (rowValue != null) {
                          assignHighlightClasses(rowValue);
                      }
                  }

                  private void assignHighlightClasses(TranslationPair rowValue) {
                      getStyleClass().remove("default");
                      getStyleClass().remove("warn");
                      getStyleClass().remove("error");

                      switch (NotableData.assessNotability(rowValue)) {
                          case INFO:
                          case DEFAULT:
                              getStyleClass().add("default");
                              break;
                          case WARNING:
                              getStyleClass().add("warn");
                              break;
                          case ERROR:
                              getStyleClass().add("error");
                              break;
                      }
                  }
              };
              cell.addEventFilter(MouseEvent.MOUSE_CLICKED, evt -> {
                  // assume the double-click selected the relevant row....
                  if (evt.getButton() != MouseButton.PRIMARY || evt.getClickCount() != 2) {
                      return;
                  }
                  translationRequestListeners.forEach(listener -> {
                      listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
                  });
              });
              return cell;
          };

        TableColumn<TranslationPair, String> leftColumn = new TableColumn<>("");
        leftColumn.setCellValueFactory(data -> new ObservableValueBase<String>() {
            @Override
            public String getValue() {
                return data.getValue().getLeft().getValue();
            }
        });
        leftColumn.setCellFactory(cellRenderer);

        TableColumn<TranslationPair, String> rightColumn = new TableColumn<>("");
        rightColumn.setCellValueFactory(data -> new ObservableValueBase<String>() {
            @Override
            public String getValue() {
                return data.getValue().getRight().getValue();
            }
        });
        rightColumn.setCellFactory(cellRenderer);

        table.getColumns().clear();
        table.getColumns().add(leftColumn);
        table.getColumns().add(rightColumn);

        table.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);
        table.setColumnResizePolicy(TableView.CONSTRAINED_RESIZE_POLICY);

        table.setOnKeyPressed(event -> {
            if (event.getCode() == KeyCode.ENTER && table.getSelectionModel().getSelectedItem() != null) {
                translationRequestListeners.forEach(listener -> {
                    // so many assumptions :/
                    listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
                });
            }
        });

        table.setEditable(false);
    }


    // FIXME this is fugly
    void triggerCloseRequest() {
        windowCloseListeners.forEach(Runnable::run);
    }
}

Since I'm not actually directly wiring any of this up and most of the logic in the View is directly related to the UI components, the Controller is very cramped. The Class actually instantiated is significantly less interesting in and of itself, because it mostly delegates to the Controller.

Interesting here is the Constructor:

public class JFXOverviewView implements OverviewView {

    private final JFXOverviewController controller;
    private final Scene ui;
    private final Stage stage;

    public JFXOverviewView(Stage stage, URL fxml) throws IOException {
        this.stage = stage;
        FXMLLoader loader = new FXMLLoader(fxml);
        ui = new Scene(loader.load());
        ui.getStylesheets().clear();
        // this works fine
        ui.getStylesheets().add(getClass().getResource("/style.css").toExternalForm());

        controller = loader.getController();

        stage.setOnCloseRequest(evt -> controller.triggerCloseRequest());
    }

    @Override
    public void addWindowClosingListener(Runnable listener) {
        controller.addWindowClosingListener(listener);
    }

    @Override
    public void addLanguageRequestListener(Runnable listener) {
        controller.addLanguageRequestListener(listener);
    }

    @Override
    public void addTranslationRequestListener(Consumer<String> listener) {
        controller.addTranslationRequestListener(listener);
    }

    @Override
    public void addSaveRequestListener(Runnable listener) {
        controller.addSaveRequestListener(listener);
    }

    @Override
    public void initialize() {
        // shouldn't be needed anymore
    }

    @Override
    public void show() {
        Platform.runLater(() -> {
            stage.setScene(ui);
            stage.show();
        });
    }

    @Override
    public void rebuildWith(List<Translation> left, List<Translation> right) {
        controller.rebuildWith(left, right);
    }

    @Override
    public void hide() {
        stage.hide();
    }
}

The Matrjoshka - Smelly or not smelly?

Basically my question boils down to: Does this smell? And if it does... how can I fix it?

As always comments on everything are appreciated.

The code is fully available on github

Answer 1

table.setOnKeyPressed(event -> {
    if (event.getCode() == KeyCode.ENTER && table.getSelectionModel().getSelectedItem() != null) {
        translationRequestListeners.forEach(listener -> {
            // so many assumptions :/
            listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
        });
    }
});

At some point, you're chaining too many function calls.

If you're making a builder of sorts, then chaining is the way to go. But in this case, you really could do with temporarily storing the result of some things.

This is the worst example - you could store table.getSelectionModel().getSelectedItem() in a separate variable here.

---

Another thing which is a pet peeve of mine is that if we're creating a GUI then, well, we ought to have one method that instantiates the GUI and all its buttons and features and functions. The idea of keeping everything in one place is fine, but I believe it violates the idea of programming at the same level of abstraction in a function.

@Override
public void initialize(URL url, ResourceBundle resourceBundle) {
    Objects.requireNonNull(save, "save was not FXML-injected correctly");
    Objects.requireNonNull(table, "table was not FXML-injected correctly");
    Objects.requireNonNull(chooseLang, "chooseLang was not FXML-injected correctly");

    save.setOnAction(evt -> saveRequestListeners.forEach(Runnable::run));
    chooseLang.setOnAction(evt -> langChoiceRequestListeners.forEach(Runnable::run));
    Callback<TableColumn<TranslationPair,String>, TableCell<TranslationPair, String>> cellRenderer =
      column -> {
          TableCell<TranslationPair, String> cell = new TableCell<TranslationPair, String>() {
              @Override
              protected void updateItem(String item, boolean empty) {
                  super.updateItem(item, empty);
                  setText(item);
                  TranslationPair rowValue = (TranslationPair) getTableRow().getItem();
                  if (rowValue != null) {
                      assignHighlightClasses(rowValue);
                  }
              }

              private void assignHighlightClasses(TranslationPair rowValue) {
                  getStyleClass().remove("default");
                  getStyleClass().remove("warn");
                  getStyleClass().remove("error");

                  switch (NotableData.assessNotability(rowValue)) {
                      case INFO:
                      case DEFAULT:
                          getStyleClass().add("default");
                          break;
                      case WARNING:
                          getStyleClass().add("warn");
                          break;
                      case ERROR:
                          getStyleClass().add("error");
                          break;
                  }
              }
          };
          cell.addEventFilter(MouseEvent.MOUSE_CLICKED, evt -> {
              // assume the double-click selected the relevant row....
              if (evt.getButton() != MouseButton.PRIMARY || evt.getClickCount() != 2) {
                  return;
              }
              translationRequestListeners.forEach(listener -> {
                  listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
              });
          });
          return cell;
      };

    TableColumn<TranslationPair, String> leftColumn = new TableColumn<>("");
    leftColumn.setCellValueFactory(data -> new ObservableValueBase<String>() {
        @Override
        public String getValue() {
            return data.getValue().getLeft().getValue();
        }
    });
    leftColumn.setCellFactory(cellRenderer);

    TableColumn<TranslationPair, String> rightColumn = new TableColumn<>("");
    rightColumn.setCellValueFactory(data -> new ObservableValueBase<String>() {
        @Override
        public String getValue() {
            return data.getValue().getRight().getValue();
        }
    });
    rightColumn.setCellFactory(cellRenderer);

    table.getColumns().clear();
    table.getColumns().add(leftColumn);
    table.getColumns().add(rightColumn);

    table.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);
    table.setColumnResizePolicy(TableView.CONSTRAINED_RESIZE_POLICY);

    table.setOnKeyPressed(event -> {
        if (event.getCode() == KeyCode.ENTER && table.getSelectionModel().getSelectedItem() != null) {
            translationRequestListeners.forEach(listener -> {
                // so many assumptions :/
                listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
            });
        }
    });

    table.setEditable(false);
}

I'm talking about this function, of course.

Let's step through each part...

    Objects.requireNonNull(save, "save was not FXML-injected correctly");
    Objects.requireNonNull(table, "table was not FXML-injected correctly");
    Objects.requireNonNull(chooseLang, "chooseLang was not FXML-injected correctly");

Argument validation. Clear and simple.

    save.setOnAction(evt -> saveRequestListeners.forEach(Runnable::run));

Save button. Also clear.

    chooseLang.setOnAction(evt -> langChoiceRequestListeners.forEach(Runnable::run));

Choose language button. Also clear.

    Callback<TableColumn<TranslationPair,String>, TableCell<TranslationPair, String>> cellRenderer =
      column -> {
          TableCell<TranslationPair, String> cell = new TableCell<TranslationPair, String>() {
              @Override
              protected void updateItem(String item, boolean empty) {
                  super.updateItem(item, empty);
                  setText(item);
                  TranslationPair rowValue = (TranslationPair) getTableRow().getItem();
                  if (rowValue != null) {
                      assignHighlightClasses(rowValue);
                  }
              }

              private void assignHighlightClasses(TranslationPair rowValue) {
                  getStyleClass().remove("default");
                  getStyleClass().remove("warn");
                  getStyleClass().remove("error");

                  switch (NotableData.assessNotability(rowValue)) {
                      case INFO:
                      case DEFAULT:
                          getStyleClass().add("default");
                          break;
                      case WARNING:
                          getStyleClass().add("warn");
                          break;
                      case ERROR:
                          getStyleClass().add("error");
                          break;
                  }
              }
          };
          cell.addEventFilter(MouseEvent.MOUSE_CLICKED, evt -> {
              // assume the double-click selected the relevant row....
              if (evt.getButton() != MouseButton.PRIMARY || evt.getClickCount() != 2) {
                  return;
              }
              translationRequestListeners.forEach(listener -> {
                  listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
              });
          });
          return cell;
      };

A cell renderer which iterates over each translation pair stored in the table and applies highlight classes to them, whilst also making it possible to click on table cells - but it's actually a double click - for passing the selected item to the translation request listeners.

That's... not clear at all. It reads like a shift change handover (info dump with everything you have to know to get back up to speed, hopefully, and also, bye).

If you had made separate functions for this bit, then this info dump could have been kept in its own box.

Moving on,

    TableColumn<TranslationPair, String> leftColumn = new TableColumn<>("");
    leftColumn.setCellValueFactory(data -> new ObservableValueBase<String>() {
        @Override
        public String getValue() {
            return data.getValue().getLeft().getValue();
        }
    });
    leftColumn.setCellFactory(cellRenderer);

Left column of the table.

    TableColumn<TranslationPair, String> rightColumn = new TableColumn<>("");
    rightColumn.setCellValueFactory(data -> new ObservableValueBase<String>() {
        @Override
        public String getValue() {
            return data.getValue().getRight().getValue();
        }
    });
    rightColumn.setCellFactory(cellRenderer);

Right column of the table.

Difference?

Left: return data.getValue().getLeft().getValue();
Right: return data.getValue().getRight().getValue();

Smells like duplication. Extract to method for creating a column.

    table.getColumns().clear();
    table.getColumns().add(leftColumn);
    table.getColumns().add(rightColumn);

Setting up the table. Might be worth its own method after cleaning the rest, then again it might not. Given how you handle the buttons, I'd just leave it.

    table.getSelectionModel().setSelectionMode(SelectionMode.SINGLE);
    table.setColumnResizePolicy(TableView.CONSTRAINED_RESIZE_POLICY);

More... table... stuff. Unclear what it does. It's separated from the other table setup, but it seems like it belongs with the column adding. Possibly wrap the table setup in a method anyway?

    table.setOnKeyPressed(event -> {
        if (event.getCode() == KeyCode.ENTER && table.getSelectionModel().getSelectedItem() != null) {
            translationRequestListeners.forEach(listener -> {
                // so many assumptions :/
                listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
            });
        }
    });

Key press listener for the table.

Also,

            translationRequestListeners.forEach(listener -> {
                // so many assumptions :/
                listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
            });

and

              translationRequestListeners.forEach(listener -> {
                  listener.accept(table.getSelectionModel().getSelectedItem().getRight().getKey());
              });

show duplication. Back to the key press listener however, this could be part of the table setup method.

    table.setEditable(false);

This also belongs to setting up the table.

The annoying part (pet peeve of mine) is that it's not sure if any of this code, aside from the argument validation, needs to be executed in this order. Does it matter in what order the buttons are given their functionality? Does it matter if you first declare the table to not be editable, and then set up the columns and the selection mode? That's more a criticism of GUI code than it is a criticism of your code. More relevant; because there is no strict order, it's hard to find stuff. If you relocate the code to separate functions, you'll find the table code in the table setup function, rather than "about halfway down initialize".

I'd want to see an initialize function which does nothing more than argument validation, initializeButtons() and initializeTable().

---

              private void assignHighlightClasses(TranslationPair rowValue) {
                  getStyleClass().remove("default");
                  getStyleClass().remove("warn");
                  getStyleClass().remove("error");

                  switch (NotableData.assessNotability(rowValue)) {
                      case INFO:
                      case DEFAULT:
                          getStyleClass().add("default");
                          break;
                      case WARNING:
                          getStyleClass().add("warn");
                          break;
                      case ERROR:
                          getStyleClass().add("error");
                          break;
                  }
              }

I am not a fan of all the magic strings here. Get a lookup table, then getStyleClass().add(lookupTableForStyleClasses.get(notability)) (maybe with assignment to variable and null check first, but still).