Overloaded applyRestrictionsToCriteria(…) methods

Question

We want to refactor two methods that are exactly the same, except for one difference: one takes an org.hibernate.Criteria and the other org.hibernate.criterion.DetachedCriteria. These two do implement a mutual interface (org.hibernate.criterion.CriteriaSpecification), but this only contains some final static fields, and no methods.

Here are the methods (removed comments and javadoc for compactness):

public static DetachedCriteria applyRestrictionsToCriteria(final DetachedCriteria criteria,
      final Vector<RestrictionsHelper> filter) {
    final Map<String, DetachedCriteria> subCriteriaMap = new HashMap<>();
    if (filter != null) {
        final Iterator<RestrictionsHelper> itp = filter.iterator();
        while (itp.hasNext()) {
            final RestrictionsHelper restric = itp.next();
            if (restric.getClassname().equals("")) {

                final Iterator<Criterion> ir = restric.getCriterions().iterator();
                while (ir.hasNext()) {
                    final Criterion criterion = ir.next();
                    criteria.add(criterion);
                    if (criterion.toString().contains("Happening_fk")) {
                        criteria.setFetchMode("Happeningdetails", FetchMode.JOIN);
                    }
                }

                final Iterator<Order> or = restric.getOrders().iterator();
                while (or.hasNext()) {
                    criteria.addOrder(or.next());
                }
            } else {
                final String[] buff = restric.getClassname().split("\\.");

                DetachedCriteria subcriteria = criteria;
                String path = "";
                for (final String element : buff) {
                    final String[] name = getNameAndAlias(element);
                    path += name[0];
                    final DetachedCriteria exsubcriteria = subCriteriaMap.get(path);
                    if (exsubcriteria == null) {
                        subcriteria = subcriteria.createCriteria(name[0], name[1], CriteriaSpecification.LEFT_JOIN);
                        subCriteriaMap.put(path, subcriteria);
                    } else {
                        subcriteria = exsubcriteria;
                    }
                    path += ".";
                }

                final Iterator<Criterion> ir = restric.getCriterions().iterator();
                while (ir.hasNext()) {
                    subcriteria.add(ir.next());
                }

                final Iterator<Order> or = restric.getOrders().iterator();
                while (or.hasNext()) {
                    subcriteria.addOrder(or.next());
                }
            }
        }
    }
    return criteria;
}

and

public static Criteria applyRestrictionsToCriteria(final Vector<RestrictionsHelper> filter,
      final Criteria criteria) {
    final Map<String, Criteria> subCriteriaMap = new HashMap<String, Criteria>();
    if (filter != null) {
        final Iterator<RestrictionsHelper> itp = filter.iterator();
        while (itp.hasNext()) {
            final RestrictionsHelper restric = itp.next();
            if (restric.getClassname().equals("")) {

                final Iterator<Criterion> ir = restric.getCriterions().iterator();
                while (ir.hasNext()) {
                    final Criterion criterion = ir.next();
                    criteria.add(criterion);
                    if (criterion.toString().contains("Happening_fk")) {
                        criteria.setFetchMode("Happeningdetails", FetchMode.JOIN);
                    }
                }

                final Iterator<Order> or = restric.getOrders().iterator();
                while (or.hasNext()) {
                    criteria.addOrder(or.next());
                }
            } else {
                final String[] buff = restric.getClassname().split("\\.");

                Criteria subcriteria = criteria;
                String path = "";
                for (final String element : buff) {
                    final String[] name = getNameAndAlias(element);
                    path += name[0];
                    final Criteria exsubcriteria = subCriteriaMap.get(path);
                    if (exsubcriteria == null) {
                        subcriteria = subcriteria.createCriteria(name[0], name[1], CriteriaSpecification.LEFT_JOIN);
                        subCriteriaMap.put(path, subcriteria);
                    } else {
                        subcriteria = exsubcriteria;
                    }
                    path += ".";
                }

                final Iterator<Criterion> ir = restric.getCriterions().iterator();
                while (ir.hasNext()) {
                    subcriteria.add(ir.next());
                }

                final Iterator<Order> or = restric.getOrders().iterator();
                while (or.hasNext()) {
                    subcriteria.addOrder(or.next());
                }
            }
        }
    }
    return criteria;
}

Since both methods do the same, we obviously want to refactor it into one method.

---

Some things we've tried without result

1. Creating an interface (GenericCriteria) and two subclasses:

   public class OwnCriteria extends CriteriaImpl implements GenericCriteria
   

   and

   public class OwnDetachedCriteria extends DetachedCriteria implements GenericCriteria
   

   and use that interface in the method.

   Problem:

   • We use Criteria#createCriteria(String, String, int) which returns a new Subcriteria(this, String, String, int);. Because Subcriteria is a final class, we can't create our own sub-class and we also can't make a convert-constructor in our Own classes because there aren't getters for all required constructor-parameters.

2. Directly make an anonymous class from the interface (i.e. new BagGenericCriteria(){ @Override ... }

---

Possible work-around that will most likely work, but is rather ugly:

Using the shared interface (org.hibernate.criterion.CriteriaSpecification) as parameter and then use multiple instanceof checks for one or the other.

---

Notes

• We use Java 7 (so we can't use Java 8 features - for now)
• We use hibernate version 3.3.2.GA (so we can't use hibernate 4+ - for now)
• Some other parts of the code in the methods can be refactored as well, but right now we just want to have two exact identical methods (apart from the parameter used) refactored into one.

Answer 1

Vector vs List/ArrayList

Vector was retrofitted to be part of the Java Collections framework, and if you do not need the synchronization feature, you should update to the ArrayList class. In fact, you should opt for the List interface, so that callers of these methods can eventually be refactored to pass in other List implementations, like ArrayList, and these two methods only know they are dealing with Lists.

Indentation

public static DetachedCriteria applyRestrictionsToCriteria(DetachedCriteria criteria,
                    Vector<RestrictionsHelper> filter) {
    final Map<String, DetachedCriteria> subCriteriaMap = new HashMap<>();
    if (filter != null) {
        // processing goes here
    }
    return criteria;
}

If you do an early return from the null-check, you can reduce one level of indentation. You also eliminate the possibly redundant new HashMap<>() declaration too, when the null-check holds true:

public static DetachedCriteria applyRestrictionsToCriteria(DetachedCriteria criteria,
                    List<RestrictionsHelper> filter) { // changed Vector -> List
    if (filter == null) {
        return criteria;
    }
    Map<String, DetachedCriteria> subCriteriaMap = new HashMap<>();
    // processing goes here
    return criteria;
}

My take on final modifiers on method arguments and variables these days is that they are largely redundant, as long as you can easily observe that they are not carelessly reassigned. If you happen to come from a (programming) culture where this is done way too often, and thus you are introducing final to check such practices, then feel free to leave them in until such 'reminders' can be removed.

Looping via iteration

Another way of doing looping via iteration is to rely on the standard for-loop as such:

for (Iterator<RestrictionsHelper> helpers : filter.iterator(); helpers.hasNext(); ) {
    // more processing goes here
}

This scopes the Iterator to within the for-loop itself. The simpler way is to use the enhanced for-each loop:

for (RestrictionsHelper helper : filter) {
    // more processing goes here
}

Deduplicating code blocks, part 1

final Iterator<Order> or = restric.getOrders().iterator();
while (or.hasNext()) {
    subcriteria.addOrder(or.next());
}

Since this is done regardless of restric.getClassname().equals(""), you can perform it outside of the if-block (illustrating only for DetachedCriteria):

public static DetachedCriteria applyRestrictionsToCriteria(DetachedCriteria criteria,
                    List<RestrictionsHelper> filter) {
    if (filter == null) {
        return criteria;
    }
    Map<String, DetachedCriteria> subCriteriaMap = new HashMap<>();
    for (RestrictionsHelper helper : filter) {
        DetachedCriteria currentCriteria;
        if (helper.getClassname().isEmpty()) { // instead of String.equals("")
            currentCriteria = criteria;
            // some processing here
        } else {
            // some processing here
            // use currentCriteria instead of subcriteria
        }
        for (Order order : helper.getOrders()) {
            currentCriteria.addOrder(order);
        }
    }
    return criteria;
}

Declaring variables closer to usage

Now let's take a look at the Map declaration again:

Map<String, DetachedCriteria> subCriteriaMap = new HashMap<>();

It's only being used when RestrictionsHelper.getClassname() is not empty. In addition, the only thing that code block seems to be doing is to eventually have currentCriteria be the final DetachedCriteria (following the example from the previous section) after splitting the class name. This suggests that we can convert this block into a method:

private static DetachedCriteria processClassName(DetachedCriteria criteria, 
                                                    String className) {
    DetachedCriteria result = critera;
    StringBuilder path = new StringBuilder();
    Map<String, DetachedCriteria> subCriteriaMap = new HashMap<>();
    for (String element : className.split("\\.")) {
        String[] name = getNameAndAlias(element);
        path.append(name[0]);
        DetachedCriteria temp = subCriteriaMap.get(path.toString());
        if (temp == null) {
            result = result.createCriteria(name[0], name[1],
                                            CriteriaSpecification.LEFT_JOIN);
            subCriteriaMap.put(path.toString(), result);
        } else {
            result = temp;
        }
        path.append('.');
    }
    return result;
}

Deduplicating code blocks, part 2

The method in question now looks much shorter:

public static DetachedCriteria applyRestrictionsToCriteria(DetachedCriteria criteria,
                    List<RestrictionsHelper> filter) {
    if (filter == null) {
        return criteria;
    }
    for (RestrictionsHelper helper : filter) {
        DetachedCriteria currentCriteria;
        if (helper.getClassname().isEmpty()) {
            currentCriteria = criteria;
            for (Criterion criterion = helper.getCriterions()) {
                criteria.add(criterion);
                if (criterion.toString().contains("Happening_fk")) {
                    criteria.setFetchMode("Happeningdetails", FetchMode.JOIN);
                }
            }
        } else {
            currentCriteria = processClassName(criteria, helper.getClassname());
            for (Criterion criterion = helper.getCriterions()) {
                currentCriteria.add(criterion);
            }
        }
        for (Order order : helper.getOrders()) {
            currentCriteria.addOrder(order);
        }
    }
    return criteria;
}

Before we even move to the DetachedCriteria/Criteria discussion, we can simplify this method one step further as such:

public static DetachedCriteria applyRestrictionsToCriteria(DetachedCriteria criteria,
                    List<RestrictionsHelper> filter) {
    if (filter == null) {
        return criteria;
    }
    for (RestrictionsHelper helper : filter) {
        boolean isClassnameEmpty = helper.getClassname().isEmpty();
        DetachedCriteria currentCriteria = isClassnameEmpty ? criteria
                    : processClassName(criteria, helper.getClassname());
        for (Criterion criterion = helper.getCriterions()) {
            currentCriteria.add(criterion);
            if (isClassnameEmpty && criterion.toString().contains("Happening_fk")) {
                currentCriteria.setFetchMode("Happeningdetails", FetchMode.JOIN);
            }
        }
        for (Order order : helper.getOrders()) {
            currentCriteria.addOrder(order);
        }
    }
    return criteria;
}

Deduplicating code blocks, part 3

Finally, the DetachedCriteria/Criteria discussion. You pointed out that an instanceof check is one way. Another alternative is to be inspired by Guava's Functions and Java 8's BiFunction to have your bespoke implementation of a two-tuple 'processor'-like interface:

public interface OwnBiFunction<T, U> {
    T apply(T original, U name);
}

(BTW, a 'true' BiFunction will have a third generic type for the return type, but since we know we want an instance of T to be returned, we'll take a short-cut here.)

Then, modify processClassName() to accept this additional argument:

private static <T> T processClassName(T criteria, String className,
                                        OwnBiFunction<T, String[]> biFunction) {
    T result = critera;
    StringBuilder path = new StringBuilder();
    Map<String, T> subCriteriaMap = new HashMap<>();
    for (String element : className.split("\\.")) {
        String[] name = getNameAndAlias(element);
        path.append(name[0]);
        T temp = subCriteriaMap.get(path.toString());
        if (temp == null) {
            result = biFunction.apply(result, name);
            subCriteriaMap.put(path.toString(), result);
        } else {
            result = temp;
        }
        path.append('.');
    }
    return result;
}

The implementations for DetachedCriteria and Criteria are respectively:

private static final OwnBiFunction<DetachedCriteria, String[]> DETACHED_CRITERIA =
    new OwnBiFunction<>() {
        DetachedCriteria apply(DetachedCriteria original, String[] name) {
            return original.createCriteria(name[0], name[1], 
                                CriteriaSpecification.LEFT_JOIN);
        }
    };

private static final OwnBiFunction<Criteria, String[]> CRITERIA =
    new OwnBiFunction<>() {
        Criteria apply(Criteria original, String[] name) {
            return original.createCriteria(name[0], name[1], 
                                CriteriaSpecification.LEFT_JOIN);
        }
    };

// Usage for both types
// ...
boolean isClassnameEmpty = helper.getClassname().isEmpty();
DetachedCriteria currentCriteria = isClassnameEmpty ? criteria
                    : processClassName(criteria, helper.getClassname(),
                                        DETACHED_CRITERIA);
// ...

// ...
boolean isClassnameEmpty = helper.getClassname().isEmpty();
Criteria currentCriteria = isClassnameEmpty ? criteria
                    : processClassName(criteria, helper.getClassname(),
                                        CRITERIA);
// ...

Putting it altogether

You can have a processClassName() method:

private static <T> T processClassName(T criteria, String className,
                                        OwnBiFunction<T, String[]> biFunction) {
    T result = critera;
    StringBuilder path = new StringBuilder();
    Map<String, T> subCriteriaMap = new HashMap<>();
    for (String element : className.split("\\.")) {
        String[] name = getNameAndAlias(element);
        path.append(name[0]);
        T temp = subCriteriaMap.get(path.toString());
        if (temp == null) {
            result = biFunction.apply(result, name);
            subCriteriaMap.put(path.toString(), result);
        } else {
            result = temp;
        }
        path.append('.');
    }
    return result;
}

And finally, a single applyRestrictionsToCriteria() method that takes in some bespoke interfaces for the actual processing on a DetachedCriteria or Criteria type:

public static <T> T applyRestrictionsToCriteria(T criteria,
                    List<RestrictionsHelper> filter,
                    OwnBiFunction<T, String[]> biFunction,
                    OwnBiFunction<T, Criterion> criterionAdder,
                    Function<T, Void> fetchModeSetter, // this can be from Guava
                    OwnBiFunction<T, Order> orderAdder) {
    if (filter == null) {
        return criteria;
    }
    for (RestrictionsHelper helper : filter) {
        boolean isClassnameEmpty = helper.getClassname().isEmpty();
        T currentCriteria = isClassnameEmpty ? criteria
                    : processClassName(criteria, helper.getClassname(), 
                                        biFunction);
        for (Criterion criterion = helper.getCriterions()) {
            criterionAdder.apply(currentCriteria, criterion);
            if (isClassnameEmpty && criterion.toString().contains("Happening_fk")) {
                fetchModeSetter.apply(currentCriteria);
            }
        }
        for (Order order : helper.getOrders()) {
            orderAdder.apply(currentCriteria, order);
        }
    }
    return criteria;
}

Java 8

When you get the chance to upgrade to Java 8, it's relatively simple to 'upgrade' the method signature to the Java 8 types of BiFunction, BiConsumer and Consumer:

public static <T> T applyRestrictionsToCriteria(T criteria,
                    List<RestrictionsHelper> filter,
                    BiFunction<T, String[], T> biFunction,
                    BiConsumer<T, Criterion> criterionAdder,
                    Consumer<T> fetchModeSetter,
                    BiConsumer<T, Order> orderAdder) {
    // same method body as above,
    // except that BiConsumer's method is accept(T, U) instead of apply(T, U)
    // and Consumer's method is accept(T) instead of Guava's Function.apply(T)
}

An example call can be:

Criteria criteria = /* ... */;
Criteria result = applyRestrictionsToCriteria(criteria,
                        filter,
                        (v, name) -> v.createCriteria(name[0], name[1], 
                                                CriteriaSpecification.LEFT_JOIN),
                        (v, c) -> v.add(c),
                        v -> v.setFetchMode("Happeningdetails", FetchMode.JOIN),
                        (v, o) -> v.addOrder(o));