General practitioner collection

Question

The following code is used to find the usual_gp(General Practitioner) from gpCollection variable of type TreeMap<Long(Date), SummableMap<String(GPId), Integer(GPCount)>> and store result (usual GP) along with date on usualGPCollection Map.

Does this code follow common best practices? Logic implemented on below code is

        Long startDate = gpCollection.firstKey();
        Long endDate = gpCollection.lastKey();

        TreeMap<Long, String> usualGPCollection = new TreeMap<Long, String>();

        for (Long i = startDate; i <= new DateTime(endDate).plusYears(1).getMillis(); i = i + new DateTime(i).plusMonths(1).getMillis()) {

            Long beforeOneYearDate = new DateTime(i).minusMonths(12).getMillis();
            Long beforeSixMonthDate = new DateTime(i).minusMonths(6).getMillis();
            Long beforeThreeMonthDate = new DateTime(i).minusMonths(3).getMillis();

            List<String> returnGPCollection = customSubMap(gpCollection, beforeOneYearDate, i);

            if(returnGPCollection.size() == 1) {

                usualGPCollection.put(i, returnGPCollection.get(0));

            } else if(returnGPCollection.size()>1){

                returnGPCollection = customSubMap(gpCollection, beforeSixMonthDate, i);

                if(returnGPCollection.size() == 1) {

                    usualGPCollection.put(i, returnGPCollection.get(0));

                }else  if(returnGPCollection.size()>1){

                    returnGPCollection = customSubMap(gpCollection, beforeThreeMonthDate, i);

                    if(returnGPCollection.size() == 1) {

                        usualGPCollection.put(i, returnGPCollection.get(0));

                    } else if(returnGPCollection.size()>1){

//                        returnGPCollection = customSubMap(gpCollection, i);//todo

                    }
                }

                returnGPCollection = customSubMap(gpCollection, beforeOneYearDate, i);
            }
    }

// customSubMap() Function is used to find max count GP on given date range.

private List<String> customSubMap(TreeMap<Long, SummableMap<String, Integer>>  gpCollectionMap, Long fromDate, Long toDate) {
        List<String> returnMap = null;

        SortedMap<Long, SummableMap<String, Integer>> temp = gpCollectionMap.subMap(fromDate, toDate);
        Collection values = temp.values();
        Iterator<SummableMap<String, Integer>> test = values.iterator();

        SummableMap<String, Integer> resultMap = new SummableMap<String, Integer>();
        TreeMap<Integer, List<String>> reverseTree = new TreeMap<Integer, List<String>>();

        while (test.hasNext()) {
            resultMap.putAll(test.next());
//            String  key = test.next().clone();
        }

        List mapValues = new ArrayList(resultMap.values());
        Collections.sort(mapValues);

        NavigableMap<String, Integer> resultMapUpdated = sortHashMapByValuesD(resultMap);

        String firstKey = resultMapUpdated.firstKey();
        String secondKey = resultMapUpdated.lowerKey(firstKey);

        Integer firstValue = resultMapUpdated.get(firstKey);
        Integer secondValue = resultMapUpdated.get(secondKey);

        if(firstValue == secondValue){
            returnMap.add(firstKey);
            returnMap.add(secondKey);
        }else {
            returnMap.add(firstKey);
        }

        return returnMap;
    }

    private List<String> customSubMap(TreeMap<Long, SummableMap<String, Integer>>  gpCollectionMap, Long toDate) {

        List<String> returnMap = null;
        Long beforeThreeMonthDate = new DateTime(toDate).minusMonths(3).getMillis();

        SortedMap<Long, SummableMap<String, Integer>> temp = gpCollectionMap.subMap(beforeThreeMonthDate, toDate);
        String latestGP = temp.get(temp.lastKey()).keySet().iterator().next();

        returnMap.add(latestGP);

        return returnMap;
    }

Answer 1

When you have cascading conditions like you have, it can become 'messy'. At some point the design-pattern 'Chain of responsibility' becomes useful....

Consider an interface:

public interface GPSelector {
    String selectGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate);
}

Then, consider an array of concrete implementations of that interface:

private static final GPSelector[] GPRULES = new GPSelector[] {

    // 1-year rule
    new GPSelector() {
        public String selectGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate) {
            List<String> lastyear = customSubMap(gpCollection, new DateTime(i).minusMonths(12).getMillis(), calcdate);
            if (lastyear.size() == 1) [
                return lastyear.get(0);
            }
            return null;
        }
    },

    // 6-month rule
    new GPSelector() {
        public String selectGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate) {
            // return not-null String if there is a successful 6-month candidate....
        }
    },

    .......

};

OK, so now you have an ordered array of rules that progressively select the right UsualGP candidate.

I your code, you can now simply have the following:

public String selectUsualGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate) {
    for (GPSelector rule : GPRULES) {
        String gp = rule.selectGP(gpCollection, calcdate);
        if (gp != null) {
            return gp;
        }
    }
    return null; // nothing matched.... :(
}

Adding a new rule is easy, just insert it in to the chain at the appropriate place....

I hope the above is enough to show how the chain-of-responsibility pattern can be applied.

Answer 2

I feel that customSubMap() is complicated and confusingly named.

I would devise a solution based around a Counter object, inspired by Python's collections.Counter class. A Counter would be a more generically useful object than a GPSelector, and furthermore, selecting visits in a date range is trivially taken care of already by NavigableMap.tailMap().

/**
 * Finds the "usual GP" according to the specification.
 *
 * @param gpCollection A record of the patient's GP visits.
 *                     Keys are visit times (in milliseconds since 1970).
 *                     Values are the names of the GPs.
 * @return the usual GP, or null if gpCollection is empty
 */
public static String usualGP(NavigableMap<Long, String> gpCollection) {
    if (gpCollection.isEmpty()) {
        return null;
    }

    // I assume that the date thresholds should be relative to the date
    // of the last visit, rather than today.
    DateTime lastVisitDate = new DateTime(gpCollection.lastKey());
    final long[] dateThresholds = new long[] {
        lastVisitDate.minusYears(1).getMillis(),
        lastVisitDate.minusMonths(6).getMillis(),
        lastVisitDate.minusMonths(3).getMillis()
    };

    for (long threshold : dateThresholds) {
        SortedMap<Long, String> recentVisits = gpCollection.tailMap(threshold);
        Counter<String> countRecent = new Counter<String>(recentVisits.values());
        Map.Entry<String, Integer>[] mostCommon = countRecent.mostCommon(2);

        // Is there a tie?
        if ( mostCommon.length > 1 &&
             mostCommon[0].getValue() > mostCommon[1].getValue() ) {
            return mostCommon[0].getKey();
        }
    }

    return gpCollection.lastEntry().getValue();
}

---

public class Counter<T> {
    public Counter(Iterable<T> values) {
        for (T value : values) {
            this.add(value);
        }
    }

    public void add(T value) {
        ...
    }

    /**
     * Returns a list of the n most common elements and their counts
     * from the most common to the least.  Elements with equal counts
     * are ordered arbitrarily.
     */
    public Map.Entry<T, Integer>[] mostCommon(int n) {
        ...
    }
}