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 - UsageThis 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);}@Overridepublic 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 - SetupAs 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
Follow the Rubberduck - Part 3: JavaFX MVC and MVP Matrjoshka
java;javafx
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.@Overridepublic 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).