2
\$\begingroup\$

I'd like to put some of my code under critique. The following code will take a supplier for elements and a filter function. Once provided -> the elements will be held by the model until "refresh" is called. Changing the filter will cause filtering.

Model changes will be published by a listener pattern like:

  • A listener registered and will get the current state
  • The filter changed
  • A refresh request was made

import java.util.HashSet;
import java.util.Iterator;
import java.util.Objects;
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.Supplier;

public class FilterModel<T, FILTERDEF> {


    public interface Listener<T, FILTERDEF> {

        void onFilterChanged(FILTERDEF filterDef, Set<T> filteredElements);

        void onRefreshedElements(Set<String> filteredElements);

        void onRegisteredAsListener(FILTERDEF filterDef, Set<T> filteredElements);

    }


    private FILTERDEF filterDef;
    private Set<T> allElements;
    private Set<T> filteredElements;
    private Supplier<Set<T>> supplierElements;
    private BiFunction<T, FILTERDEF, Boolean> filter;


    private Set<Listener<T, FILTERDEF>> listeners;


    public FilterModel(Supplier<Set<T>> supplierElements, BiFunction<T, FILTERDEF, Boolean> filter) {
        this.listeners = new HashSet<>();
        this.supplierElements = supplierElements;
        this.filter = filter;
    }


    public void filter(FILTERDEF filterDef) {

        if (!Objects.equals(filterDef, this.filterDef)) {

            this.filterDef = filterDef;

            applyFilter();

        }

    }


    private void applyFilter() {

        this.getFilteredElements().clear();

        Iterator<T> iterator = this.getAllElements().iterator();

        while (iterator.hasNext()) {
            T element = iterator.next();
            if (this.filter.apply(element, this.filterDef)) {
                this.getFilteredElements().add(element);
            }
        }

        this.listeners.forEach((listener) -> listener.onFilterChanged(this.filterDef, new HashSet<>(this.getFilteredElements())));

    }


    private Set<T> getAllElements() {
        if (this.allElements == null) {
            this.allElements = new HashSet<>();
            this.allElements.addAll(supplierElements.get());
        }

        return this.allElements;
    }


    private Set<T> getFilteredElements() {
        if (this.filteredElements == null) {
            this.filteredElements = new HashSet<>(this.getAllElements());
        }

        return this.filteredElements;
    }


    public void addListener(Listener<T, FILTERDEF> listener) {
        this.listeners.add(listener);
        listener.onRegisteredAsListener(this.filterDef, new HashSet<>(this.getFilteredElements()));
    }


    public void removeListener(Listener<T, FILTERDEF> searchModelListener) {
        this.listeners.remove(searchModelListener);
    }


    public void refresh() {
        this.allElements = null;
        this.filteredElements = null;
        this.applyFilter();
        this.listeners.forEach(listener -> listener.onFilterChanged(this.filterDef, getFilteredElements()));
    }


}

Example of usage:

public class Main {

    public static void main(String[] args) {

        Set<String> elements = new HashSet<>();

        elements.add("B");
        elements.add("BBC");
        elements.add("BBCCC");
        elements.add("CCC");

        BiFunction<String, String, Boolean> elementsFilter = (string, filterString) -> string.startsWith(filterString);
        Supplier<Set<String>> elementsProvider = () -> elements;

        FilterModel<String, String> filterModel = new FilterModel<String, String>(elementsProvider, elementsFilter);

        filterModel.filter("BBC");

        filterModel.addListener(new Listener<String, String>() {

            @Override
            public void onRegisteredAsListener(String filterDef, Set<String> filteredElements) {
                System.out.println("Initial filter:" + filterDef);
                for (String string : filteredElements) {
                    System.out.println(string);
                }
            }

            @Override
            public void onFilterChanged(String filterDef, Set<String> filteredElements) {
                System.out.println("Filter changed to:" + filterDef);
                for (String string : filteredElements) {
                    System.out.println(string);
                }
            }

            @Override
            public void onRefreshedElements(Set<String> filteredElements) {
                System.out.println("Elements changed with same filter:");
                for (String string : filteredElements) {
                    System.out.println(string);
                }
            }


        });

        filterModel.filter("B");

        elements.add("BX");

        filterModel.refresh();

    }

}
\$\endgroup\$
1
\$\begingroup\$

Method names

I found it a bit hard to understand all the methods, mainly because they do something I did not expect.

For example getAllElements() is an internal method that lazily fill the elements but is not exposed, there is a filter and a applyFilter method.

Accessing filteredElements

Currently, the only way to accessing the filteredElements is by registring a listener. I'd expect a getter? Is there a design choice fro not allowing them to be accessed directly?

When is a listener called?

Besides that, I don't really understand the listener onFilterChanged. I expect that to be called on the filter() method. This makes more sense, because the refresh() does not change the filter itself.

Missed nice stream() opportunity

I'd do this:

private void applyFilter() {

    this.filteredElements = this.getAllElements().stream().filter (f -> this.filter.apply(f, this.filterDef)).collect(Collectors.toSet());
}

(Note this generates a new Set, something that might not be desired)

Type name

You currently use the generic type name FILTERDEF. While valid, it is custom to use a single character, I would choose F here.

\$\endgroup\$
1
  • \$\begingroup\$ "Is there a design choice fro not allowing them to be accessed directly?". Yes. Listener Pattern with push concept. The other way around would be providing no additonal data and the interesting object will have to "pull" the information with "getters". \$\endgroup\$ – oopexpert Nov 27 '17 at 15:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.