Grouping objects by two fields in java

I have bunch of log files and I want to process them in java, but I want to sort them first so I can have more human readable results.

My Log Class :

public class Log{
//only relevant fields here
private String countryCode;
private AccessType accessType;
...etc..
}

AccessType is Enum, which has values WEB, API, OTHER.

I'd like to group Log objects by both countryCode and accessType, so that end product would be log list.

I got this working for grouping Logs into log list by countryCode like this :

public List<Log> groupByCountryCode(String countryCode开发者_StackOverflow社区) {
        Map<String, List<Log>> map = new HashMap<String, List<Log>>();
        for (Log log : logList) {
            String key = log.getCountryCode();
            if (map.get(key) == null) {
                map.put(key, new ArrayList<Log>());
            }
            map.get(key).add(log);
        }
        List<Log> sortedByCountryCodeLogList = map.get(countryCode);

        return sortedByCountryCodeLogList;
    }

from this @Kaleb Brasee example :

Group by field name in Java

Here is what I've been trying for some time now, and really stuck now ..

public List<Log> groupByCountryCode(String countryCode) {
        Map<String, Map<AccessType, List<Log>>> map = new HashMap<String, Map<AccessType, List<Log>>>();
        AccessType mapKey = null;
        List<Log> innerList = null;
        Map<AccessType, List<Log>> innerMap = null;
        // inner sort
        for (Log log : logList) {
            String key = log.getCountryCode();
            if (map.get(key) == null) {
                map.put(key, new HashMap<AccessType, List<Log>>());
                innerMap = new HashMap<AccessType, List<Log>>();
            }

            AccessType innerMapKey = log.getAccessType();
            mapKey = innerMapKey;
            if (innerMap.get(innerMapKey) == null) {
                innerMap.put(innerMapKey, new ArrayList<Log>());
                innerList = new ArrayList<Log>();
            }

            innerList.add(log);
            innerMap.put(innerMapKey, innerList);
            map.put(key, innerMap);
            map.get(key).get(log.getAccessType()).add(log);
        }

        List<Log> sortedByCountryCodeLogList = map.get(countryCode).get(mapKey);

        return sortedByCountryCodeLogList;
    }

I'm not sure I know what I'm doing anymore

Your question is confusing. You want to sort the list, but you are creating many new lists, then discarding all but one of them?

Here is a method to sort the list. Note that Collections.sort() uses a stable sort. (This means that the original order of items within a group of country code and access type is preserved.)

class MyComparator implements Comparator<Log> {
  public int compare(Log a, Log b) {
    if (a.getCountryCode().equals(b.getCountryCode()) {
      /* Country code is the same; compare by access type. */
      return a.getAccessType().ordinal() - b.getAccessType().ordinal();
    } else
      return a.getCountryCode().compareTo(b.getCountryCode());
  }
}
Collections.sort(logList, new MyComparator());

If you really want to do what your code is currently doing, at least skip the creation of unnecessary lists:

public List<Log> getCountryAndAccess(String cc, AccessType access) {
  List<Log> sublist = new ArrayList<Log>();
  for (Log log : logList) 
    if (cc.equals(log.getCountryCode()) && (log.getAccessType() == access))
      sublist.add(log);
  return sublist;
}

If you're able to use it, Google's Guava library has an Ordering class that might be able to help simplify things. Something like this might work:

   Ordering<Log> byCountryCode = new Ordering<Log>() {
     @Override
     public int compare(Log left, Log right) {
        return left.getCountryCode().compareTo(right.getCountryCode());
     }
   };

   Ordering<Log> byAccessType = new Ordering<Log>() {
     @Override
     public int compare(Log left, Log right) {
        return left.getAccessType().compareTo(right.getAccessType());
     }
   };
   Collections.sort(logList, byCountryCode.compound(byAccessType));

You should create the new inner map first, then add it to the outer map:

        if (map.get(key) == null) {
            innerMap = new HashMap<AccessType, List<Log>>();
            map.put(key, innerMap);
        }

and similarly for the list element. This avoids creating unnecessary map elements which will then be overwritten later.

Overall, the simplest is to use the same logic as in your first method, i.e. if the element is not present in the map, insert it, then just get it from the map:

    for (Log log : logList) {
        String key = log.getCountryCode();
        if (map.get(key) == null) {
            map.put(key, new HashMap<AccessType, List<Log>>());
        }

        innerMap = map.get(key);
        AccessType innerMapKey = log.getAccessType();
        if (innerMap.get(innerMapKey) == null) {
            innerMap.put(innerMapKey, new ArrayList<Log>());
        }
        innerMap.get(innerMapKey).add(log);
    }

Firstly, it looks like you're adding each log entry twice with the final line map.get(key).get(log.getAccessType()).add(log); inside your for loop. I think you can do without that, given the code above it.

After fixing that, to return your List<Log> you can do:

List<Log> sortedByCountryCodeLogList = new ArrayList<Log>();
for (List<Log> nextLogs : map.get(countryCode).values()) {
    sortedByCountryCodeLogList.addAll(nextLogs);
}

I think that code above should flatten it down into one list, still grouped by country code and access type (not in insertion order though, since you used HashMap and not LinkedHashMap), which I think is what you want.