General practitioner collection

The following code is used to find the usual_gp(General Practitioner) from gpCollection variable of type TreeMap> 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 usualGPCollection = new TreeMap();

for (Long i = startDate; i 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 customSubMap(TreeMap> gpCollectionMap, Long fromDate, Long toDate) {
List returnMap = null;

SortedMap> temp = gpCollectionMap.subMap(fromDate, toDate);
Collection values = temp.values();
Iterator> test = values.iterator();

SummableMap resultMap = new SummableMap();
TreeMap> reverseTree = new TreeMap>();

while (test.hasNext()) {
resultMa

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> 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> gpCollection, Long calcdate) {
            List 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> 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> 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.

StackExchange Code Review Q#44069, answer score: 5