Generic Comparator

Question

I have tried to make a generic comparator with the use of reflection, but I am not sure how efficient it is or if it is the right way to do so Also, I don't know how it will handle dates.

<T> List<?> sortList(List<T> l,String field,Class<T> c) throws NoSuchFieldException, SecurityException, IntrospectionException{ 
 final Field f = c.getDeclaredField(field);
 PropertyDescriptor propertyDescriptor = new 
 PropertyDescriptor(f.getName(),c);
 final Method getter = propertyDescriptor.getReadMethod();
 Collections.sort(l, new Comparator<T>() {
 public int compare(T e1, T e2) {
 int i=0;
 try {
 if(f.getType().isAssignableFrom(String.class)){
 i = getter.invoke(e1).toString().compareTo(getter.invoke(e2).toString());
 } 
 else if(f.getType().isAssignableFrom(int.class)){
 i = Integer.parseInt(getter.invoke(e1).toString()) - 
 Integer.parseInt(getter.invoke(e2).toString()); 
 } 
 else if(f.getType().isAssignableFrom(long.class)){
 i = (int)(Long.parseLong(getter.invoke(e1).toString()) - 
 Long.parseLong(getter.invoke(e2).toString())); 
 }
 else if(f.getType().isAssignableFrom(float.class)){
 if(Float.parseFloat(getter.invoke(e1).toString()) - 
 Float.parseFloat(getter.invoke(e2).toString()) < 0)
 i = -1;
 else if(Float.parseFloat(getter.invoke(e1).toString()) - 
 Float.parseFloat(getter.invoke(e2).toString()) > 0)
 i = 1;
 else 
 i = 0;
 } 
 } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
 e.printStackTrace();
 }
 return i;
 }
 });
 return l;
} 

• Timothy Truckle: A generic comparator does not make any sense since usually there is no generic way to compare your custom objects. E.g.: what about the order in which the properties of your objects are used for compare?
• Amol: i thought this can be used in UI by adding a sort button only for ascending or descending order
• Timothy Truckle: Your generic Comparator does not even know how many properties there are in your object. how can it respect any externally created order? The only useful generic approach I see is to by using a @SortKey(order=n) annotation on properties. IMHO anything else will be too complex to be usefull...
• Roland Illig: Please indent your code properly. It looks horrible right now.

Answer 1

I don't think it's a good idea to pass the sorting key as a String into your method, thus needing reflection.

But let's have a look at the implementation.

It's overly complicated and buggy.

However you implement your method, you can only sort by a field if that field has a type that allows comparing its values, e.g. an int, a String and so on. For e.g. a java.awt.Point field, I wouldn't know a universal way how to sort by that. And all the types that you might sort by typically implement Comparable [A special case are the primitive types like int or double, where their wrapper counterpart Integer or Double implements Comparable]. And Comparable gives you the comparison function for free that you wrote in a very complicated way.

Here's an improved version of your method:

public static <T> List<?> sortList(List<T> l, String field, Class<T> c) 
 throws ReflectiveOperationException, IntrospectionException { 
 final Field f = c.getDeclaredField(field);
 PropertyDescriptor propertyDescriptor = new PropertyDescriptor(f.getName(),c);
 final Method getter = propertyDescriptor.getReadMethod();
 Class<?> returnType = getter.getReturnType();
 if (Comparable.class.isAssignableFrom(returnType) || returnType.isPrimitive()) {
 // The result from invoke() will be Comparable
 Collections.sort(l, new Comparator<T>() {
 public int compare(T e1, T e2) {
 try {
 Comparable val1 = (Comparable) getter.invoke(e1);
 Comparable val2 = (Comparable) getter.invoke(e2);
 return val1.compareTo(val2);
 } catch (RuntimeException e) {
 throw e;
 } catch (Exception e) {
 throw new RuntimeException(e);
 }
 }
 });
 } else {
 throw new IllegalArgumentException("Cannot compare on field " + field + " of type " + returnType.getName());
 }
 return l;
} 

Finally some comments regarding your implementation:

i = Integer.parseInt(getter.invoke(e1).toString()) - 
 Integer.parseInt(getter.invoke(e2).toString());

This is effectively i = (Integer) getter.invoke(e1) - (Integer) getter.invoke(e2);. And the risk is overflow: if the value from e1 is 2000000000, and the value from e2 is -2000000000 (both fit within the integer number range), you'd expect e1 greater than e2, resulting in positive comparison value. But subtracting the two numbers gives a negative result (as 4000000000 is outside of the int range und thus overflows to a negative value). So you'd better use Integer.compare() instead of subtracting.

Even worse with the Long case: casting the subtraction result to int cuts off the higher binary digits, so giving you wrong results even in cases where the subtraction still was well within the long number range. Use Long.compare() instead.

You do things like Integer.parseInt(getter.invoke(e1).toString()), which is complete nonsense. getter.invoke(e1) already gives you an Integer, but you convert that to the decimal String representation of that number, just to parse it back to the original number. That's a huge waste of CPU resources. You just have to tell the compiler that the result is an Integer: (Integer) getter.invoke(e1).

Your choice of using PropertyDescriptor forces the user of your method to have both a getter and a setter method for the field that you use for sorting, although you really only need the getter.