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

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 e

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, TableCell> cellRenderer =
      column -> {
          TableCell cell = new TableCell() {
              @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 leftColumn = new TableColumn<>("");
    leftColumn.setCellValueFactory(data -> new ObservableValueBase() {
        @Override
        public String getValue() {
            return data.getValue().getLeft().getValue();
        }
    });
    leftColumn.setCellFactory(cellRenderer);

    TableColumn rightColumn = new TableColumn<>("");
    rightColumn.setCellValueFactory(data -> new ObservableValueBase() {
        @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.

```
cho

StackExchange Code Review Q#132605, answer score: 5