Efficient way of sorting a List based on multiple properties using Google Guava

Question

I'm using Google Guava 17.0 library. I have a class named AroundBust containing only two properties id of type Integer and size of type BigDecimal.

This class is instantiated based on the data stored in an XML file. A java.util.List is populated by a list of objects of this class.

This list is displayed in a PrimeFaces data table.

This list is sorted based on sort orders supplied by the data table (multiple sorting).

I'm trying sort the list as follows (in the associated JSF managed bean).

@ManagedBean
@ViewScoped
public final class AroundBustManagedBean extends LazyDataModel<AroundBust> implements Serializable {

    private List<AroundBust> entireList;
    private static final long serialVersionUID = 1L;

    private static final class IntegerSorting {

        private IntegerSorting() {}

        public static Function<AroundBust, Integer> getInstance() {
            return Holder.INSTANCE;
        }

        private enum Holder implements Function<AroundBust, Integer> {

            INSTANCE {
                @Override
                public Integer apply(AroundBust f) {
                    return f.getId();
                }
            }
        }
    }

    private static final class BigDecimalSorting {

        private BigDecimalSorting() {}

        public static Function<AroundBust, BigDecimal> getInstance() {
            return Holder.INSTANCE;
        }

        private enum Holder implements Function<AroundBust, BigDecimal> {

            INSTANCE {
                @Override
                public BigDecimal apply(AroundBust f) {
                    return f.getSize();
                }
            }
        }
    }

    @Override
    public List<AroundBust> load(int first, int pageSize, List<SortMeta> multiSortMeta, final Map<String, Object> filters) {
        //The instance variable entireList is populated by unmarshalling the XML file in question.
        int listSize = 0;

        Map<String, String> map = null;
        //Assuming the List parameter multiSortMeta is somehow turned into a map.
        //I'm not presenting it here to reduce the code complexity.

        //The sorting is performed by the code below using the Guava library.
        if (map != null && !map.isEmpty()) {
            Ordering<AroundBust> id = null;
            Ordering<AroundBust> value = null;
            Ordering<AroundBust> idValue = null;
            Ordering<AroundBust> valueId = null;

            for (Map.Entry<String, String> entry : map.entrySet()) {
                if (entry.getKey().equalsIgnoreCase("id")) {
                    id = entry.getValue().equalsIgnoreCase("asc") ? Ordering.natural().onResultOf(IntegerSorting.getInstance()) : Ordering.natural().onResultOf(IntegerSorting.getInstance()).reverse();

                    if (value != null) {
                        valueId = value.compound(id);
                    }
                } else if (entry.getKey().equalsIgnoreCase("size")) {
                    value = entry.getValue().equalsIgnoreCase("asc") ? Ordering.natural().onResultOf(BigDecimalSorting.getInstance()) : Ordering.natural().onResultOf(BigDecimalSorting.getInstance()).reverse();

                    if (id != null) {
                        idValue = id.compound(value);
                    }
                }
            }

            if (idValue != null) {
                entireList = idValue.sortedCopy(entireList);
            } else if (valueId != null) {
                entireList = valueId.sortedCopy(entireList);
            } else if (id != null) {
                entireList = id.sortedCopy(entireList);
            } else if (value != null) {
                entireList = value.sortedCopy(entireList);
            }
        } else {
            //This is just a default sort order, when the data table is loaded for the first time.
            entireList = Ordering.natural().onResultOf(IntegerSorting.getInstance()).reverse().sortedCopy(entireList);
        }

        return new ArrayList<AroundBust>(entireList.subList(first, Math.min(pageSize + first, listSize)));
    }
}

The map maintains a list of fields to be sorted with the field name as a key and the sort order ("asc" for ascending order and "desc" for descending order) as its value in the exact order in which the data table column headers are clicked.

There are only two fields but it requires many conditional checks as can be seen in the code snippet.

These conditional checks are increased significantly as the number of properties/fields are increased in the class. As a result, the code may be unmanageable later on.

How to reduce this code at a minimum extent?

Answer 1

Code smell: you are doing the same thing over and over again, with different instances of the same variable type.

            if(idValue!=null) {
                entireList=idValue.sortedCopy(entireList);
            } else if(valueId!=null) {
                entireList=valueId.sortedCopy(entireList);
            }else if(id!=null) {
                entireList=id.sortedCopy(entireList);
            }else if(value!=null) {
                entireList=value.sortedCopy(entireList);
            }

Instead, assign the correct value for that type to a new variable, and then use it once.

            Ordering<AroundBust> selectedOrdering = createOrdering(map);
            entireList = selectedOrdering.sortedCopy(entireList);

Code smell: don't repeat yourself

value = entry.getValue().equalsIgnoreCase("asc")?Ordering.natural().onResultOf(BigDecimalSorting.getInstance()):Ordering.natural().onResultOf(BigDecimalSorting.getInstance()).reverse();

Here, the key tells you that you want a BigDecimal sort, and the value tells you whether or not to reverse it.

Ordering<AroundBust> selectedOrdering = Ordering.natural().onResultOf(BigDecimalSorting.getInstance());
if (entry.getValue().equalsIgnoreCase("asc")) {
    selectedOrdering = selectedOrdering.reverse();
}

You have the same issue here:

    else {
        //This is just a default sort order, when the data table is loaded for the first time.
        entireList=Ordering.natural().onResultOf(IntegerSorting.getInstance()).reverse().sortedCopy(entireList);
    }

Once again, you aren't really doing anything different - you create an Ordering, and then use it to sortedCopy the list. So your code really wants to look like

Ordering<AroundBust> selectedOrdering = Ordering.natural().onResultOf(IntegerSorting.getInstance()).reverse();

if(map!=null&&!map.isEmpty())
{
    selectedOrdering = this.createOrdering(map);
}

entireList = selectedOrdering.sortedCopy(entireList);

The holder enums in your sorting functions aren't really giving you anything here. Unless you are borrowing that code from elsewhere, it should be possible to use the enumeration directly.

    private enum BigDecimalSorting implements Function<AroundBust, BigDecimal> {
        INSTANCE {
            public BigDecimal apply(AroundBust f) {
                return f.getSize();
            }
        }
    }

But I think even this much is overkill -- the enum singleton trick is important if the singleton is Serializable, which isn't the case here. A static member variable should be fine

private static final Function<AroundBust,BigDecimal> GET_SIZE = new Function<AroundBust, BigDecimal>() {
    public BigDecimal apply(codeReview.sortingAList.AroundBust f) {
        return f.getSize();
    }
};

The logic for selecting the entries to be returned should be cleaned up a bit

return new ArrayList<AroundBust>(entireList.subList(first, Math.min(pageSize + first, listSize)));

First, you should call out what the min calculation is for, and make it explicit. You can then pull out the list you want, with a useful name to clarify what is going on

int last = Math.min(pageSize + first, listSize);
List<AroundBust> page = Lists.newArrayList(entireList.subList(first,last));
return page;

Of course, since we aren't doing anything interesting with page here, that should probably be inlined.

Answer 2

I could come up with the following code.

@Override
public List<AroundBust> load(int first, int pageSize, List<SortMeta> multiSortMeta, final Map<String, Object> filters)
{
    Map<String, String> map=new LinkedHashMap<String, String>();
    //Turn the List multiSortMeta into a map 
    //containing sort field as a key and sort order as its value in same order in which the table headers are clicked.
    //The map is actually maintained in another JSF managed bean.

    if(map!=null&&!map.isEmpty()) {
        List<Ordering<AroundBust>>list=new ArrayList<Ordering<AroundBust>>();

        for(Map.Entry<String, String>entry:map.entrySet()) {
            if(entry.getKey().equalsIgnoreCase("id")) {
                Ordering<AroundBust> ordering = Ordering.natural().onResultOf(IntegerSorting.ID_SORT);
                list.add(entry.getValue().equalsIgnoreCase("asc")?ordering:ordering.reverse());
            } else if(entry.getKey().equalsIgnoreCase("size")) {
                Ordering<AroundBust> ordering = Ordering.natural().onResultOf(BigDecimalSorting.VALUE_SORT);
                list.add(entry.getValue().equalsIgnoreCase("asc")?ordering:ordering.reverse());
            }
            //More fields as and when required.
        }

        Ordering<AroundBust> compoundOrdering=list.remove(0);

        for(Ordering<AroundBust>ordering:list) {
            compoundOrdering=compoundOrdering.compound(ordering);
        }

        entireList=compoundOrdering.sortedCopy(entireList);
    } else {
        entireList=Ordering.natural().onResultOf(IntegerSorting.ID_SORT).reverse().sortedCopy(entireList);
    }
    return Lists.newArrayList(entireList.subList(first, Math.min(pageSize + first, listSize)));
}

IntegerSorting.ID_SORT and BigDecimalSorting.VALUE_SORT are static fields as the accepted answer suggests.

The last foreach loop in the code snippet above,

for(Ordering<AroundBust>ordering:list) {
    compoundOrdering=compoundOrdering.compound(ordering);
}

is effectively something similar to the following while loop.

int i = 0;

while(i < n) {
    i = i + 1; //No short hand operator like i++ here.
}
//Do something with i.

The foreach loop can combine any number of ordering statements in any given order (the sort order is determined based on the data table headers which are clicked and is maintained abe LinkedHashMap<String, String> as described above).