This chapter is a complete, worked case study of refactoring org.jfree.date.SerialDate (by David Gilbert), found in the org.jfree.date package of the JCommon library. It demonstrates the two-phase discipline -- "First, Make It Work" (fix bugs found through testing) then "Then Make It Right" (systematic cleanup) -- and catalogs every specific code smell Martin found and fixed. The chapter is as much about the attitude of professional code review as it is about the mechanics.
Martin notes that SerialDate was written to address a real pain -- java.util.Date and java.util.Calendar are about times, not dates. He welcomes "a class that is about dates instead of times" and acknowledges the author's opening Javadoc (line 67) explains the motivation well.
When to Use
Apply this checker whenever you are:
Reviewing or refactoring someone else's code
Reviewing or refactoring your own legacy code
Preparing to modify a class you did not write
Performing a professional code critique or review
Cleaning up a codebase after achieving passing tests
Deciding where methods, constants, and enums belong
Looking for dead code, redundant comments, or naming inaccuracies
Converting raw int/String constants to type-safe enums
Changing static methods to instance methods or vice versa
Deciding whether to keep or remove final keyword clutter
Extracting explaining temporary variables to clarify algorithms
Applying the Boy Scout Rule ("leave the code cleaner than you found it")
Core Philosophy: Respectful Professional Critique
Before any technical checklist, internalize the attitude Martin models:
Critiquing code is not an act of malice or arrogance. It is a professional review, like a doctor reviewing a colleague's case or a pilot debriefing a flight. We should all be comfortable doing it and having it done to our code.
Acknowledge the author's competence. Martin explicitly states David Gilbert is an experienced and competent programmer, and the code is "good code" -- before proceeding to "rip it to pieces." Start with genuine respect.
Acknowledge the author's courage. Open-source authors put their code out for public scrutiny. That takes courage and good will. Recognize it.
Your own code has the same problems. Martin says: "if you were to find some of my code, I'm sure you could find plenty of things to complain about." Nobody is above critique.
Critique is how we learn. Doctors, pilots, and lawyers do peer review. Programmers must learn to do it too.
Checklist
Phase 1: First, Make It Work (Test-First Refactoring)
C16-WORK-1: Write your own independent test suite before refactoring. Do not rely on the existing tests -- they may have gaps. Martin found existing tests (SerialDateTests, Listing B-2) covered only ~50% of executable statements (91 of 185). A "Find Usages" search showed MonthCodeToQuarter (line 334) was never called [F4], and the tests didn't test it [T1]. The coverage map looked like "a patchwork quilt, with big gobs of unexecuted code littered all through the class." He wrote his own completely independent suite (Listing B-4) achieving 92% coverage (170 of 185) even with some tests commented out.
C16-WORK-2: Include tests for behavior you THINK the code should have. Comment them out if they fail. As you refactor, work to make them pass. These represent your design intent. Martin admits the first few commented-out tests (lines 23-63) "were a bit of conceit on my part" -- the program was not designed to pass them, but the behavior seemed obvious [G2]. He also left tests at lines 32 and 45 commented out because it was unclear whether "tues" and "thurs" abbreviations ought to be supported.
C16-WORK-3: Use coverage tools (e.g., Clover, JaCoCo) to find unexecuted code. Coverage maps reveal "big gobs of unexecuted code littered all through the class." Understand what is tested and what is not before changing anything.
C16-WORK-4: Fix boundary condition bugs. Martin found getFollowingDayOfWeek had > where it should have been >= (line 685). Boundary errors are a classic bug category [G3],[T1],[T5]. The method returned December 25th as the Saturday following December 25th, 2004 -- an off-by-one boundary error. Interestingly, the change history (line 43) showed this function was the target of an earlier repair that claimed to fix "bugs" in getPreviousDayOfWeek, getFollowingDayOfWeek, and getNearestDayOfWeek [T6] -- yet the bugs persisted.
C16-WORK-5: Fix algorithmically wrong code. The getNearestDayOfWeek algorithm was fundamentally broken. The testGetNearestDayOfWeek unit test (line 329) was not initially exhaustive -- Martin added many test cases and the pattern of failure [T7] revealed the algorithm fails when the nearest day is in the future. The coverage pattern from Clover [T8] confirmed line 719 was never executed -- the if statement at line 718 was always false because the adjust variable was always negative and so could never be greater or equal to 4. Martin rewrote the algorithm entirely using: int delta = targetDOW - base.getDayOfWeek(); int positiveDelta = delta + 7; int adjust = positiveDelta % 7; if (adjust > 3) adjust -= 7;
C16-WORK-6: Make string comparisons case-insensitive where appropriate. The testWeekdayCodeToString tests were trivially fixed by changing lines 259 and 263 to use equalsIgnoreCase. The tests on lines 153 and 154 for stringToMonthCode didn't pass either [G2] -- fixed by making the stringToMonthCode function (lines 163-213) compare using equalsIgnoreCase for both short and long month names.
C16-WORK-7: Throw exceptions instead of returning error codes. Tests at line 417 and 429 were made to pass by throwing IllegalArgumentException instead of returning an error string from weekInMonthToString and relativeToString.
C16-WORK-8: Run ALL tests (including the broader framework's tests) after every change. Martin ran all JCommon unit tests plus his improved SerialDate tests after every single change to ensure nothing broke.
Phase 2: Then Make It Right (Systematic Cleanup)
Walk through the file top-to-bottom, improving as you go. Run tests after every change.
Comments and Formatting
C16-RIGHT-1: Delete change history comments [C1], but preserve required legal notices. Source control tools handle change history. A change history log in the source file is a leftover from the 1960s. However, Martin explicitly acknowledges that copyrights and licenses must stay -- "there are certain legalities that need to be addressed, and so the copyrights and licenses must stay."
C16-RIGHT-2: Shorten import lists using wildcards [J1].java.text.* and java.util.* instead of individual imports.
C16-RIGHT-3: Avoid HTML in Javadoc comments [G1]. Having four languages in a source file (Java, English, Javadoc, HTML) makes things hard to keep straight. Use <pre> blocks to preserve formatting rather than embedding <ul> and <li> tags.
C16-RIGHT-4: Delete redundant comments [C2]. "Redundant comments are just places to collect lies and misinformation." If the code says what the comment says, delete the comment. The comment on line 93 was redundant. Comments at line 97 and 100 about "serial numbers" were stale.
C16-RIGHT-5: Delete comments whose information is captured by well-named code [C3]. Tables starting at line 140 had comments that simply restated what the variable names already said. Their names were sufficient.
C16-RIGHT-6: Delete comments on closing braces, repetitious comments, and other noise. The comment on lines 895-899 was unnecessary repetition. Delete it along with all similar noise.
Naming
C16-RIGHT-7: Rename classes to reflect abstraction, not implementation [N1, N2].SerialDate was named after its implementation (serial number = days since Dec 30, 1899). The term "serial number" is not even correct -- "ordinal" is more accurate, as "serial number" has more to do with product identification markers. More importantly, the name implies implementation in what should be an abstract class. Martin's preferred name was simply Date, but too many Java classes already use that name. He considered Day, but it is also heavily used elsewhere. He settled on DayDate as the best compromise -- a name at the right level of abstraction.
C16-RIGHT-8: Rename variables to be self-descriptive [N1].SERIAL_LOWER_BOUND / SERIAL_UPPER_BOUND became EARLIEST_DATE_ORDINAL / LATEST_DATE_ORDINAL with comments like // 1/1/1900 and // 12/31/9999. toSerial became toOrdinal, then getOrdinalDay. getYYYY became getYear.
C16-RIGHT-9: Use mathematically precise naming [N3].INCLUDE_NONE, INCLUDE_FIRST, INCLUDE_SECOND, INCLUDE_BOTH are obscure. These describe interval boundary inclusion. Renamed to enum DateInterval with values OPEN, CLOSED_LEFT, CLOSED_RIGHT, CLOSED -- standard mathematical terminology.
C16-RIGHT-10: Rename methods to avoid ambiguity [N4].addDays/addMonths/addYears are misleading because they do NOT modify the object -- they return a new DayDate. Renamed to plusDays/plusMonths/plusYears to clearly convey immutability. DayDate date = oldDate.plusDays(5); reads correctly. date.plusDays(5); (ignoring return value) correctly reads as a mistake.
C16-RIGHT-11: Rename compare to daysSince [N1]. The method (lines 902-913) returns the difference in days, not a comparison result. The name should describe what it actually returns. Martin noted there were no existing tests for this method, so he wrote them as part of pulling the implementation up into DayDate.
C16-RIGHT-12: Rename createInstance to makeDate [N1]. Improves clarity significantly.
C16-RIGHT-13: Rename Month.make to Month.fromInt [N1]. And similarly for all other enums. Clearer factory method naming.
C16-RIGHT-14: Rename weekdayCodeToString to toString [N1]. When moved into the Day enum, the longer name becomes redundant. The enum context provides the meaning.
C16-RIGHT-15: Make function names more self-descriptive [N1].getMonths (which returned month name strings) was renamed to getMonthNames. monthCodeToString became toString and toShortString when moved to the Month enum. stringToMonthCode became Month.parse (using a private matches(String s) helper that does equalsIgnoreCase comparison against both toString() and toShortString()). getEndOfCurrentMonth was renamed to getEndOfMonth and clarified as a true instance method.
Enums Instead of Constants
C16-RIGHT-16: Replace constant-inheritance pattern with enums [J2, J3]. Inheriting from a constants class (like MonthConstants) to get JANUARY, FEBRUARY, etc. is an old Java trick but a bad idea. Replace with a proper Month enum with values JANUARY(1) through DECEMBER(12), a make(int) factory, and encapsulated index.
C16-RIGHT-17: Convert day-of-week constants to a Day enum [J3]. Day constants (line 109) become Day enum: MONDAY(Calendar.MONDAY) through SUNDAY(Calendar.SUNDAY). Include make(int), parse(String), and toString() methods. Move to its own source file [G13].
C16-RIGHT-19: Convert relative-day-of-week constants to WeekdayRange enum [J3]. With values LAST, NEXT, NEAREST. Martin notes a key advantage of enums over int constants: because they are now passed as symbols rather than integers, you can use your IDE's "change name" function to rename them easily without worrying about missing some -1 or 2 somewhere in the code [J3].
C16-RIGHT-20: When enums make validation methods obsolete, delete them [G9].isValidMonthCode (line 326) was made irrelevant by the Month enum and was deleted. isValidWeekdayCode was similarly made obsolete by the Day enum.
Encapsulation and Method Placement
C16-RIGHT-21: Move implementation-specific variables to the implementing class [G6].EARLIEST_DATE_ORDINAL and LATEST_DATE_ORDINAL do not belong in abstract DayDate -- they are specific to SpreadsheetDate. Move them down.
C16-RIGHT-22: Move MINIMUM_YEAR_SUPPORTED / MAXIMUM_YEAR_SUPPORTED appropriately [G6]. These are implementation details. However, they are used by RelativeDayOfWeekRule (another class) for validation. Solution: use the ABSTRACT FACTORY pattern -- DayDateFactory provides getMinimumYear() and getMaximumYear() abstractly, with SpreadsheetDateFactory supplying the concrete values.
C16-RIGHT-23: Base classes must not know about their derivatives [G7].DayDate contained createInstance (line 808) which directly created SpreadsheetDate instances. This is a dependency inversion violation. Fix with Abstract Factory: DayDateFactory with a default SpreadsheetDateFactory, using SINGLETON + DECORATOR + ABSTRACT FACTORY patterns.
C16-RIGHT-24: Move methods to where their data lives -- cure Feature Envy [G14].monthCodeToQuarter (lines 356-375) envied the Month enum. Replaced it with Month.quarter(): public int quarter() { return 1 + (index-1)/3; }. stringToWeekdayCode became Day.parse(String). stringToMonthCode became Month.parse(String). weekdayCodeToString became Day.toString(). monthCodeToString became Month.toString() and Month.toShortString(). getEndOfCurrentMonth (lines 728-740) envied its DayDate argument -- made it a true instance method.
C16-RIGHT-25: Move lastDayOfMonth to the Month enum [G17]. The function uses LAST_DAY_OF_MONTH array. The array belongs in Month, and the function should be simplified to use month.lastDay().
C16-RIGHT-25a: Push leapYearCount down to SpreadsheetDate [G6]. The leapYearCount function (lines 519-536) does not really belong in DayDate -- nobody calls it except two methods in SpreadsheetDate. Pushed it down to where it is used.
C16-RIGHT-26: Change static methods to instance methods when they operate on instance data [G18].addDays (lines 562-576) was static but operated on DayDate variables. Changed to instance method plusDays. Same for addMonths and addYears (became plusMonths and plusYears). Same for getPreviousDayOfWeek, getFollowingDayOfWeek, getNearestDayOfWeek. Same for getEndOfCurrentMonth.
C16-RIGHT-27: Pull abstract methods up from derivatives when they don't depend on implementation [G6].toDate (converting DayDate to java.util.Date) was abstract but its SpreadsheetDate implementation didn't depend on anything implementation-specific. Pushed the implementation up into DayDate. Similarly, compare (renamed daysSince) was pulled up. Six functions (lines 915-980) that were all abstract methods implemented in SpreadsheetDate were pulled up.
C16-RIGHT-28: Handle logical dependencies physically [G22].getDayOfWeek appears to have no physical dependency on SpreadsheetDate, but it has a logical dependency (the algorithm depends on the origin of ordinal day 0). Created an abstract method getDayOfWeekForOrdinalZero implemented in SpreadsheetDate to return Day.SATURDAY. This makes the logical dependency explicit and physical.
C16-RIGHT-29: Move the isInRange switch statement into the DateInterval enum [G23]. The ugly switch on interval types becomes polymorphic behavior: each DateInterval enum value (OPEN, CLOSED_LEFT, CLOSED_RIGHT, CLOSED) implements its own isIn(int d, int left, int right) method. The isInRange method itself was refactored to use Math.min(d1.getOrdinalDay(), d2.getOrdinalDay()) and Math.max(...) to normalize argument order, then delegate to interval.isIn(getOrdinalDay(), left, right).
C16-RIGHT-30: Move enums to their own source files when they grow large enough [G11, G13]. The Day enum was moved out of DayDate into its own file because it does not depend on DayDate. Same for Month when it grew large enough (with quarter(), toString(), toShortString(), parse(), lastDay()). Same for DateInterval, WeekInMonth, WeekdayRange.
C16-RIGHT-31: Extract utility methods to a utility class [G6]. Static variable dateFormatSymbols and static methods getMonthNames, isLeapYear, lastDayOfMonth were moved into a new DateUtil class.
C16-RIGHT-32: Move abstract methods to the top of the class [G24]. Abstract methods should be listed first so readers immediately see the class's contract.
Dead Code and Clutter Removal
C16-RIGHT-33: Delete the serialVersionUID variable [G4]. If you don't declare it, the compiler generates one. Manual control is risky -- you might forget to update it. An InvalidClassException from auto-generated UID is easier to debug than silent undefined behavior from a stale manual UID.
C16-RIGHT-34: Delete unused tables and constants [G9].AGGREGATE_DAYS_TO_END_OF_MONTH was not used anywhere in JCommon. Deleted. Same for LEAP_YEAR_AGGREGATE_DAYS_TO_END_OF_MONTH.
C16-RIGHT-35: Make tables private and expose through functions [G8, G10, G11].LAST_DAY_OF_MONTH table had no good reason to be public. Make it private, expose via lastDayOfMonth function. AGGREGATE_DAYS_TO_END_OF_PRECEDING_MONTH was used only in SpreadsheetDate -- moved close to where it is used, made private, exposed through julianDateOfLastDayOfMonth.
C16-RIGHT-36: Delete the degenerate default constructor [G12]. The compiler will generate it. Explicit empty constructors are clutter.
C16-RIGHT-37: Delete unused methods and their tests.weekInMonthToString (lines 742-761) -- after refactoring to use WeekInMonth enum toString(), the method was deleted. Then the tests that were its only callers were also deleted. relativeToString (lines 765-781) -- nobody called it except tests, so both function and tests were deleted.
C16-RIGHT-38: Collapse wrapper methods [G9, G12, F4]. Two getMonths functions (lines 288-316) where the first just called the second. Collapsed to one and renamed to getMonthNames.
C16-RIGHT-39: Delete Javadocs that add no value [C3, G12]. Javadocs on stringToWeekdayCode (lines 242-270) don't add much to the method signature. The only value was describing the -1 return value, but since the method now throws IllegalArgumentException, even that is gone. Delete the Javadoc.
C16-RIGHT-40: Delete the description field and its accessor/mutator [G9]. Line 208's description field was not used by anyone. Deleted along with getter and setter.
C16-RIGHT-41: Remove unnecessary final keywords [G12]. All final keywords in arguments and variable declarations were removed. They add clutter without real value. The kinds of errors final might catch are already caught by unit tests. (Note: final on occasional constants is still fine.)
Simplifying Logic
C16-RIGHT-42: Merge duplicate if statements [G5]. Two separate if statements inside a for loop (lines 259, 263) checking short name and long name were merged into a single if using ||.
C16-RIGHT-43: Use Explaining Temporary Variables to clarify complex algorithms [G19].addMonths used opaque expressions. Refactored with variables like thisMonthAsOrdinal, resultMonthAsOrdinal, resultYear, resultMonth, lastDayOfResultMonth, resultDay. Similarly for getPreviousDayOfWeek, getFollowingDayOfWeek, getNearestDayOfWeek -- variables like offsetToTarget, offsetToThisWeeksTarget, offsetToFutureTarget, offsetToPreviousTarget.
C16-RIGHT-44: Simplify isLeapYear with Explaining Temporary Variables [G16].boolean fourth = year % 4 == 0; boolean hundredth = year % 100 == 0; boolean fourHundredth = year % 400 == 0; return fourth && (!hundredth || fourHundredth);
C16-RIGHT-45: Simplify lastDayOfMonth [G16]. Reduced to: if (month == Month.FEBRUARY && isLeapYear(year)) return month.lastDay() + 1; else return month.lastDay();
C16-RIGHT-46: Simplify addDays / plusDays [N1]. Reduced to a single line: return DayDateFactory.makeDate(toOrdinal() + days);
C16-RIGHT-47: Extract common code into shared helper methods [G5]. Duplication between plusYears and plusMonths was eliminated by extracting correctLastDayOfMonth, making all three methods (plusDays, plusMonths, plusYears) much clearer.
C16-RIGHT-48: Replace magic numbers with named constants [G25]. Magic number 1 was replaced with Month.JANUARY.toInt() or Day.SUNDAY.toInt() as appropriate.
C16-RIGHT-49: Make enum index fields private. Created toInt() accessor for all enums instead of exposing index directly.
C16-RIGHT-50: Avoid passing boolean flags as function arguments [G15]. The pattern of monthCodeToString(month, boolean shortened) where one method calls its twin with a flag is a bad idea. Replaced with two distinct methods: toString() and toShortString() on the Month enum.
Violations to Detect
Each violation references the Clean Code smell code Martin cites in the chapter.
ID
Smell Code
Violation
Description
V1
N1, N2
Implementation-leaking name
Class/method/variable named after implementation details rather than abstraction (e.g., SerialDate instead of DayDate, toSerial instead of getOrdinalDay)
V2
N4
Misleading mutability name
Method name implies mutation but returns new object (e.g., addDays on immutable type -- should be plusDays)
V3
N1
Vague or inaccurate name
Name does not describe what the thing actually is/does (e.g., compare for a method returning day count difference -- should be daysSince; getYYYY should be getYear)
V4
N3
Non-standard terminology
Using invented terms when standard ones exist (e.g., INCLUDE_FIRST instead of CLOSED_LEFT from interval math)
V5
J2
Constant inheritance
Inheriting from a constants interface/class just to access constants without qualification. Use enums instead.
V6
G6
Misplaced variable/method
Variable or method in the wrong class -- typically implementation details in an abstract class, or behavior that belongs to the data it operates on
V7
G7
Base class knows its derivatives
Abstract/base class directly references or creates instances of a concrete subclass
V8
G14
Feature Envy
Method that manipulates another class's data more than its own. Move it to the class it envies.
V9
G18
Inappropriate static method
Static method that operates on instance data and should be an instance method
V10
G9
Dead code
Methods, variables, tables, imports that are never called/used. Delete them.
V11
G12
Clutter
Default constructors, unused variables, final on everything, unnecessary Javadocs, degenerate methods
V12
C1
Change history comment
Manually maintained change log in source file. Use version control.
V13
C2
Redundant/stale comment
Comment that restates what code says, or is outdated/wrong
V14
C3
Comment restating the obvious
Comments on tables/variables whose names are already descriptive enough
V15
G1
Multi-language source file
Mixing Java + HTML + Javadoc + English in comments makes maintenance hard
V16
G4
Manual serialVersionUID
Manually maintained serial version UIDs are risky -- let the compiler generate them
V17
G5
Code duplication
Duplicate if checks, duplicate methods, copy-paste logic across plusMonths/plusYears
V18
G8
Unnecessarily public data
Tables/arrays that should be private, exposed only through functions
V19
G10
Data far from usage
Variables/tables defined far from where they are used. Move them close.
V20
G11
Inconsistency
Doing the same conceptual thing differently in different places (e.g., some enums in separate files, others not)
V21
G13
Enum/inner class that should be in its own file
Inner types that don't depend on the enclosing class should be extracted
V22
G15
Boolean flag argument
Passing a boolean to select between two behaviors. Use two separate methods instead.
V23
G16
Obscured algorithm
Algorithm that could be clarified with explaining temporary variables
V24
G17
Misplaced responsibility
Data (like LAST_DAY_OF_MONTH) residing in a class other than the one it logically belongs to
V25
G19
Missing explaining variables
Complex expressions without named intermediates that explain what each part means
V26
G20
Misleading method semantics
date.addDays(5) looks like it mutates date but actually returns a new object
V27
G21
Unprotected algorithm complexity
Complex algorithm that is not simplified and made transparent
V28
G22
Logical dependency not physical
Code logically depends on another module's implementation but has no physical (code-level) dependency expressing that
V29
G23
Switch/case replaceable with polymorphism
Switch statements on type codes that should be replaced with polymorphic behavior (e.g., isInRange switch on DateInterval values)
V30
G24
Poorly ordered declarations
Abstract methods buried in the middle of a class instead of at the top
V31
G25
Magic numbers
Raw numeric literals where named constants or enum values should be used
V32
T1-T8
Insufficient testing
Low coverage, missing boundary tests, untested methods, tests that reveal dead code
V33
T5
Boundary condition error
Off-by-one errors, wrong comparison operators (> vs >=), especially in date/range logic
V34
F4
Wrapper method with no additional logic
Method that only delegates to another method with no transformation -- collapse them
V35
J1
Overly specific imports
Importing individual classes when wildcard import is cleaner
How to Fix
Refactoring Workflow (The Two-Phase Approach)
PHASE 1: MAKE IT WORK
1. Read and understand the existing code thoroughly
2. Run existing tests -- note what passes and what coverage looks like
3. Write your OWN comprehensive, independent test suite
- Include tests for behavior you believe SHOULD exist (comment out failing ones)
- Aim for high coverage (Martin achieved 92% up from ~50%)
4. Fix bugs revealed by your tests:
- Boundary condition errors (>= vs >, off-by-one)
- Algorithmically broken code (always-false conditions, wrong formulas)
- Case sensitivity issues
- Error codes where exceptions should be thrown
5. Run ALL tests after each fix
6. All tests pass --> move to Phase 2
PHASE 2: MAKE IT RIGHT
Walk top-to-bottom through the file. After EVERY change, run ALL tests.
Step A -- Comments and formatting:
- Delete change history
- Shorten imports
- Remove HTML from Javadoc
- Delete redundant/stale/obvious comments
Step B -- Naming:
- Rename class to reflect abstraction, not implementation
- Rename variables to be self-descriptive
- Use standard terminology (math, domain)
- Rename methods to avoid mutation ambiguity
- Rename methods to describe return values accurately
Step C -- Type safety:
- Convert constant sets to enums
- Delete validation methods made obsolete by enums
- Add factory methods (make/fromInt) and parse methods to enums
- Make enum index fields private, add toInt() accessors
Step D -- Encapsulation and placement:
- Move implementation-specific data to implementing class
- Use Abstract Factory to break base-class-knows-derivative dependencies
- Move methods to the class whose data they envy (cure Feature Envy)
- Change static methods to instance methods when they use instance data
- Pull up abstract methods that don't depend on implementation
- Make logical dependencies physical
- Move enums to their own files when independent
- Extract utility methods to utility classes
- Move abstract method declarations to top of class
Step E -- Dead code and clutter:
- Delete unused methods, tables, constants, variables
- Delete degenerate default constructors
- Remove unnecessary final keywords
- Collapse trivial wrapper methods
- Delete Javadocs that add no value
- Delete unused fields and their accessors/mutators
Step F -- Simplify logic:
- Merge duplicate conditionals
- Use explaining temporary variables
- Extract common code into helper methods
- Replace magic numbers with named constants/enums
- Replace switch statements with polymorphism
- Split boolean-flag methods into distinct methods
Step G -- Final pass:
- Review the whole class for flow and readability
- Shorten and improve the opening comment
- Verify all enums in their own files
- Verify all tests still pass
- Check coverage -- it may have changed (Martin's went to 84.9%
because the class shrank so much that remaining uncovered lines
have greater weight, but 45 of 53 executable statements were covered)
Specific Fix Patterns
Fix: Implementation-leaking name
// BEFORE: Name reveals implementation (serial number offset)
public abstract class SerialDate { ... }
int serial = toSerial();
// AFTER: Name reflects abstraction
public abstract class DayDate { ... }
int ordinal = getOrdinalDay();
Fix: Constant inheritance --> Enum
// BEFORE: Inheriting constants interface
public class SerialDate implements MonthConstants {
// uses JANUARY, FEBRUARY directly...
}
// AFTER: Proper enum
public static enum Month {
JANUARY(1), FEBRUARY(2), /* ... */ DECEMBER(12);
public final int index;
Month(int index) { this.index = index; }
public static Month make(int monthIndex) { /* ... */ }
}
Fix: Base class creates derivative
// BEFORE: Abstract class directly creates concrete subclass
public abstract class DayDate {
public static DayDate createInstance(int ordinal) {
return new SpreadsheetDate(ordinal); // BAD: knows derivative
}
}
// AFTER: Abstract Factory
public abstract class DayDateFactory {
private static DayDateFactory factory = new SpreadsheetDateFactory();
public static DayDate makeDate(int ordinal) {
return factory._makeDate(ordinal);
}
protected abstract DayDate _makeDate(int ordinal);
}
Fix: Feature Envy -- move method to data owner
// BEFORE: Method in DayDate envies Month data
public static int monthCodeToQuarter(int month) {
switch (month) { /* 12 cases mapping month to quarter */ }
}
// AFTER: Method on Month enum
public int quarter() {
return 1 + (index - 1) / 3;
}
Fix: Static method that should be instance method
// BEFORE: Static method using instance data
public static SerialDate addDays(int days, SerialDate base) {
return createInstance(base.toSerial() + days);
}
// AFTER: Instance method
public DayDate plusDays(int days) {
return DayDateFactory.makeDate(toOrdinal() + days);
}
Fix: Explaining Temporary Variables
// BEFORE: Opaque algorithm
int adjust = targetDOW - base.getDayOfWeek();
// ... hard to follow ...
// AFTER: Named intermediates explain each step
int offsetToTarget = targetDayOfWeek.index - getDayOfWeek().index;
if (offsetToTarget >= 0)
offsetToTarget -= 7;
return plusDays(offsetToTarget);
Fix: Misleading method name on immutable type
// BEFORE: Looks like it mutates the date object
date.addDays(7); // Reader thinks date changed
// AFTER: Clearly returns new object
DayDate date = oldDate.plusDays(7); // Obviously a new date
Fix: Switch to polymorphism in enum (with normalized arguments)
// BEFORE: Switch statement in isInRange
switch (interval) {
case INCLUDE_NONE: return d > left && d < right;
case INCLUDE_FIRST: return d >= left && d < right;
// ...
}
// AFTER: Polymorphic enum + normalized isInRange
public enum DateInterval {
OPEN {
public boolean isIn(int d, int left, int right) {
return d > left && d < right;
}
},
CLOSED_LEFT {
public boolean isIn(int d, int left, int right) {
return d >= left && d < right;
}
},
// ...
public abstract boolean isIn(int d, int left, int right);
}
// isInRange normalizes with Math.min/Math.max:
public boolean isInRange(DayDate d1, DayDate d2, DateInterval interval) {
int left = Math.min(d1.getOrdinalDay(), d2.getOrdinalDay());
int right = Math.max(d1.getOrdinalDay(), d2.getOrdinalDay());
return interval.isIn(getOrdinalDay(), left, right);
}
Fix: Enum parse method with private matches helper
// Month.parse uses a private matches() helper for clean string comparison
public static Month parse(String s) {
s = s.trim();
for (Month m : Month.values())
if (m.matches(s))
return m;
try {
return make(Integer.parseInt(s));
}
catch (NumberFormatException e) {}
throw new IllegalArgumentException("Invalid month " + s);
}
private boolean matches(String s) {
return s.equalsIgnoreCase(toString()) ||
s.equalsIgnoreCase(toShortString());
}
Fix: Boolean flag argument
// BEFORE: Flag selects output format
public static String monthCodeToString(int month, boolean shortened) {
if (shortened) return shortMonthNames[month - 1];
else return monthNames[month - 1];
}
// AFTER: Two explicit methods on the enum
public String toString() {
return dateFormatSymbols.getMonths()[index - 1];
}
public String toShortString() {
return dateFormatSymbols.getShortMonths()[index - 1];
}
Self-Improvement Protocol
After completing a review using this checker:
Measure coverage before AND after. Martin started at ~50% coverage, wrote tests to get 92%, and after refactoring the class shrank so much that coverage showed 84.9% -- but that was 45 of 53 statements, with uncovered lines being trivial.
Count the changes. Martin's refactoring of SerialDate resulted in the class shrinking from 185 executable statements to 53. The amount of code removed is itself a metric of improvement.
Verify the Boy Scout Rule. The code should be cleaner when you leave than when you arrived. Test coverage increased, bugs were fixed, code was clarified and shrunk.
Check for cascading improvements. Converting constants to enums cascaded into deleting validation methods, simplifying parsing, enabling Feature Envy fixes, and moving code to proper locations. One good change often enables several others.
Beware of being too clever. Martin deleted weekInMonthToString through an "elegant" chain of refactorings, only to realize the only callers were his own tests. "Fool me once, shame on you. Fool me twice, shame on me!" -- he then checked relativeToString and found the same situation. Always verify a method has real callers before investing in refactoring it.
Every change, run tests. This cannot be overstated. Martin ran ALL JCommon unit tests plus his improved SerialDate tests after every single change. No exceptions. This is the safety net that makes aggressive refactoring possible.
Track code smells by their codes. This chapter references these specific smell categories from elsewhere in the book:
G1 (Multiple Languages in Source File), G4 (Overridden Safeties), G5 (Duplication), G6 (Code at Wrong Level of Abstraction), G7 (Base Classes Depending on Derivatives), G8 (Too Much Information / public internals), G9 (Dead Code), G10 (Vertical Separation), G11 (Inconsistency), G12 (Clutter), G13 (Artificial Coupling), G14 (Feature Envy), G15 (Selector Arguments), G16 (Obscured Intent), G17 (Misplaced Responsibility), G18 (Inappropriate Static), G19 (Use Explaining Temporary Variables), G20 (Function Names Should Say What They Do), G21 (Understand the Algorithm), G22 (Make Logical Dependencies Physical), G23 (Prefer Polymorphism to If/Else or Switch/Case), G24 (Follow Standard Conventions), G25 (Replace Magic Numbers with Named Constants)
J1 (Avoid Long Import Lists), J2 (Don't Inherit Constants), J3 (Constants vs Enums)
N1 (Choose Descriptive Names), N2 (Choose Names at Appropriate Level of Abstraction), N3 (Use Standard Nomenclature Where Possible), N4 (Unambiguous Names)
T1 (Insufficient Tests), T2 (Use a Coverage Tool), T3 (Don't Skip Trivial Tests), T5 (Test Boundary Conditions), T6 (Exhaustively Test Near Bugs), T7 (Patterns of Failure Are Revealing), T8 (Test Coverage Patterns Can Be Revealing)
Apply this recursively. The Boy Scout Rule is not a one-time activity. Every time you touch code, leave it a bit cleaner. Over time, codebases improve organically.
Reference listings. The original SerialDate is in Listing B-1 (page 349). The original tests are in SerialDateTests Listing B-2 (page 366). MonthConstants is Listing B-3 (page 372). Martin's independent test suite is Listing B-4 (page 374). SpreadsheetDate is Listing B-5 (page 382). RelativeDayOfWeekRule is Listing B-6 (page 390). The final refactored result spans Listings B-7 through B-16 (pages 394-405).
Bibliography cited in this chapter: [GOF] Design Patterns, Gamma et al., 1996. [Simmons04] Hardcore Java, Robert Simmons Jr., O'Reilly, 2004 (cited for "spread final all over your code" advice that Martin disagrees with). [Refactoring] Refactoring: Improving the Design of Existing Code, Fowler et al., 1999. [Beck97] Smalltalk Best Practice Patterns, Kent Beck, 1997 (cited for Explaining Temporary Variables).
