Code Refactoring of a GORM query

Question

I wrote a code to get some entities from my DB, and the code is quite ugly (duplication of code, etc.) and not very fast for big chunk of entities. The code gets all entities, then sort it (in memory), then take a range of items (for paginating the results).

There it is:

List<T> retrieveData() {
    def filterPastEvents = (params.filterPastEvents) ?: 0
    def entities = retrieveUnsortedData(filterPastEvents).sort {
        entity1, entity2 ->
            entity2.criterion1 <=> entity1.criterion1 ?:
                    entity1.criterion2 <=> entity2.criterion2 ?:
                            entity1.criterion3 <=> entity2.criterion3
    }
    if (params.offset != null && params.max != null) {
        return entities.subList(params.offset, Math.min(params.to+1,entities.size()))
    } else {
        return entities
    }
}

private List<Entity> retrieveUnsortedData(filterPastEvents) {

    List<Entity> entityList
    if (params.param1 && params.params2) {
        entityList = Entity.findAll({
            param1 == params.param1
            params2 == params.params2
            endTimeUTC >= filterPastEvents
        })
    }
    if (params.param1 && !params.params2) {
        entityList = Entity.findAll({
            param1 == params.param1
            endTimeUTC >= filterPastEvents
        })
    }
    if (params.params2 && !params.param1) {
        entityList = Entity.findAll({
            params2 == params.params2
            endTimeUTC >= filterPastEvents
        })
    }
    if (entityList == null) {
        entityList = Entity.findAll({
            endTimeUTC >= filterPastEvents
        })
    }
    return entityList
}

I figured it would be more efficient to do it with criterion and without duplicated code though this code works.
That's what I tried to simplify the code:

List<T> retrieveData() {

    def filterPastEvents = (params.filterPastEvents) ?: 0
    Closure closureOrder = {
        endTimeUTC >= filterPastEvents
        order('criteria1', 'desc')
        order('criteria2', 'asc')
        order('criteria3', 'desc')
    }
    Closure param1Closure = {}
    if (params.param1) {
        param1Closure = {
            param1 == params.param1
        }
    }
    Closure param2Closure = {}
    if (params.param2) {
        param2Closure = {
            param2 == params.param2
        }
    }
    return Entity.findAll(max: params.max, offset: params.offset).list {
        param1Closure
        param2Closure
        closureOrder
    }
}

My problems are, at this point:
the closure aren't 'run', condition on param1 and param2 is not correct.
and this code

  Closure param1Closure = {}
    if (params.param1) {
        param1Closure = {
            param1 == params.param1
        }
    }

is still duplicated with param2.

How can I compose closures depending on conditions (ie: params.param1?), I tried using Closure c << param1Closure but it doesn't seem to work.

Answer 1

Here's an easier way:

List<T> retrieveData() {
    Entity.findAll(max: params.max, offset: params.offset) {
        if(params.param1) param1 == params.param1
        if(params.param2) param2 == params.param2
        if(params.filterPastEvents) endTimeUTC >= filterPastEvent

        order 'criteria1', 'desc'
        order 'criteria2', 'asc'
        order 'criteria3', 'desc'
    }
}

The example above is quite concise, but if you really want to use composition, you can compose where queries. Something like this:

import grails.gorm.DetachedCriteria

List<T> retrieveData() {
    ...
    DetachedCriteria<Entity> param1Closure = (params.param1 ? { param1 == params.param1 } : { }) as DetachedCriteria<Entity>
    ...

    def query = Entity.where(param1Closure)

    query = query.where(param2Closure)
    query = query.where(closureOrder)

    return query.list(max: params.max, offset: params.offset)
}

The important key is that if you call where(Closure) with a variable as the closure, then the closure must be cast to a DetachedCriteria. A regular closure won't work.