I've been working on a plugin for JIRA to provide tracking and reporting of various metrics associated with Issues raised under an SLA (Service Level Agreement), during which I've tried to put in to practice many of the things I've learned (e.g from "Clean Code") and preached about myself (TDD). I've tried to be as well behaved as I can and written the tests first in true TDD style, and where I have not I have tried to ensure that appropriate tests are written after the implementation.
Some background on the domain; An SLA comprises two main elements: a coverage policy, and a set of durations defining the maximum permitted time for a response or resolution before that Issue is considered to be in breach of the SLA. The coverage policy describes the hours of the day during which measurements count against the threshold and is define by a mapping {Weekday -> {startTime, endTime}}, and hence it is impossible to represent more complex coverage patterns such as non-contiguous cover with a single day, e.g MONDAY 0900-1300 and 1400-1800.
The overall problem we are trying to solve is: given a particular Issue and SLA calculate whether responded to, and resolved within the threshold limits. However, I would like to discuss how I tried to implement a solution to a sub-problem of the overall one: given a start DateTime, an end DateTime, and a coverage policy determine the number of hours which count against the thresholds, i.e the total number of covered hours. This is best illustrated by a simple example:
Suppose that a particular SLA provides 9-5 coverage Monday to Friday with a response threshold of 30 minutes, and that an Issue is received at 8am on Monday and the response is made at 9.15am. The actual elapsed time between receiving and responding is 1 hour 15 minutes, but since an hour of that time lies outside a coverage period the actual response time is 15 minutes and hence the threshold is not breached.
So far so good I hope - lets try and implement a solution using TDD (note this is pretty much what I did but for the sake of brevity I won't reproduce the all tests nor all method bodies).
Start Simple And Build Up
So what is a simple case to start with?
@Test public void testGetCoveredHoursInInterval() { DateTime now = new DateTime(); Weekday nowWeekday = Weekday.valueOf(now.getDayOfWeek()); policy.addCoveragePeriod(nowWeekday, new LocalTime(8, 00), new LocalTime(17, 00)); DateTime start = new LocalTime(10, 0).toDateTime(now); DateTime end = new LocalTime(13, 0).toDateTime(now); Duration coveredDuration = policy.getCoveredDurationInInterval(start, end); assertEquals(new Duration(start, end), coveredDuration); }
Here we set up a coverage policy with a single coverage period for today between 8am and 5pm, since both the start and the end times are within this period the covered hours in the interval is the interval itself. In the cases where the start and end points of the interval are in the same day (the above of which is one such example) there are a total of (3! * 3!) / (2! * 2) = 18 combinations (since we ignore order, that is xy is the same as yx), of which 3 are invalid as the start falls after the end:
| Relative to Coverage Period | End Before | End During | End After |
|---|---|---|---|
| Start Before | X | X | X |
| Start During | Invalid | X | X |
| Start After | Invalid | Invalid | X |
Assume that we implement the method getCoveredDurationInInterval(DateTime start, DateTime end) incrementally as we create tests for the remaining 7 cases (where one case covers all those which violate causality); assuming that all the methods (e.g. isIntervalInSingleDay(DateTime start, DateTime end)) are implemented correctly the code looks something like:
public Duration getCoveredDurationInInterval(DateTime start, DateTime end) { if(start.isAfter(end)) throw new IllegalArgumentException("Start must be before the end, unless causality is broken.); if(isIntervalInSingleDay(start, end) { Interval coverageInterval = getCoverageIntervalForDay(start); if(start.isBefore(coverageInterval.getStart() && end.isBefore(coverageInterval.getStart()) { return Duration.ZERO; } if(start.isBefore(coverageInterval.getStart() && coverageInterval.contains(end) { return new Duration(coverageInterval.getStart(), end); } if(start.isBefore(coverageInterval.getStart() && end.isAfter(coverageInterval.getEnd()) { return coverageInterval; } if(coverageInterval.contains(start) && coverageInterval.contains(end)) { return new Duration(start, end); } if(coverageInterval.contains(start) && end.isAfter(coverageInterval.getEnd()) { return new Duration(start, coverageInterval.getEnd()); } if(start.isAfter(coverageInterval.getEnd() && end.isAfter(coverageInterval.getEnd()) { return Duration.ZERO; } } else { throw new IllegalArgumentException("Unable to cope with intervals which span multiple days"); } }
This is a fairly tidy method, but already the number of if statements should be starting to worry you; in this case although the logic is quite clear its still quite complex and therefore will make maintenance and furture extension more difficult. However I am reluctant to refactor at this point as, although I have tests, the implementation of the method is incomplete and by being distracted by cleaning up the code there is the possibility that logical inconsistencies are introduced manipulating the solution "in my head". Instead we continue completing all the possible cases of the input ranges; having implmented those cases where the start and end times are within the same day, we consider first the cases involving adjacent days, and finally where the interval spans multiple days. We arrive at the completed method:
public Duration getCoveredDurationInInterval(DateTime start, DateTime end) { if(start.isAfter(end)) throw new IllegalArgumentException("Start must be before the end, unless causality is broken.); if(isIntervalInSingleDay(start, end) { // <SNIP /> } if(isIntervalAcrossAdjacentDays(start, end) { Duration totalDuration = Duration.ZERO; Interval startDayCoverage = getCoverageIntervalForDay(start); if(start.isBefore(startDayCoverage.getStart()) { totalDuration = totalDuration.plus(startDayCoverage); } if(startDayCoverage.contains(start)) { Duration startDayDuration = new Duration(start, startDayCoverage.getEnd()); totalDuration = totalDuration.plus(startDayDuration); } if(start.isAfter(startDayCoverage.getEnd()) { //totalDuration = totalDuration.plus(Duration.ZERO); } Interval endDayCoverage = getCoverageIntervalForDay(end); if(end.isBefore(endDayCoverage.getStart()) { //totalDuration = totalDuration.plus(Duration.ZERO); } if(endDayCoverage.contains(end)) { Duration endDayDuration = new Duration(endDayCoverage.getStart(), end); totalDuration = totalDuration.plus(startDayDuration); } if(end.isAfter(endDayCoverage.getEnd()) { totalDuration = totalDuration.plus(startDayCoverage); } return totalDuration; } else { Duration totalDuration = Duration.ZERO; Interval startDayCoverage = getCoverageIntervalForDay(start); if(start.isBefore(startDayCoverage.getStart()) { totalDuration = totalDuration.plus(startDayCoverage); } if(startDayCoverage.contains(start)) { Duration startDayDuration = new Duration(start, startDayCoverage.getEnd()); totalDuration = totalDuration.plus(startDayDuration); } if(start.isAfter(startDayCoverage.getEnd()) { //totalDuration = totalDuration.plus(Duration.ZERO); } Duration middleDaysDuration = Duration.ZERO; int numberOfWholeDays = JodaTimeUtils.getNumberOfDaysInInterval(start, end) - 2; DateTime firstWholeDay = start.plusDays(1); for(int i = 0; i < numberOfWholeDays; i++) { Weekday weekday = Weekday.valueOf(firstWholeDay.plusDays(i).getDayOfWeek()); CoveragePeriod period = getCoveragePeriod(weekday); if(period != null) { Duration wholeDayCoverageDuration = period.getMaximumCoveredDuration(); middleDaysDuration = middleDaysDuration.plus(wholeDayCoverageDuration); } } totalDuration = totalDuration.plus(middleDaysDuration); Interval endDayCoverage = getCoverageIntervalForDay(end); if(end.isBefore(endDayCoverage.getStart()) { //totalDuration = totalDuration.plus(Duration.ZERO); } if(endDayCoverage.contains(end)) { Duration endDayDuration = new Duration(endDayCoverage.getStart(), end); totalDuration = totalDuration.plus(endDayDuration); } if(end.isAfter(endDayCoverage.getEnd()) { totalDuration = totalDuration.plus(endDayCoverage); } return totalDuration; } }
By now you should be squirming looking at this code; yes the code is fairly clean (largely thanks to the three big logic checks already being separated from the calculation code) and vaguely readable the number and depth of the logic branches makes it a somewhat complex algorithm, which in addition to its length means that it is harder to "see the method as a whole" and several readings are necessary to understand its behaviour. Furthermore, as noted earlier the if-complexity makes this algorithm much harder to debug and/or extend. But, thanks to following the TDD methodology we have a complete set of tests which cover all cases of the problem, and more importantly, treating the method getCoveredDurationInInterval as a black box which means that we can change the internal implementation however we wish and provided those test still pass we know we have not violated the "behaviour contract". In other words, time to refactor!
Refactor 0 - Burn the deadwood
For the purposes of clarity several cases that have no effect on the output, e.g. adding Duration.ZERO, were included in the original method. Theses serve no function and therefore are bad and should be removed before they poison the surrounding code.
Refactor 1 - DRY
The first thing that stands out to me is the massive duplication between the adjacent day and multiple day calculations, namely that adjacent is simply the case when JodaTimeUtils.getNumberOfDaysInInterval(start, end) - 2 == 0. Phew, that was easy, with a simple observation we've cut the length by 1/3 - some people will say that they would have modified the method earlier and gone straight to this step when coding to which I say, to each their own. In doing this we also remove the need for the method isIntervalAcrossAdjacentDays.
Refactor 2 - RTFM
A quick reading of the JodaTime API reveals the overlap method which calculates the overlap, or intersection, of two Intervals. This can be used to remove all the if cases from the single day scenario so that our method now looks like this:
public Duration getCoveredDurationInInterval(DateTime start, DateTime end) { if(start.isAfter(end)) throw new IllegalArgumentException("Start must be before the end, unless causality is broken.); if(isIntervalInSingleDay(start, end) { Interval totalInterval = new Interval(start, end); Interval coveredIntervalForDay = getCoverageIntervalForDay(start); return coveredIntervalForDay.overlap(totalInterval).toDuration(); } else { Duration totalDuration = Duration.ZERO; Interval startDayCoverage = getCoverageIntervalForDay(start); if(start.isBefore(startDayCoverage.getStart()) { totalDuration = totalDuration.plus(startDayCoverage); } if(startDayCoverage.contains(start)) { Duration startDayDuration = new Duration(start, startDayCoverage.getEnd()); totalDuration = totalDuration.plus(startDayDuration); } int numberOfWholeDays = JodaTimeUtils.getNumberOfDaysInInterval(start, end) - 2; DateTime firstWholeDay = start.plusDays(1); for(int i = 0; i < numberOfWholeDays; i++) { Weekday weekday = Weekday.valueOf(firstWholeDay.plusDays(i).getDayOfWeek()); CoveragePeriod period = getCoveragePeriod(weekday); if(period != null) { Duration wholeDayCoverageDuration = period.getMaximumCoveredDuration(); totalDuration = totalDuration.plus(wholeDayCoverageDuration); } } Interval endDayCoverage = getCoverageIntervalForDay(end); if(endDayCoverage.contains(end)) { Duration endDayDuration = new Duration(endDayCoverage.getStart(), end); totalDuration = totalDuration.plus(endDayDuration); } if(end.isAfter(endDayCoverage.getEnd()) { totalDuration = totalDuration.plus(endDayCoverage); } return totalDuration; } }
Refactor 3 - Algorithmic Encapsulation
While the previous small changes have done vast amounts to improve the clean-ness off the code, the overall length of the method still bothers me; when calculating the multiple day case there are three distinct steps in the algorithm which can be encapsulated in their own methods, these steps are calculate duration for the: first day, last day, intermediary days. The canny eye also spots that the first and last day calculations can also be achieved using the JodaTime Interval.overlap() method (similar to the same day calculation) which has the benefit of removing further if statements.
The complete code is now:
public Duration getCoveredDurationInInterval(DateTime start, DateTime end) { if(start.isAfter(end)) throw new IllegalArgumentException("Start must be before the end, unless causality is broken.); if(isIntervalInSingleDay(start, end) { Interval totalInterval = new Interval(start, end); Interval coveredIntervalForDay = getCoverageIntervalForDay(start); return coveredIntervalForDay.overlap(totalInterval).toDuration(); } else { Duration totalDuration = Duration.ZERO; totalDuration = totalDuration.plus(getCoveredDurationInDayFromInstant(start)); totalDuration = totalDuration.plus(getCoveredDurationForDaysBetween(start.plusDays(1), end.minusDays(1))); totalDuration = totalDuration.plus(getCoveredDurationInDayToInstant(end)); return totalDuration; } } protected Duration getCoveredDurationInDayFromInstant(DateTime instant) { DateTime nextMidnight = instant.plusDays(1).toMidnightDateTime(); Interval totalInterval = new Interval(instant, nextMidnight); Interval coverage = getCoverageIntervalForDay(instant); return totalInterval.overlap(coverage).toDuration(); } protected Duration getCoveredDurationForDaysBetween(DateTime start, DateTime end) { Duration totalDuration = Duration.ZERO; int numberOfWholeDays = JodaTimeUtils.getNumberOfDaysInInterval(start, end); for(int i = 0; i < numberOfWholeDays; i++) { Weekday weekday = Weekday.valueOf(start.plusDays(i).getDayOfWeek()); CoveragePeriod period = getCoveragePeriod(weekday); if(period != null) { Duration wholeDayCoverageDuration = period.getMaximumCoveredDuration(); totalDuration = totalDuration.plus(wholeDayCoverageDuration); } } return totalDuration; } protected Duration getCoveredDurationInDayToInstant(DateTime instant) { DateTime thisMidnight = instant.toMidnightDateTime(); Interval totalInterval = new Interval(thisMidnight, instant); Interval coverage = getCoverageIntervalForDay(instant); return totalInterval.overlap(coverage).toDuration(); }
NOTE BENE all these changes were done without changing the tests developed at the start, when this is the case it demonstrates your tests are testing behaviour not implementation. And that you've not changed that behaviour during the refactoring too of course 
And there we have it, a case study in refactoring. Arguably the method getCoveredDurationForDaysBetween could be improved upon further but in the authour's opinion unless it proves to be a problem it is now best left alone and attention moved elsewhere.
