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

enter image description here

 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; }

rolfl · Accepted Answer · 2014-03-11 14:49:10Z

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.

Woow Great Answer... Thanks @rolfl. I really appreciate your effort. — nKandel
– nKandel, Commented Mar 11, 2014 at 16:12

200_success · Accepted Answer · 2014-03-11 18:48:00Z

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) { ... } }

I've posted a follow-up question of my own for the Counter. — 200_success
– 200_success, Commented Mar 19, 2014 at 19:54

Stack Exchange Network

General practitioner collection

2 Answers 2

You must log in to answer this question.

Linked

Hot Network Questions

General practitioner collection

2 Answers 2

You must log in to answer this question.

Linked

Related

Hot Network Questions