5
\$\begingroup\$

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;
    }
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Woow Great Answer... Thanks @rolfl. I really appreciate your effort. \$\endgroup\$ – nKandel Mar 11 '14 at 16:12
1
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.