3
\$\begingroup\$

I wrote some code to filter a list in Java and I think there must be a much better way of doing it...

Essentially I have a web page with a jqGrid and I am passing the filtering parameters into the java back end and trying to duplicate them. I have done this by making parsing the rule json into a List of Rule objects which contain field name, condition to be applied and condition (data).

so something like

public class Rule {

    private String field;
    private String op; 
    private String data; 
    ...

At the moment strings are filtered on a contains and numbers by >, <, >= and <= although the string condition may change.

I wrote some very very quick code with loops and a remove list to avoid List modification issues. I must stress that this is awful and only here as a kind of pseudo code so that it shows what I'm trying to do...

private List<RowObject> ApplyjqTableFilter 
                (List<RowObject> unfilteredList, List<Rule> filterRules) {

        List<RowObject> filteredList = unfilteredList;
        List<RowObject> removeList = new ArrayList<>();
        Boolean addedFlag = false;

        for (RowObject cpsr : unfilteredList){
            addedFlag = false;
            for (Rule r : filterRules){
                switch (r.getField()) {
                    case "ref" :
                        if (!cpsr.getRef().contains(r.getData().toLowerCase()))
                            removeList.add(cpsr);
                            addedFlag = true;
                        break;

                ...
                similar cases for other String fields
                ...

                case "Amount" :
                    switch (r.getOp()) {
                        case "lt":
                            if(cpsr.getAmount() >= Double.parseDouble(r.getData())){
                                removeList.add(cpsr);
                                addedFlag = true;
                            }
                            break;
                        case "gt":
                            if(cpsr.getAmount() <= Double.parseDouble(r.getData())){
                                removeList.add(cpsr);
                                addedFlag = true;
                            }       
                            break;
                        case "le":
                            if(cpsr.getAmount() > Double.parseDouble(r.getData())){
                                removeList.add(cpsr);
                                addedFlag = true;
                            }       
                            break;
                        case "ge":
                            if(cpsr.getAmount() < Double.parseDouble(r.getData())){
                                removeList.add(cpsr);
                                addedFlag = true;
                            }
                            break;
                        }

                        ...
                        other value field cases
                        ...   
                    }
                    if (addedFlag==true) break;
                }
            }
        }

        filteredList.removeAll(removeList);
        return filteredList;
}

So once I had validated that this was the business requirement (lets try and justify this 'code' as Agile ;-) I tried to turn this into proper code. So far I have got to here but I think I am still missing a trick, probably approaching it from the wrong direction ? It works well in a timely and consistent way but it seems very clunky ?

private List<RowObject> ApplyjqTableFilter 
                (List<RowObject> unfilteredList, List<Rule> filterRules) {

        List<RowObject> filteredList = unfilteredList;

        for (Rule r : filterRules){
            switch (r.getField()) {
                    case "ref" :
                        filteredList = filteredList.stream()
                            .filter(cl -> cl.getRef().contains(r.getData().toLowerCase()))
                             .collect(Collectors.toList());
                        break;

                    ...
                    similar String cases for other fields
                    ...

                    case "Amount" :
                        switch (r.getOp()) {
                            case "lt":
                                filteredList = filteredList.stream()
                                    .filter(cl -> cl.getAmount() < Double.parseDouble(r.getData()))
                                        .collect(Collectors.toList());
                                break;
                            case "gt":
                                filteredList = filteredList.stream()
                                    .filter(cl -> cl.getAmount() > Double.parseDouble(r.getData()))
                                        .collect(Collectors.toList());
                                break;
                            case "le":
                                filteredList = filteredList.stream()
                                    .filter(cl -> cl.getAmount() <= Double.parseDouble(r.getData()))
                                        .collect(Collectors.toList());
                                break;
                            case "ge":
                                filteredList = filteredList.stream()
                                    .filter(cl -> cl.getAmount() >= Double.parseDouble(r.getData()))
                                        .collect(Collectors.toList());
                                break;
                        }
                        break;
                     ...
                     other value field cases
                     ...   
                    }
                }
        return filteredList;
}

Any suggestions ?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

With this code, you filter your list over and over for each rule you encounter, which seems sub-optimal to say the least. I'd rather construct a single rule, which gets applied to the list once. Here, you can use the Predicate combination via "and" as this is basically what you do with chained filtering.

Something along the lines of:

Predicate myFilter = x -> true; // start out with "always true"
for (Rule r : filterRules) {
    ...
    switch...
        case "ref":
            myFilter = myFilter.and(
                            cl -> cl.getRef().contains(r.getData().toLowerCase()));
...
finalList = filteredList.stream().filter(myFilter).collect(...);

Additionally, I'd try to get rid of this mega-switch statement. Ideas depend a bit on your concrete data types, but I could imagine to create an enum with constants for your comparisons, which can generically operate on comparable types or the like. Then you could use reflection / bean introspection to read the requested value and compare generically. Something like:

private enum Operators {
    LEQ {
        @Override
        public <T extends Comparable<T>> boolean test(T value, T comparisonValue) {
            return value.compareTo(comparisonValue) <= 0;
        }
    },
    LT {
        @Override
        public <T extends Comparable<T>> boolean test(T value, T comparisonValue) {
            return value.compareTo(comparisonValue) < 0;
        }
    },
       ...
    ;

    public abstract <T extends Comparable<T>> boolean test(T value, T comparisonValue);
}

Fetch the concrete enum constant via lookup by name, extract the value from the object via reflection, and create the comparison value by some form of String-to-whatever conversion. Then, simply apply the test.

\$\endgroup\$
1
  • \$\begingroup\$ That's great thanks. I think I've seen that way of building up a filter before but I'd completely forgotten about it and it works perfectly. Just what I was after! I'll try and incorporate the second idea later today / tomorrow. Thanks again. \$\endgroup\$ Commented Apr 24, 2017 at 13:52

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.