Refactoring SerialDate

SerialDate is a date-handling class from David Gilbert's JCommon library, and the chapter walks through improving it top to bottom as a second, longer case study. Where the ComparisonCompactor chapter is about tightening code that was already clean, this one is messier and more representative of code you inherit: solid engineering overall, written by a competent author, but with real bugs, thin test coverage, and design decisions that made sense once and stopped making sense as the class grew. Martin is explicit that this is not an attack on Gilbert - it is the kind of professional review every programmer should welcome, and the class was generous enough to be open source in the first place.

First, make it work

Before any renaming or restructuring, the chapter establishes that you cannot safely refactor what you cannot verify. The existing test suite only exercised about half the class's executable statements, so the first step was writing an independent suite with much higher coverage - which immediately surfaced several real bugs: a boundary condition error in getFollowingDayOfWeek that returned the wrong date at a week boundary, a nearest-day-of-week algorithm that failed whenever the target day was in the future, and dead code that coverage tooling exposed because it could never execute. Only once "does it work" was answered honestly did the "make it right" pass begin.

Comment and Javadoc hygiene

The class opened with a block of change history and author metadata in a comment - the kind of thing a version control system already tracks far more reliably, and which rots the moment someone forgets to update it by hand. The lesson generalizes: comments that duplicate what tooling already knows, or that describe behavior which has since changed, are worse than no comment at all, because they actively mislead the next reader. The Javadoc itself also mixed four different "languages" (Java, English, Javadoc, HTML) in one comment block, which the chapter flags as its own kind of clutter.

Defensive programming and ambiguous returns

Several methods used sentinel return values and raw booleans in ways that hid intent. weekInMonthToString and relativeToString returned an error string on invalid input instead of throwing, which forces every caller to remember to check the return value against a magic string rather than let an exception make the failure impossible to ignore. The fix was straightforward: throw IllegalArgumentException and let the type system and the call stack do the defensive work instead of a convention only some callers followed.

That fix uncovered a bigger one. Once the enum refactor gave every enumerator a working toString(), the only remaining callers of weekInMonthToString turned out to be the tests Martin had just rewritten to exercise it - so he deleted the method and its tests outright, then found the same thing was true of relativeToString and deleted that too. "Throw a better exception" and "delete the function" are both legitimate fixes for the same smell; it's worth checking whether the smaller one still applies before settling for it.

Ambiguous names and unclear semantics

The chapter spends real effort on a subtle naming trap: methods like addDays were changed from static to instance methods for good reason, but date.addDays(7) reads exactly like a mutation even though it returns a new DayDate and leaves the original untouched. The fix was renaming the whole family to plusDays, plusMonths, plusYears - names that read correctly at the call site (newDate = oldDate.plusDays(5)) instead of implying the wrong thing. A name that could plausibly mean two different behaviors is a defect even when the code behind it is correct.

From raw ints to real constructs

# SerialDate, condensed: months are raw ints, and the

# abstract base class knows the concrete subclass that creates it.

MIN_YEAR = 1900

MAX_YEAR = 9999

 

def create_instance(ordinal):

# An "abstract" date class hardcoding its own concrete subclass.

return SpreadsheetDate(ordinal)

 

def add_days(date, days):

return create_instance(to_serial(date) + days)

 

def month_code_to_string(month, short):

# One method, one flag, deciding between two unrelated jobs.

if short:

return SHORT_MONTH_NAMES[month - 1]

return MONTH_NAMES[month - 1]

 

def add_days_to_date(date, days):

# Reads like it mutates "date" in place - it doesn't.

date.add_days(days)

# After: Month is a real enum, dates are created through a

# factory the base class doesn't need to know about, and every

# name says whether it mutates or returns something new.

class Month(Enum):

JANUARY = 1

# ...

DECEMBER = 12

 

def to_string(self):

return MONTH_NAMES[self.value - 1]

 

def to_short_string(self):

return SHORT_MONTH_NAMES[self.value - 1]

 

 

class DayDateFactory:

_instance = None

 

@classmethod

def make_date(cls, ordinal):

return cls._instance.make_date(ordinal)

 

 

class DayDate:

def plus_days(self, days):

# Name makes clear this returns a new DayDate.

return DayDateFactory.make_date(self.to_ordinal() + days)

Months, weekdays, and week-in-month values were all represented as raw int codes, validated by ad hoc range checks and converted through case statements. Turning each into a proper enum (Month, Day, WeekInMonth) eliminated the validation methods entirely - an enum cannot hold an invalid value - and let behavior that used to live in scattered conditionals (like computing a month's quarter, or formatting a weekday name) move onto the enum itself, where it belongs. This is the same "encapsulate what varies" idea from the design principles page, applied to a small, ordinal-shaped type instead of a whole subsystem. A related enum, DateInterval (OPEN, CLOSED, CLOSED_LEFT, CLOSED_RIGHT), replaced a switch statement in isInRange the same way - each enumerator implements its own range check, so a new interval kind never means hunting down every switch that handled the old ones.

extendsdependency

DayDate no longer hardcodes which concrete subclass to build - DayDateFactory does.

Dead and commented-out code

Several tables and helper methods turned out to be unused entirely - not called from anywhere in the class or its dependents - and were deleted outright rather than left "just in case." The chapter's position on commented-out code is blunt: nobody can tell how old it is or whether it still matters, so it just rots in place. Delete it; version control remembers it if anyone ever needs it back.

Leave it better, not perfect

The chapter closes on the same note as the JUnit case study: SerialDate wasn't rewritten into a different design, and Martin doesn't claim the result is flawless. Every class you touch doesn't need to become a showcase. The point of the exercise - and of the Boy Scout Rule more generally - is that the class left JCommon measurably cleaner, better tested, and with several real bugs fixed, which is a realistic and sufficient bar for a single pass through someone else's code.