Is Java Reflection bad practice?

Question

Consider this piece of code:

public void doSearch(ActionEvent event) {
    String query = searchTextField.getText();
    if (query.isEmpty()) {
        data = FXCollections.observableArrayList(dc.getJobCoachRepo().getList());
        usersTableView.setItems(data);
    } else {

        String searchOn = "search" + searchChoiceBox.getValue();
        try {
            Method m = this.getClass().getMethod(searchOn, String.class);
            m.invoke(this, query);
        } catch (Exception e) {

        }
    }
}

public void searchFirstName(String query) {
    data = FXCollections.observableArrayList(dc.getJobCoachRepo().searchFirstName(query));
    usersTableView.setItems(data);

}
...
...

I'm using java reflection here to avoid an if construct. The choicebox is used to let the user decide on what attribute he wants to search, there are 6 possibilities right now. I've gotten some comments from other students that using reflection is 'bad practice'. Is this the case? Why?

Answer 1

There are many reasons this is bad practice. Among them:

1. It is not robust. The text that the user types in the text field has to match the name of a method, which implies a horrendous level of coupling between things that should not be related at all
2. Reflection performs badly. Probably not a big deal here, but if there is no good reason to use reflection, you shouldn't.
3. There is a ready-made better solution using lambda expressions.

Consider instead populating your combo box with Consumer<String> objects:

ComboBox<Consumer<String>> searchChoiceBox = new ComboBox<>();
searchChoiceBox.getItems().add(createSearchOption(this::searchFirstName, "First Name"));

// ...

private Consumer<String> createSearchOption(Consumer<String> search, String name) {
    return new Consumer<String>() {
        @Override
        public void accept(String s) {
            search.accept(s);
        }
        @Override
        public String toString() {
            return name ;
        }
    };
}

Then you just do:

public void doSearch(ActionEvent event) {
    String query = searchTextField.getText();
    if (query.isEmpty()) {
        data = FXCollections.observableArrayList(dc.getJobCoachRepo().getList());
        usersTableView.setItems(data);
    } else {

        searchChoiceBox.getValue().accept(query);

    }
}

Answer 2

Yes, reflection is slow, and it creates fragile code when used in this way.

If you want to avoid the if statement, you should use polymorphism. Create an interface Searcher with public void search(String query), create implementations for every type of search you want to do, and then put an instance of each implementation as the value of a Map<String, Searcher>, keyed by the value of the search choice box.

Because Java enums are objects, you could also use an enum as your map. Each enum value would define their own search(string) implementation. You could then call the implementation you want using SearchEnumTypeName.valueOf(searchChoiceBox.getValue()).search(query)