6
\$\begingroup\$

In my application, I have a small hierarchy of related classes (partner categories - partners - partner points).

The application has two representations of its content:

  • in the form of partners points on the map
  • in the form of a list of partner categories or partners within a category

The application has the filters for searching and sorting. The map used only the search filters, but in the list used both types of filters.

Please, criticize my code and help me choose a proper name instead of SearchableSortable.

This class was written using TDD.

SearchableSortable.java

public class SearchableSortable<T> implements Serializable {

    public static interface OnEachHandler<T> {
        void onEach(T each, boolean meetsCriteria);
    }

    public static interface SearchCriteria<T> {
        boolean meetCriteria(T obj);
    }

    private final List<T> elements;
    private final List<SearchCriteria<T>> searchCriterias;
    private final List<Comparator<T>> comparators;


    public static <T> SearchableSortable<T> newInstance(Collection<T> elements) {
        return new SearchableSortable<T>(elements);
    }

    private SearchableSortable(Collection<T> elements) {
        this.elements = new ArrayList<T>(elements);
        this.searchCriterias = new ArrayList<SearchCriteria<T>>();
        this.comparators = new ArrayList<Comparator<T>>();
    }

    public void addSearchCriteria(SearchCriteria<T> searchCriteria) {
        searchCriterias.add(searchCriteria);
    }

    public void addComparator(Comparator<T> comparator) {
        comparators.add(comparator);
    }

    public void clear() {
        searchCriterias.clear();
        comparators.clear();
    }

    public void forEach(OnEachHandler<T> onEachHandler) {
        for (T each : elements) {
            onEachHandler.onEach(each, meetCriteria(each));
        }
    }

    private boolean meetCriteria(T element) {
        for (SearchCriteria<T> searchCriteria : searchCriterias) {
            if (!searchCriteria.meetCriteria(element)) {
                return false;
            }
        }
        return true;
    }

    public void sort() {
        sort(elements);
    }

    private void sort(List<T> list) {
        for (Comparator<T> comparator : comparators) {
            Collections.sort(list, comparator);
        }
    }

    public List<T> toUnsortedList() {
        List<T> list = new ArrayList<T>();
        for (T each : elements) {
            if (meetCriteria(each)) {
                list.add(each);
            }
        }
        return list;
    }

    public List<T> toSortedList() {
        List<T> list = toUnsortedList();
        sort(list);
        return list;
    }
}

TestSearchableSortable.java

public class TestSearchableSortable extends TestCase {
    private final Integer[] array = { 1, 2, 3, 4, 5, 6, 7, 8, 9 };

    private List<Integer> collection;
    private SearchableSortable<Integer> searchableSortable;
    private int index;

    @Override
    public void setUp() throws Exception {
        super.setUp();
        collection = Arrays.asList(array);
        searchableSortable = SearchableSortable.newInstance(collection);
    }


    public void testClassUsesCopyOfInputCollection() {
        List<Integer> before = new ArrayList<Integer>(collection);
        searchableSortable.addComparator(new ReverseOrderComparator());
        searchableSortable.addSearchCriteria(new OnlyEvenNumbersSearchFilter());
        searchableSortable.sort();
        searchableSortable.forEach(new SearchableSortable.OnEachHandler<Integer>() {
            @Override
            public void onEach(Integer each, boolean meetsCriteria) {
                // do nothing
            }
        });
        assertEquals(before, collection);
    }


    public void testForEachWithoutSearchFilterAndComparator() {
        checkForEach(new SearchableSortable.OnEachHandler<Integer>() {
            @Override
            public void onEach(Integer each, boolean meetsCriteria) {
                assertEquals(array[index], each);
                assertTrue(meetsCriteria);
            }
        });
    }


    public void testForEachWithSearchFilter() {
        searchableSortable.addSearchCriteria(new OnlyEvenNumbersSearchFilter());
        checkForEach(new SearchableSortable.OnEachHandler<Integer>() {
            @Override
            public void onEach(Integer each, boolean meetsCriteria) {
                assertEquals(array[index], each);
                assertEquals(evenNumber(each), meetsCriteria);
            }
        });
    }

    private static class OnlyEvenNumbersSearchFilter implements SearchableSortable.SearchCriteria<Integer> {
        @Override
        public boolean meetCriteria(Integer number) {
            return evenNumber(number);
        }
    }

    private static boolean evenNumber(Integer number) {
        return number % 2 == 0;
    }

    private void checkForEach(final SearchableSortable.OnEachHandler<Integer> onEachHandler) {
        index = 0;
        searchableSortable.forEach(new SearchableSortable.OnEachHandler<Integer>() {
            @Override
            public void onEach(Integer each, boolean meetsCriteria) {
                onEachHandler.onEach(each, meetsCriteria);
                ++index;
            }
        });
        assertEquals(array.length, index);
    }

    public void testForEachWithSeveralSearchFilters() {
        searchableSortable.addSearchCriteria(new OnlyEvenNumbersSearchFilter());
        final Integer[] numbers = { 2, 8, 120, 133, 1 };
        searchableSortable.addSearchCriteria(new OnlyTheseNumbersSearchFilter(numbers));
        checkForEach(new SearchableSortable.OnEachHandler<Integer>() {
            @Override
            public void onEach(Integer each, boolean meetsCriteria) {
                assertEquals(array[index], each);
                boolean expectedMeetsCriteria = (evenNumber(each) && arrayContainsElement(numbers, each));
                assertEquals(expectedMeetsCriteria, meetsCriteria);
            }
        });
    }

    private static class OnlyTheseNumbersSearchFilter implements SearchableSortable.SearchCriteria<Integer> {
        private final Integer[] numbers;

        OnlyTheseNumbersSearchFilter(Integer... numbers) {
            this.numbers = numbers;
        }

        @Override
        public boolean meetCriteria(Integer element) {
            return arrayContainsElement(numbers, element);
        }
    }

    private static <T> boolean arrayContainsElement(T[] arr, T element) {
        for (T each : arr) {
            if (each.equals(element)) {
                return true;
            }
        }
        return false;
    }


    public void testForEachWithComparator() {
        searchableSortable.addComparator(new ReverseOrderComparator());
        searchableSortable.sort();
        checkForEach(new SearchableSortable.OnEachHandler<Integer>() {
            @Override
            public void onEach(Integer each, boolean meetsCriteria) {
                assertEquals(getElementFromReversedArray(index), each);
                assertTrue(meetsCriteria);
            }
        });
    }

    private static class ReverseOrderComparator implements Comparator<Integer> {
        @Override
        public int compare(Integer number1, Integer number2) {
            return -(number1 - number2);
        }
    }

    private Integer getElementFromReversedArray(int index) {
        int lastIndex = array.length - 1;
        return array[lastIndex - index];
    }

    private static class PairOfNumbers extends Pair<Integer, Integer> {
        public PairOfNumbers(Integer first, Integer second) {
            super(first, second);
        }
    }

    public void testForEachWithSeveralComparators() {
        final List<PairOfNumbers> pairs = Arrays.asList(
                new PairOfNumbers(2, 4),
                new PairOfNumbers(6, 3),
                new PairOfNumbers(1, 4),
                new PairOfNumbers(2, 3)
        );
        final List<PairOfNumbers> expectedPairsAfterSorting = Arrays.asList(
                new PairOfNumbers(1, 4),
                new PairOfNumbers(2, 3),
                new PairOfNumbers(2, 4),
                new PairOfNumbers(6, 3)
        );
        SearchableSortable<PairOfNumbers> searchableSortablePairs = SearchableSortable.newInstance(pairs);
        searchableSortablePairs.addComparator(createComparatorBySecondComponent());
        searchableSortablePairs.addComparator(createComparatorByFirstComponent());
        index = 0;
        searchableSortablePairs.sort();
        searchableSortablePairs.forEach(new SearchableSortable.OnEachHandler<PairOfNumbers>() {
            @Override
            public void onEach(PairOfNumbers each, boolean meetsCriteria) {
                assertEquals(expectedPairsAfterSorting.get(index), each);
                assertTrue(meetsCriteria);
                ++index;
            }
        });
        assertEquals(expectedPairsAfterSorting.size(), index);
    }

    private Comparator<PairOfNumbers> createComparatorBySecondComponent() {
        return new Comparator<PairOfNumbers>() {
            @Override
            public int compare(PairOfNumbers pair1, PairOfNumbers pair2) {
                return pair1.second - pair2.second;
            }
        };
    }

    private Comparator<PairOfNumbers> createComparatorByFirstComponent() {
        return new Comparator<PairOfNumbers>() {
            @Override
            public int compare(PairOfNumbers pair1, PairOfNumbers pair2) {
                return pair1.first - pair2.first;
            }
        };
    }


    public void testToSortedListWithoutSearchFilterAndComparator() {
        assertEquals(collection, searchableSortable.toSortedList());
    }

    public void testToSortedListWithSearchFilter() {
        SearchableSortable.SearchCriteria<Integer> searchCriteria = new OnlyEvenNumbersSearchFilter();
        searchableSortable.addSearchCriteria(searchCriteria);
        Collection<Integer> numbersWhichMeetCriteria = getNumbersWhichMeetCriteria(collection, searchCriteria);
        assertEquals(numbersWhichMeetCriteria, searchableSortable.toSortedList());
    }

    private Collection<Integer> getNumbersWhichMeetCriteria(Collection<Integer> numbers,
                                        SearchableSortable.SearchCriteria<Integer>... searchCriterias) {
        Collection<Integer> numbersWhichMeetCriteria = new ArrayList<Integer>();
        for (Integer each : numbers) {
            if (meetCriteria(each, searchCriterias)) {
                numbersWhichMeetCriteria.add(each);
            }
        }
        return numbersWhichMeetCriteria;
    }

    private boolean meetCriteria(Integer each, SearchableSortable.SearchCriteria<Integer>... searchCriterias) {
        for (SearchableSortable.SearchCriteria<Integer> searchCriteria : searchCriterias) {
            if (!searchCriteria.meetCriteria(each)) {
                return false;
            }
        }
        return true;
    }


    public void testToSortedListWithSeveralSearchFilters() {
        SearchableSortable.SearchCriteria<Integer> onlyEvenNumbersSearchCriteria = new OnlyEvenNumbersSearchFilter();
        SearchableSortable.SearchCriteria<Integer> onlyTheseNumbersSearchCriteria = new OnlyTheseNumbersSearchFilter(2, 4, 6);
        searchableSortable.addSearchCriteria(onlyEvenNumbersSearchCriteria);
        searchableSortable.addSearchCriteria(onlyTheseNumbersSearchCriteria);
        Collection<Integer> numbersWhichMeetCriteria = getNumbersWhichMeetCriteria(collection,
                onlyEvenNumbersSearchCriteria, onlyTheseNumbersSearchCriteria);
        assertEquals(numbersWhichMeetCriteria, searchableSortable.toSortedList());
    }


    public void testToListWithComparator() {
        searchableSortable.addComparator(new ReverseOrderComparator());
        assertEquals(reversed(collection), searchableSortable.toSortedList());
    }

    private List<Integer> reversed(List<Integer> list) {
        List<Integer> reversedList = new ArrayList(list.size());
        for (int i = list.size() - 1; i >= 0; --i) {
            reversedList.add(list.get(i));
        }
        return reversedList;
    }


    public void testToSortedListWithSeveralComparators() {
        final List<PairOfNumbers> pairs = Arrays.asList(
                new PairOfNumbers(2, 4),
                new PairOfNumbers(6, 3),
                new PairOfNumbers(1, 4),
                new PairOfNumbers(2, 3)
        );
        final List<PairOfNumbers> expectedPairs = Arrays.asList(
                new PairOfNumbers(1, 4),
                new PairOfNumbers(2, 3),
                new PairOfNumbers(2, 4),
                new PairOfNumbers(6, 3)
        );
        SearchableSortable<PairOfNumbers> searchableSortablePairs = SearchableSortable.newInstance(pairs);
        searchableSortablePairs.addComparator(createComparatorBySecondComponent());
        searchableSortablePairs.addComparator(createComparatorByFirstComponent());

        assertEquals(expectedPairs, searchableSortablePairs.toSortedList());
    }


    public void testRemoveAllFilters() {
        final List<PairOfNumbers> pairs = Arrays.asList(
                new PairOfNumbers(2, 4),
                new PairOfNumbers(6, 3),
                new PairOfNumbers(1, 4),
                new PairOfNumbers(2, 3),
                new PairOfNumbers(1, 1),
                new PairOfNumbers(5, 5),
                new PairOfNumbers(2, 2)
        );
        SearchableSortable<PairOfNumbers> searchableSortablePairs = SearchableSortable.newInstance(pairs);
        searchableSortablePairs.addSearchCriteria(new SearchableSortable.SearchCriteria<PairOfNumbers>() {
            @Override
            public boolean meetCriteria(PairOfNumbers obj) {
                return obj.first != obj.second;
            }
        });
        searchableSortablePairs.addComparator(createComparatorBySecondComponent());
        searchableSortablePairs.addComparator(createComparatorByFirstComponent());

        searchableSortablePairs.clear();

        index = 0;
        searchableSortablePairs.forEach(new SearchableSortable.OnEachHandler<PairOfNumbers>() {
            @Override
            public void onEach(PairOfNumbers each, boolean meetsCriteria) {
                assertEquals(pairs.get(index), each);
                assertTrue(meetsCriteria);
                ++index;
            }
        });
        assertEquals(pairs.size(), index);

        assertEquals(pairs, searchableSortablePairs.toSortedList());
    }
}

Filter classes

public interface Filter<T> {
    void includeIn(SearchableSortable<T> searchableSortable);
}

public abstract class SearchFilter<T> implements Filter<T>, SearchableSortable.SearchCriteria<T> {
    @Override
    public void includeIn(SearchableSortable<T> searchableSortable) {
        searchableSortable.addSearchCriteria(this);
    }
}

public abstract class SortingFilter<T> implements Filter<T>, Comparator<T> {
    @Override
    public void includeIn(SearchableSortable<T> searchableSortable) {
        searchableSortable.addComparator(this);
    }
}

Or maybe I should divide SearchableSortable class into 2 classes:

Searchable and SearchableSortable (based on Searchable class) and replace the methods toUnsortedList() and toSortedList() by single method toList() (which returns unsorted list in Searchable class and returns sorted list in SearchableSortable class).

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Your class feels wrongly named. The first sign of a problem is that "-able" and "-ible" suffixes are generally used for interfaces rather than classes. Let's see what it is really trying to do:

  • It represents an ordered collection of objects of type T (private final List<T> elements)
  • It associates several search criteria with the collection (private final List<SearchCriteria<T>> searchCriterias)

    By the way, criterion is singular ; criteria is its plural form.

  • It associates several sorting keys with the collection (private final List<Comparator<T>> comparators)

I would call the class SearchableListing<T>. I chose "listing" instead of "list" because it merely helps enumerate the items in the list; the class does not actually extend List<T>. I would leave "sortable" out of the name, as "listing" somewhat implies that the search results can be ordered by relevance.

The search criteria and comparator seem like they should be more intimately related. For example, if you search for partners located within n kilometers of a location, that implies that you also want to rank the search results by distance.

My suggestion for an interface would be:

public class SearchableListing<T> implements Iterable<T> {

    public static interface Criterion<T> extends Comparator<T> {
        public boolean filter(T t);
    }

    /**
     * Iterator that produces all elements that meet the search criteria,
     * sorted by the natural ordering for those search criteria.
     */
    private class ResultIterator<T> implements Iterator<T> {
        public ResultIterator(List<T> list, List<Criterion> crit) {
        }

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

        @Override
        public boolean hasNext() { /* TODO */ }

        @Override
        public T next() { /* TODO */ }
    }

    private Collection<T> elements;
    private List<Criterion> criteria;

    public void SearchableListing(Collection<T> elements) { /* TODO */ }

    public void addCriterion(Criterion c) { /* TODO */ }

    @Override
    public Iterator<T> iterator() {
        return new ResultIterator<T>(this.list, this.criteria);
    }
}

Example usage:

List<Partner> partners = getPartners();
SearchableListing<Partner> partnerListing = new SearchableListing(partners);
partnerListing.addCriterion(new DistanceFromPoint(here));
for (Partner p : partnerListing) {
    // Do stuff with p
}

One main difference, which may be either an advantage or disadvantage, is that you would avoid having multiple unmanaged Lists floating around, as the caller is forced to enumerate the results using your class.

That's just a suggestion; you'll probably have to adapt it for your situation as appropriate.

\$\endgroup\$
2
\$\begingroup\$

I am not particularly fussed with the name SearchableSortable. This is a generic class ... if it was directly linked to Partners, then it would be different. You could probably do something to indicate the underlying data infrastructure, like ListView which allows you to understand that the tool manipulates Lists to show different views of the data, and, in this case, two tools are available for modifying the View, sorting, and filtering.

Other things I think you should consider changing though, are:

  • your abstract classes (SearchingFilter<T>, SortingFilter<T>) and their underlying interface Filter<T> have the method includeIn(...). This type of double-abstraction leads to confusion and gnashing of teeth. The issue is that the way the instances get included in to the ListView is dependant on the instance type. These methods also add no value.... is it really a problem to have:

    listview.addSearchCriteria(criteria);
    listview.addComparator(comparator);
    

    Do you really also need

    criteria.includeIn(listview);
    comparator.includeIn(listview);
    

    The second (redundant) versions of the operation lead to confusion, you have to know what type of instance is being called in order to understand what the operation is doing.

    Remove the includeIn(*) redundancies.

  • clear() should, in theory, clear the elements data as well.

  • Adding multiple Comparators is likely going to lead to problems. The implementation you have will result in the last-comparator-added being used to sort the whole dataset. Is that what you want?
  • Serializable implies you can serialize all the data, but, in fact, you cant be sure that your <T> data type is serializable. You should probably define your class as <T extends Serializable> or remove the Serializable entirely from your class.

All in all, it looks like the code is relatively well structured, and it looks like you have some good ideas in there.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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