3
\$\begingroup\$

I'm currently filtering a list based on several conditions, which may or may not be present:

    def requests = Request
        .list()
        .findAll { if (exportFrom) { it.tsCreated.millis >= exportFrom.millis } else { it } }
        .findAll { if (exportTo) {  it.tsCreated.millis <= exportTo.millis } else { it } }
        .findAll { if (clientId) { it.resolvedClient?.clientId == clientId } else { it } }
        .findAll { if (params.exportType) { it.typeName == params.exportType } else { it } }
        .findAll { if (params.exportStatus) { it.status.toString() == params.exportStatus } else { it } }

For instance, exportFrom is a date coming from the user interface. If it's present, we only want to export requests that occurred on that date or later. If not, we can disregard that test.

it will never be 0 or "", as it from a list of objects that have been loaded from the database and so have already been validated.

While this is readable and works, it seems like a poor solution and probably won't be very performant on a large dataset. Is there a Groovy idiom for this kind of series of selective filters?

\$\endgroup\$
7
  • \$\begingroup\$ So you check to see if a criteria object is valid and then apply a check (which is not actually contained in the criteria object, so it isn't really a criterion object at all) or, if the object is not valid, you use the element being matched as the basis of truth? That's both very non-OO (nor good functional style) and very fragile. Do you realise that if the element is, for example 0 or "" then {it} will evaluate, in this context, to false? So [0,1,2,3,4] would return [1,2,3,4] which is probably not what you intended. \$\endgroup\$ – itsbruce Mar 4 '15 at 23:48
  • \$\begingroup\$ Is that really working code or just an example of your approach? It looks like dummy code, which is off topic here. \$\endgroup\$ – itsbruce Mar 4 '15 at 23:49
  • \$\begingroup\$ It's working code, it's just been anonymized. I'll go ahead and post the real code. \$\endgroup\$ – Charles Wood Mar 4 '15 at 23:59
  • \$\begingroup\$ OK, I see you also answered my first question a bit. But if the Request object is never going to evaluate to false, why not just return true? Cheaper and eliminates a potential error. Of course, null evaluates to false in this context in Groovy. Is that what you are after? \$\endgroup\$ – itsbruce Mar 5 '15 at 0:05
  • \$\begingroup\$ Well, this is my first approach. I just knew I needed to return something in order to get an object back if the test didn't apply. Is this still a bad approach? If so, what is a better one? \$\endgroup\$ – Charles Wood Mar 5 '15 at 0:07
6
\$\begingroup\$

Few problems here.

  1. Fragility and possible inefficiency by returning {it} rather than true
  2. Iterating over the collection more often than is necessary (even though the later iterations may be over smaller sets).
  3. Duplication of the same pattern

Point 1: Fragility

Just return true (oh, and not { true }). That's what you want. By returning the object you risk some odd circumstance where the object evaluates to false. Why expose yourself to that risk? And returning true is just cheaper.

Point 2: Repeated iteration

Each call to findAll is returning a new collection, which you iterate over with the next call. That's a waste. You can do it with one findAll and one closure. The simplest (and most naive) way to do that would be this:

def requests = Request
    .list()
    .findAll { (exportFrom ? it.tsCreated.millis >= exportFrom.millis : true)
        && (exportTo ? it.tsCreated.millis <= exportTo.millis : true)
        && (clientId ? it.resolvedClient?.clientId == clientId : true)
        && (params.exportType ? it.typeName == params.exportType : true)
        && (params.exportStatus ? it.status.toString() == params.exportStatus : true)
        && true }

That will do just what your code does only without creating four intermediate copies of the list. It is, however, ugly. So one approach would be to turn each of those criteria into a function which takes a parameter and returns true or false. So you would have

def requests = Request
    .list()
    .findAll { cond1(it) && cond2(it) && cond3(it) && cond4(it) && cond5(it) }

Which is at least cleaner. However, let's consider

Point 3: Duplication of a pattern

You have a set of basic conditions. Each, if true, has a matching predicate with which you test the list item for validity. If the condition is not valid/present, you accept the list item. So...

Imagine you create a Criteria class. It should have two properties

  1. criterion: the condition which, if true, requires a list element to be tested. Should probably be a closure.
  2. predicate: the test (another closure) to apply to the list element

The Criteria class should also have a reject method which might look like this

    boolean reject (Object item) {
        if (this.criterion(item)) ! this.predicate(item) else false
    }

(I used Object because I'm not sure what type is actually in your list)

Now you can create a list of Criteria objects, each containing a criterion and a matching test. Lets say you call it criteriaList. Now the main bit of code can look like this:

def requests = Request
    .list()
    .findAll { x -> ! criteriaList.find { c -> c.reject(x) } }

If none of the criteria objects reject the item, then the find call returns null (empty list) and ! null evaluates to true. No rejections, so we like this item.

On the other hand, if a list item is going to fail one of the tests, then find will return the first Criteria object in the list which rejected the item (and not try any others after that). So in that case ! criteriaList.find will evaluate to false and the item will be discarded.

Can you see why I used a reject method (which returns true if the test fails) rather than an accept method (which would return true if the test succeeds)? I did that because find returns as soon as it finds a match, so we want to return as soon as one Criteria object rejects the list item.

There are other ways to do this (e.g. closure composition, although that's a more functional approach which you said isn't your style) but I hope this is a good example which will help you think of your own solutions.

\$\endgroup\$
4
  • \$\begingroup\$ Hmm, the chained else ifs in point 2 don't actually recreate the functionality. They pass anything that meets one criterion, but if multiple criteria are present, they all have to pass. I'll have to try the series of and'ed functions. \$\endgroup\$ – Charles Wood Mar 5 '15 at 23:47
  • \$\begingroup\$ FYI, the &&-ed functions (actually closures) worked. \$\endgroup\$ – Charles Wood Mar 6 '15 at 0:19
  • \$\begingroup\$ But I do recommend the final OO/functional suggestion. It is less fragile, much more flexible and actually creates cleaner code. You could make it even more OO by creating a class which contains a criteria list and and an accept method which does the "run it through the list till something rejects it" bit. \$\endgroup\$ – itsbruce Mar 6 '15 at 10:49
  • 1
    \$\begingroup\$ As for the naive example (which I didn't originally check thoroughly because I would never use it), I've rewritten it with ternary operators and chained && operators. Works as intended, now. But still don't do it. Do the OO/functional technique ;) \$\endgroup\$ – itsbruce Mar 6 '15 at 14:35

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.