I was advised not to use reflection here... Why not?

Question

This code is used to sort a List. The list could contain in the thousands of elements but less than 10k.

protected <E> int compareFields(E o1, E o2, String fieldName){
    try { 
        Comparable o1Data = (Comparable) o1.getClass().getMethod(fieldName).invoke(o1);
        Comparable o2Data = (Comparable) o2.getClass().getMethod(fieldName).invoke(o2);
        return o1Data == null ? o2Data == null ? 0 : 1 :
               o2Data == null ? -1 : o1Data.compareTo(o2Data);
    } catch(Exception e) {
        throw new RuntimeException(e);
    }
}

I was advised to

"Please don't use reflection for things like this!! Either supply the method with a suitable Comparator, or a method to extract the relevant property (may be computed in a way not supported by the original type), or both."

An example of a better way to do this would be nice.

Context: I've got many screens with data tables. Each one is build from a List. Each data table needs to be sortable by each of its 6 columns. The columns are either Date or String.

Answer 1

Using reflection here will potentially be much slower, as you are adding number of stack frames to each comparison by using getClass, getMethod and invoke rather than using the objects' native compare method.

Ideally, you would write the method to avoid the use of object in the signature. A "suitable Comparator" would at least be strongly bound to the objects' type (which you assume are the same). If you must have dynamic field comparison (as it appears), then at least the reflection could be encapsulated in that comparator.

If you are going to call this thousands of times, though, it would be best to pre-bind a Comparator to the field you are sorting by. This way, you only call getMethod once up front, rather than once for each individual comparison.

Answer 2

It's hard to give a good example without context, so for now here's a small list of why it's not the best idea:

1. The field provided has no guarantee of being Comparable (not sure why the code here needs to catch that exception and rebrand it).
2. What if the type of objects provided are not meant to be compared in this way? (It's an overly generic method name to know how it's supposed to be used).
3. It's not strongly typed. Providing the field name as a string means you'll have to change your code everywhere whenever the property name changes, and it'll be hard to track down where you need to make those changes.
4. Reflection is potentially slower than if implemented in a strongly typed manner.

Answer 3

The other answers described well why it is not advised to use reflection. I want to add an example using a more conventional solution.

Instead of specifying the field that is used to compare the two objects, you should take a Comparator instance as an argument. This way the client who uses this method can specify how to compare the two objects.

protected <E> int compareFields(E o1, E o2, Comparator<E> comparator) {
    return comparator.compare(o1, o2);
}

And an example call to this function would look like this:

MyClass a = ...;
MyClass b = ...;
Comparator<MyClass> intFieldComparator = new Comparator<MyClass> {
    public int compare(MyClass o1, MyClass o2) {
        int field1 = o1.getIntField();
        int field2 = o2.getIntField();

        return field2 - field1;
    }
};

compareFields(a, b, intFieldComparator);

You can define different comparators if you want to compare the objects using several fields.