Code quality checker based on Clean Code Ch14: Successive Refinement — checks incremental refactoring discipline, TDD during refactoring, recognizing code smells early, refactoring moves, and the principle of never letting messes build up
Installation
Install with Codex or Claude Copy this prompt, paste it into Codex, Claude, or another assistant, and let it review the skill page and install it for you.
Code quality checker based on Clean Code Ch14: Successive Refinement — checks incremental refactoring discipline, TDD during refactoring, recognizing code smells early, refactoring moves, and the principle of never letting messes build up
"To write clean code, you must first write dirty code and then clean it."
Programming is a craft, not a science. Clean code is produced through successive refinement -- like writing rough drafts of a composition, then revising until it is polished. The cardinal sin is leaving code in its rough-draft form once it "works." Most seasoned programmers know that moving on from merely-working code is professional suicide.
"It is not enough for code to work. Code that works is often badly broken. Programmers who satisfy themselves with merely working code are behaving unprofessionally."
This chapter is a case study in successive refinement -- you see a module that started well but did not scale, and then you see how the module was refactored and cleaned. The case study is the Args class: a command-line argument parser. The chapter presents the finished clean code first, then the messy rough draft, then the entire step-by-step refactoring journey between them.
The Writing Analogy
"We learned this truth in grade school when our teachers tried (usually in vain) to get us to write rough drafts of our compositions. The process, they told us, was that we should write a rough draft, then a second draft, then several subsequent drafts until we had our final version. Writing clean compositions, they tried to tell us, is a matter of successive refinement."
When to Use
Apply this skill when:
You are adding a new feature to existing code and the code is getting harder to change
You notice that adding a new type, variant, or case requires touching many places
A module that started clean is growing messy as requirements expand
You have working code that you are tempted to leave "as is" and move on
You are doing any refactoring -- small or large -- on a working system
You are reviewing code that was written quickly under deadline pressure
You have a class with too many instance variables of different conceptual types
You see parallel data structures (multiple Maps/Lists that track the same logical entity)
You see type-checking logic (instanceof, type codes, switch on type) that should be polymorphism
You are about to add a feature and sense that the current structure will not accommodate it cleanly
You want to extract a class, interface, or module from a monolithic file
You need to move error handling or exception logic into its own module
Checklist
The Successive Refinement Discipline
Get it working first: Write the rough draft. Get the tests passing. Do not try to write clean code in one pass -- that is unrealistic. "I am not expecting you to be able to write clean and elegant programs in one pass."
Then clean it: Once working, immediately refactor. Never move to the next task leaving rough-draft code behind.
Maintain a comprehensive test suite: You need automated tests (unit and acceptance) that you can run on a whim to verify behavior is unchanged after each refactoring step. "I use the discipline of Test-Driven Development (TDD). One of the central doctrines of this approach is to keep the system running at all times."
Make one tiny change at a time: Each change should be small enough that the system keeps working (or breaks in an obvious, quickly-fixable way). Run tests after every change. "Using TDD, I am not allowed to make a change to the system that breaks that system. Every change I make must keep the system working as it worked before."
Never make massive structural changes all at once: One of the best ways to ruin a program is to make massive changes to its structure in the name of improvement. Some programs never recover from such "improvements." "The problem is that it's very hard to get the program working the same way it worked before the 'improvement.'"
Refactoring is like solving a Rubik's cube: There are lots of little steps required to achieve a large goal. Each step enables the next. You will put things in temporarily so you can take them out later.
If a test breaks, fix it immediately: Incrementalism demands that you get things working quickly before making any other changes. Never proceed with a broken test.
Recognizing When to Stop and Clean
Watch for the tipping point: Code that handles one case cleanly may become a mess when a second or third case is added. The Boolean-only Args was clean; adding String and Integer turned it into a festering pile.
Count your instance variables: A sheer number of instance variables is a smell. The rough draft of Args had 11 instance variables -- that is daunting.
Watch for parallel data structures: Multiple HashMaps (booleanArgs, stringArgs, intArgs) tracking the same conceptual entity (argument marshalers) is a sign you need a unified abstraction.
Watch for duplicated patterns across types: If each new argument type requires code in three places (schema parsing, argument setting, value getting), that is a pattern begging for polymorphism.
Stop adding features when you sense the structure cannot absorb them: Martin had two more argument types to add but stopped because he could tell they would make things much worse. The structure needed fixing first.
Recognize the "three major places" pattern: Each argument type required new code in three major places: (1) parsing the schema element to select the right HashMap, (2) parsing the argument string and converting to the true type, (3) a getXxx method to return the value as its true type. "Many different types, all with similar methods -- that sounds like a class to me." This is how the ArgumentMarshaler concept was born.
Recognize odd strings and magic values: Strings like "TILT" as error parameters, raw HashSets and TreeSets used as state flags -- these are signs of ad-hoc code that needs proper abstractions.
The TDD Safety Net During Refactoring
Have tests before you refactor: For the Args class, Martin had both JUnit unit tests and FitNesse acceptance tests. The acceptance tests caught a bug the unit tests missed (ClassCastException on getBoolean with nonboolean argument). "To achieve this, I need a suite of automated tests that I can run on a whim and that verifies that the behavior of the system is unchanged."
Run tests after every single change: Not after a batch of changes -- after each individual tiny step. "I could run these tests any time I wanted, and if they passed, I was confident that the system was working as I specified."
When tests break, stop and fix before proceeding: Do not accumulate broken tests. Incrementalism demands immediate repair. "Incrementalism demanded that I get this working quickly before making any other changes."
Add new tests when you discover gaps: When the FitNesse tests caught something unit tests missed, Martin added a new unit test that invoked all the FitNesse tests to prevent future surprises. (Footnote 2: "To prevent further surprises of this kind, I added a new unit test that invoked all the FitNesse tests.")
Write the test first when adding new features to clean code: When adding the double argument type to the refactored code, Martin wrote testSimpleDoublePresent first, then added the implementation. This is TDD. He also wrote testInvalidDouble and testMissingDouble test cases to verify error processing.
Test both the happy path and error paths: The chapter shows tests for valid input (testSimpleBooleanPresent, testSimpleStringPresent, testSimpleIntPresent, testSimpleDoublePresent) AND error paths (testInvalidInteger, testMissingInteger, testInvalidDouble, testMissingDouble, testMissingStringArgument, testNonLetterSchema, testInvalidArgumentFormat).
Complete Listing Reference (14-1 through 14-16)
The chapter contains 16 listings that tell the full story of the Args case study:
Listing
Title
What It Shows
14-1
Simple use of Args
The ideal client code: new Args("l,p#,d*", args) with getBoolean('l'), getInt('p'), getString('d') -- shows how simple the API is to use
14-2
Args.java (final clean version)
The polished Args class with just 4 instance variables: marshalers, argsFound, currentArgument (ListIterator), argsList. Reads top to bottom. Schema format: no tail = Boolean, * = String, # = Integer, ## = Double, [*] = String array
14-3
ArgumentMarshaler.java
The interface: just void set(Iterator<String> currentArgument) throws ArgsException and (implicitly) each derivative has a static getValue method
14-4
BooleanArgumentMarshaler.java
Implements ArgumentMarshaler: set sets booleanValue = true, static getValue returns false if null/wrong-type
14-5
StringArgumentMarshaler.java
Implements ArgumentMarshaler: set calls currentArgument.next(), catches NoSuchElementException for MISSING_STRING
14-6
IntegerArgumentMarshaler.java
Implements ArgumentMarshaler: set calls currentArgument.next() then Integer.parseInt, catches both NoSuchElementException (MISSING_INTEGER) and NumberFormatException (INVALID_INTEGER)
14-7
ArgsException.java (final clean version)
Full exception class with ErrorCode enum (OK, INVALID_FORMAT, UNEXPECTED_ARGUMENT, INVALID_ARGUMENT_NAME, MISSING_STRING, MISSING_INTEGER, INVALID_INTEGER, MISSING_DOUBLE, INVALID_DOUBLE), multiple constructors, getters/setters, and errorMessage() method
14-8
Args.java (first draft / rough draft)
The messy version with 11 instance variables, separate booleanArgs/stringArgs/intArgs HashMaps, boolean valid, errorParameter = "TILT", inner ArgsException extends Exception {} stub, falseIfNull/zeroIfNull/blankIfNull helpers
14-9
Args.java (Boolean only)
The clean early version with only Boolean support -- compact, simple, about 6 instance variables, numberOfArguments counter, simple parseSchemaElement
14-10
Args.java (Boolean and String)
After adding String support -- now has stringArgs map, ErrorCode enum with OK and MISSING_STRING, setStringArg with ArrayIndexOutOfBoundsException handling. "It's starting to get out of hand" but "not festering quite yet"
14-11
ArgumentMarshaller appended to Args.java
The first skeleton: ArgumentMarshaler class with booleanValue field and getter/setter, plus empty BooleanArgumentMarshaler, StringArgumentMarshaler, IntegerArgumentMarshaler derivatives. "Clearly, this wasn't going to break anything."
14-12
Args.java (After first refactoring)
Intermediate state after unifying maps but before eliminating the type-case. Still has boolean valid, String[] args, int currentArgument, errorParameter = "TILT", the instanceof chain in setArgument, and ClassCastException catches in getters. "After all that work, this is a bit disappointing."
14-13
ArgsTest.java
Complete JUnit test suite with: testCreateWithNoSchemaOrArguments, testWithNoSchemaButWithOneArgument, testWithNoSchemaButWithMultipleArguments, testNonLetterSchema, testInvalidArgumentFormat, testSimpleBooleanPresent, testSimpleStringPresent, testMissingStringArgument, testSpacesInFormat, testSimpleIntPresent, testInvalidInteger, testMissingInteger, testSimpleDoublePresent, testInvalidDouble, testMissingDouble
Complete ArgsException with ErrorCode enum including INVALID_ARGUMENT_NAME, all constructors (including the 3-arg constructor with errorCode, char errorArgumentId, String errorParameter), and full errorMessage() switch statement
14-16
Args.java (final)
The final clean Args: 5 instance variables (schema, marshalers, argsFound, currentArgument, argsList), parse() is void and throws ArgsException, parseSchemaElement uses elementTail.length() == 0 / .equals("*") / .equals("#") / .equals("##") dispatch, setArgument is a single polymorphic m.set(currentArgument) call
The Args Case Study Progression (The Story Arc)
The chapter tells the story in a specific pedagogical order:
The final clean code is shown FIRST (Listings 14-1 through 14-7) -- so you can appreciate what "clean" looks like before seeing the mess
"How Did I Do This?" -- Martin confesses he did NOT write it clean from scratch. "I did not simply write this program from beginning to end in its current form."
The rough draft is shown (Listing 14-8) -- the messy working version after Boolean + String + Integer were added
The Boolean-only version (Listing 14-9) -- showing the code was once clean before it degraded
The Boolean+String version (Listing 14-10) -- showing the mess starting to grow
"So I Stopped" -- the critical decision point
"On Incrementalism" -- the philosophy of tiny steps
The complete refactoring journey (Listings 14-11 through 14-12 and inline code) -- every step shown
Adding double via TDD -- proving the refactored code is easy to extend
Separating ArgsException -- the final cleanup
The completely separated final code (Listings 14-13 through 14-16)
Conclusion -- the "never let the rot get started" message
The "So I Stopped" Decision Point
This is one of the most important moments in the chapter. After adding Boolean, String, and Integer argument types:
"I had at least two more argument types to add, and I could tell that they would make things much worse. If I bulldozed my way forward, I could probably get them to work, but I'd leave behind a mess that was too large to fix. If the structure of this code was ever going to be maintainable, now was the time to fix it."
Key observations at the stopping point:
Each argument type required code in three major places: schema parsing, argument setting, value getting
"Many different types, all with similar methods -- that sounds like a class to me." This is how ArgumentMarshaler was born
The mess had built gradually -- earlier versions (Boolean-only) had not been nearly so nasty
"The addition of just two more argument types had a massively negative impact on the code. It converted it from something that would have been reasonably maintainable into something that I would expect to become riddled with bugs and warts."
Schema Format Symbols
Symbol
Meaning
Example
(none)
Boolean argument
l in schema "l,p#,d*"
*
String argument
d* in schema "l,p#,d*"
#
Integer argument
p# in schema "l,p#,d*"
##
Double argument
x## in schema "x##"
[*]
String array argument
Not shown in detail but mentioned in final code
What Makes the Final Code "Clean" (Listing 14-2/14-16)
Martin asks the reader to "read it very carefully" and "pay special attention to the way things are named, the size of the functions, and the formatting." Key qualities:
Reads top to bottom without jumping around or looking ahead
Adding a new argument type requires only: a new ArgumentMarshaler derivative, a new getXxx function, a new case in parseSchemaElement, and probably a new ArgsException.ErrorCode and error message -- "a trivial amount of effort"
Each class has a single responsibility: Args parses arguments, ArgsException handles errors, each Marshaler handles its type
No parallel data structures, no type-checking conditionals, no null-checking helpers
Note: Martin acknowledges Java verbosity: "One of the reasons for this is that we are using a particularly wordy language. Java, being a statically typed language, requires a lot of words in order to satisfy the type system. In a language like Ruby, Python, or Smalltalk, this program is much smaller." (Footnote 1: "I recently rewrote this module in Ruby. It was 1/7th the size and had a subtly better structure.")
Violations to Detect
V1: Leaving Rough-Draft Code in Place
Smell: Code that "works" but has obvious structural problems -- too many instance variables, duplicated patterns, mixed concerns.
The rough draft (Listing 14-8): 11 instance variables, inline error codes, type-checking scattered throughout, inner exception class as empty stub. "Actually 'rough draft' is probably the kindest thing you can say about this code. It's clearly a work in progress. The sheer number of instance variables is daunting. The odd strings like 'TILT,' the HashSets and TreeSets, and the try-catch-catch blocks all add up to a festering pile."
The clean version (Listings 14-2/14-16): 4-5 instance variables (schema, marshalers, argsFound, currentArgument, argsList), polymorphic ArgumentMarshaler hierarchy, separated ArgsException module.
Rule: "Most freshman programmers don't follow this advice particularly well. They believe that the primary goal is to get the program working. Once it's 'working,' they move on to the next task, leaving the 'working' program in whatever state they finally got it to 'work.' Most seasoned programmers know that this is professional suicide."
The Gradual Degradation: The mess built gradually -- not all at once. The Boolean-only version (Listing 14-9) "it's really not that bad. It's compact and simple and easy to understand." But within it "it is easy to see the seeds of the later festering pile." Adding just two more types (String and Integer) "had a massively negative impact on the code."
Smell: Attempting to rewrite or restructure an entire module in one large change.
Rule: "One of the best ways to ruin a program is to make massive changes to its structure in the name of improvement. Some programs never recover from such 'improvements.'"
Fix: Make a large number of very tiny changes, each keeping the system working. Martin made approximately 30 tiny steps to transform the Args class, running tests between each.
V3: Separate Maps/Collections for Each Type Variant
Smell: Map<Character, Boolean> booleanArgs, Map<Character, String> stringArgs, Map<Character, Integer> intArgs -- one collection per type when they should be one unified collection.
Clean version: Map<Character, ArgumentMarshaler> marshalers -- a single map using polymorphism.
Refactoring path: Do not just delete the three maps. Add the new unified map alongside, migrate one type at a time (booleans first, then strings, then integers), delete each old map only after all its usages are migrated. Tests pass at every step.
V4: Type-Case Chains (instanceof / switch on type)
Smell: if (m instanceof BooleanArgumentMarshaler) ... else if (m instanceof StringArgumentMarshaler) ... else if (m instanceof IntegerArgumentMarshaler) in the setArgument method.
Clean version: m.set(currentArgument) -- a single polymorphic call. The type-case chain is eliminated entirely because each marshaler knows how to set itself.
Refactoring path demonstrated:
First, gather all marshalling into the base class
Create abstract set(Iterator) method in ArgumentMarshaler
Implement set(Iterator) in each derivative
Replace each type-case branch with m.set(currentArgument) one at a time
Remove the type-case entirely when all branches are replaced
V5: Base Class Accumulating Derivatives' Concerns
Smell: The ArgumentMarshaler base class had booleanValue, stringValue, and integerValue fields, plus getters/setters for all of them -- every derivative's data lived in the base.
Clean version: Each derivative (BooleanArgumentMarshaler, StringArgumentMarshaler, IntegerArgumentMarshaler) owns only its own data. The base is an interface with just set(Iterator) and get().
Refactoring path: Move fields one at a time from base to derivative. During migration, fields were temporarily protected (e.g., protected boolean booleanValue = false;) so derivatives could access them. Once get() was implemented in the derivative, the field moved down and became private. First move booleanValue down to BooleanArgumentMarshaler, run tests. Then stringValue down to StringArgumentMarshaler, run tests. Then integerValue, run tests. Then make base abstract. Then convert to interface.
V6: Mixed Exception Types and Error Handling Scattered Throughout
Smell: The rough draft threw ParseException from some methods and had an inner ArgsException class that was an empty stub (private class ArgsException extends Exception {}). Error codes, error parameters, and error messages were all scattered across the Args class.
Clean version: A dedicated ArgsException class in its own module with proper ErrorCode enum, constructors, error message formatting, and its own test class (ArgsExceptionTest).
Rule: Separate error handling concerns into their own module. The errorMessage method on ArgsException is technically an SRP violation. "Clearly it was a violation of the SRP to put the error message formatting into Args. Args should be about the processing of arguments, not about the format of the error messages. However, does it really make sense to put the error message formatting code into ArgsException?" Martin concludes: "Frankly, it's a compromise. Users who don't like the error messages supplied by ArgsException will have to write their own. But the convenience of having canned error messages already prepared for you is not insignificant."
Smell: falseIfNull(Boolean b), zeroIfNull(Integer i), blankIfNull(String s) -- these null-safety wrappers exist because the code uses raw maps that return null for missing keys.
Clean version: These disappear entirely. Each ArgumentMarshaler derivative has proper default values (false, 0, ""), and the static getValue methods handle the null-marshaler case.
Rule: If you have multiple helper methods that all handle null in type-specific ways, you are missing an abstraction that provides proper defaults.
V8: Boolean "valid" Flags and Error State Variables
Smell: private boolean valid = true; used as return value from parse(), combined with scattered valid = false; assignments throughout the error handling.
Clean version: Exceptions are thrown immediately on error. No boolean state tracking needed. The constructor either succeeds or throws ArgsException.
Rule: Replace boolean error flags with proper exception handling. If parse() can fail, it should throw, not set a flag.
V9: Raw Array Indexing with Manual Position Tracking
Smell: private int currentArgument; used as index into private String[] args; with manual currentArgument++ scattered throughout set methods, and ArrayIndexOutOfBoundsException catches for missing arguments.
Clean version: private Iterator<String> currentArgument; iterating over private List<String> argsList; with NoSuchElementException catches -- cleaner, more intention-revealing, and enables passing the iterator to ArgumentMarshaler derivatives.
Refactoring path: Convert String[] to List<String>, change the for loop to use listIterator(), change args[currentArgument] to currentArgument.next(), change exception types. About ten tiny steps, tests passing between each.
V10: Adding Features Without Refactoring First
Smell: Bulldozing forward with new features when the current structure is straining.
Example: Martin had added Boolean, then String, then Integer argument types. After Integer, the code was a mess. He had two more types to add (double, String array) but stopped.
Rule: "I had at least two more argument types to add, and I could tell that they would make things much worse. If I bulldozed my way forward, I could probably get them to work, but I'd leave behind a mess that was too large to fix. If the structure of this code was ever going to be maintainable, now was the time to fix it."
V11: Not Separating Modules into Their Own Files
Smell: ArgumentMarshaler and its derivatives as inner classes of Args. ArgsException as an inner class.
Clean version: ArgumentMarshaler is a public interface in its own file. Each derivative (BooleanArgumentMarshaler, StringArgumentMarshaler, IntegerArgumentMarshaler, DoubleArgumentMarshaler) is in its own file. ArgsException is in its own file with its own test.
Rule: "Much of good software design is simply about partitioning -- creating appropriate places to put different kinds of code. This separation of concerns makes the code much simpler to understand and maintain."
How to Fix
The Complete Refactoring Sequence Demonstrated in Chapter 14
This is the exact sequence of moves Martin used to transform the messy Args into the clean version, each step keeping tests passing:
Phase 1: Introduce the Polymorphic Skeleton
Add an empty ArgumentMarshaler class with a booleanValue field and getter/setter at the end of the file (this breaks nothing)
Change HashMap<Character, Boolean> to HashMap<Character, ArgumentMarshaler> for booleanArgs
Fix the code that broke: parseBooleanSchemaElement creates new BooleanArgumentMarshaler(), setBooleanArg calls .setBoolean(), getBoolean calls .getBoolean()
Run tests -- some fail due to NullPointerException (the null check moved)
Fix null check in getBoolean to check ArgumentMarshaler instead of Boolean
Repeat steps 3-6 for stringArgs and intArgs
Phase 2: Push Behavior Down to Derivatives
8. Make ArgumentMarshaler abstract with abstract set(String s) method
9. Implement set in BooleanArgumentMarshaler (sets booleanValue = true)
10. Replace setBoolean call with set("true") call
11. Remove old setBoolean from base class
12. Add abstract get() returning Object to ArgumentMarshaler
13. Implement get() in BooleanArgumentMarshaler (returns booleanValue)
14. Change getBoolean to cast result of get() call
15. Make get abstract, move booleanValue from base to BooleanArgumentMarshaler
16. Repeat steps 9-15 for String and Integer derivatives
Phase 3: Unify the Maps
17. Add a new unified Map<Character, ArgumentMarshaler> marshalers
18. In each parseXxxSchemaElement, add a second put to the marshalers map alongside the old map (double-entry: booleanArgs.put(elementId, m); marshalers.put(elementId, m);)
19. Change isXxxArg methods to use marshalers.get() with instanceof check
20. Eliminate duplicate calls to marshalers.get by extracting to a local variable (ArgumentMarshaler m = marshalers.get(argChar))
21. Inline the isXxxArg methods since they are now trivial (they just do return m instanceof XxxArgumentMarshaler)
22. Start using marshalers map in set functions (pass m instead of argChar), eliminating the old maps one by one
23. Change getBoolean to use marshalers map -- this is where the ClassCastException was discovered. When getBoolean used (Boolean) am.get() on a nonboolean argument, the FitNesse acceptance tests caught it (unit tests did not). Martin added a try/catch for ClassCastException and added a unit test to invoke FitNesse tests.
24. Delete booleanArgs map after all usages migrated. Then migrate and delete stringArgs and intArgs in the same manner.
25. Inline the three parseXxxSchemaElement methods into parseSchemaElement
Phase 4: Eliminate the Type-Case
The motivation: "I'd really like to get rid of that type-case up in setArgument [G23]. What I'd like in setArgument is a single call to ArgumentMarshaler.set." But to do this, the set functions need to be pushed down into the derivatives, and they use two instance variables: args and currentArg. To avoid passing both as function arguments (which is dirty [F1] -- "I'd rather pass one argument instead of two"), the solution is to convert the args array to a List and pass an Iterator.
Convert String[] args to List<String> argsList with Iterator<String> currentArgument -- "The following took me ten steps, passing all the tests after each. But I'll just show you the result."
Change args.length to argsList.size(), args[currentArgument] to currentArgument.next()
Change ArrayIndexOutOfBoundsException catches to NoSuchElementException
Remove the else return false from setArgument and add if (m == null) return false at the top -- "This change is important because we want to completely eliminate the if-else chain. Therefore, we needed to get the error condition out of it."
Pass currentArgument iterator to setBooleanArg(m, currentArgument) -- even though setBooleanArg does not use it (setBooleanArg just calls m.set("true"))
Add abstract set(Iterator<String> currentArgument) to ArgumentMarshaler as a SECOND abstract method alongside the old set(String s). Implement empty bodies in each derivative.
Move parsing logic from setBooleanArg/setStringArg/setIntArg into each derivative's set(Iterator) method, one at a time. Delete the old set functions and the old set(String) abstract method.
Replace type-case in setArgument with single m.set(currentArgument) -- "And so the coup de grace: The type-case can be removed! Touche!"
"Now we can get rid of some crufty functions in IntegerArgumentMarshaler and clean it up a bit" -- remove the old set(String) method, consolidate exception handling
Convert ArgumentMarshaler from abstract class to interface: private interface ArgumentMarshaler { void set(Iterator<String> currentArgument) throws ArgsException; Object get(); }
Phase 4b: Add Double via TDD (Proving the Refactored Code Works)
35. Write testSimpleDoublePresent test first -- TDD style: new Args("x##", new String[]{"-x","42.3"}) with assertEquals(42.3, args.getDouble('x'), .001)
36. Clean up parseSchemaElement to use elementTail.length() == 0 / .equals("*") / .equals("#") / .equals("##") dispatch
37. Write DoubleArgumentMarshaler class implementing ArgumentMarshaler with Double.parseDouble, catching NoSuchElementException (MISSING_DOUBLE) and NumberFormatException (INVALID_DOUBLE)
38. Add MISSING_DOUBLE and INVALID_DOUBLE to ErrorCode enum
39. Add getDouble function to Args
40. Write testInvalidDouble and testMissingDouble tests to verify error processing -- "And all the tests pass! That was pretty painless."
Phase 5: Separate Modules
41. Extract ArgsException into its own public class with ErrorCode enum -- "The exception code is pretty ugly and doesn't really belong in the Args class. We are also throwing a ParseException, which doesn't really belong to us. So let's merge all the exceptions into ArgsException and move it into its own module."
42. Change all ParseException throws to ArgsException throws throughout Args
43. Move error message formatting from Args to ArgsException.errorMessage() -- "The majority of the changes to the Args class were deletions. A lot of code just got moved out of Args and put into ArgsException. Nice."
44. Move each ArgumentMarshaler derivative into its own file -- "We also moved all the ArgumentMarshallers into their own files. Nicer!"
45. Write ArgsExceptionTest (Listing 14-14) to test error message formatting independently -- tests each error code's message format
Specific Refactoring Moves Used (Catalog)
Move
Description
Example from Chapter
Extract Class
Pull a new class out of an existing one
ArgumentMarshaler extracted from Args; ArgsException extracted from Args
Extract Interface
Convert abstract class to interface
ArgumentMarshaler became an interface with set(Iterator) and get()
Extract Method
Break a large method into smaller ones
parseSchemaElement broken into parseSchema + parseSchemaElement
Push Down Field
Move field from base to derivative
booleanValue moved from ArgumentMarshaler to BooleanArgumentMarshaler
Push Down Method
Move method from base to derivative
setBoolean/getBoolean moved down into BooleanArgumentMarshaler as set/get
Inline Method
Replace a method call with its body
isBooleanArg/isStringArg/isIntArg inlined into setArgument
Inline Class
Merge a class back (temporarily)
parseBooleanSchemaElement/parseStringSchemaElement/parseIntegerSchemaElement inlined back into parseSchemaElement
Rename Variable
Make names more intention-revealing
argumentMarshaller shortened to am for local scope [N5 -- "I didn't care for the long variable name; it was badly redundant and cluttered up the function. So I shortened it to am."]; booleanArgs/stringArgs/intArgs unified to marshalers
Change Type
Replace one type with a more appropriate one
String[] to List; int index to Iterator; HashMap<Char,Boolean> to HashMap<Char,ArgumentMarshaler>
Replace Type Code with Polymorphism
Use inheritance instead of type flags/checks [G23]
instanceof chain replaced with single m.set(currentArgument) call
Replace Error Code with Exception
Use exceptions instead of boolean flags
boolean valid + ErrorCode enum replaced with thrown ArgsException
Separate Module
Move class to its own file
ArgsException, each ArgumentMarshaler derivative
Add Temporary Scaffolding
Add code you know you will remove
Extra iterator parameter passed to setBooleanArg even though it does not use it, because setStringArg and setIntArg will need it; exception handling added to setBooleanArg just to be removed later
Remove Error Condition from Chain
Extract error guard to top of method
if (m == null) return false; moved to top of setArgument so the if-else type chain can be eliminated cleanly
Convert Array to List
Use List + Iterator instead of array + int index [F1]
String[] args becomes List<String> argsList to enable passing a single Iterator argument instead of two (array + index)
The "Getting Worse Before Better" Principle
Refactoring often makes code temporarily worse. Key examples from the chapter:
Adding exception handling into setBooleanArg just so you can take it out later ("Didn't we just put that exception processing in? Putting things in so you can take them out again is pretty common in refactoring.")
Passing an Iterator parameter to setBooleanArg that it does not need ("Why did we pass that iterator when setBooleanArg certainly doesn't need it? Because setIntArg and setStringArg will! And because I want to deploy all three of these functions through an abstract method in ArgumentMarshaller, I need to pass it to setBooleanArg.")
Having both old maps AND new marshalers map simultaneously during migration -- adding a second marshalers.put(elementId, m) call in each parseXxxSchemaElement alongside the old map put
The intermediate state (Listing 14-12) was "a bit disappointing" even after significant refactoring work: "After all that work, this is a bit disappointing. The structure is a bit better, but we still have all those variables up at the top; there's still a horrible type-case in setArgument; and all those set functions are really ugly. Not to mention all the error processing."
The set(String s) abstract method was an intermediate step. The BooleanArgumentMarshaler's implementation ignores the string parameter entirely (booleanValue = true;) because it was put there knowing String and Integer marshalers would use it. Later, the signature changed to set(Iterator<String> currentArgument).
Making get() return Object required ugly casting in callers ((Boolean) am.get(), (String) am.get(), (Integer) am.get()) -- this is acknowledged as ugly but necessary for the interface.
This is normal. "The smallness of the steps and the need to keep the tests running means that you move things around a lot. Refactoring is a lot like solving a Rubik's cube. There are lots of little steps required to achieve a large goal. Each step enables the next."
The "Coup de Grace" Moment
After deploying set(Iterator<String> currentArgument) to all three derivatives, the type-case can finally be removed entirely:
"And so the coup de grace: The type-case can be removed! Touche!"
The setArgument method goes from an instanceof chain with three branches to simply:
m.set(currentArgument);
return true;
This is the payoff of all the incremental work -- a single polymorphic call replaces all the type-checking.
The Two Intermediate set Methods
During the refactoring, the ArgumentMarshaler temporarily had TWO abstract set methods:
public abstract void set(Iterator<String> currentArgument) throws ArgsException; -- the new one
public abstract void set(String s) throws ArgsException; -- the old one
The derivatives implemented both, with empty bodies for the new set(Iterator) initially. The old set(String) was gradually phased out as the new set(Iterator) took over the actual work. Eventually the old set(String) was removed entirely.
Self-Improvement Protocol
Before Writing New Code
Ask: "Will I need to add more variants/types/cases to this later?"
If yes, design for polymorphism from the start -- but do not over-engineer. Start simple, refine when the second case arrives.
Write tests first (or at least alongside) so you have a safety net for future refactoring.
While Adding Features
After each feature addition, assess: "Is the code still clean, or is complexity growing faster than functionality?"
Watch the warning signs: more instance variables, duplicated patterns, longer methods, more type-checking.
If you sense the tipping point approaching (like Args after adding Integer), STOP adding features and REFACTOR.
"If I bulldozed my way forward, I could probably get them to work, but I'd leave behind a mess that was too large to fix."
During Refactoring
Never refactor without tests. Period.
Make one tiny change. Run tests. Make the next tiny change. Run tests.
If tests break, fix immediately -- do not accumulate broken tests.
Accept that the code may get temporarily worse (more files, more parameters, duplicate structures) on the way to better.
Keep the system running at all times. "Using TDD, I am not allowed to make a change to the system that breaks that system."
Aim for approximately 30 tiny steps for a significant refactoring, not 3 big steps.
After Refactoring
Verify the final code reads top-to-bottom without jumping around. "Notice that you can read this code from the top to the bottom without a lot of jumping around or looking ahead."
Verify adding a new type/variant requires only: one new derivative class, one new case in parseSchemaElement, one new getXxx method, and one new ErrorCode and error message. Minimal touch points. "It should be obvious how you would add a new argument type, such as a date argument or a complex number argument, and that such an addition would require a trivial amount of effort."
Verify each class has a single responsibility: Args parses arguments, ArgsException handles errors, each Marshaler handles its type.
Verify there are no leftover null-checking helper methods, boolean flags, or manual index tracking.
Verify the final Listing 14-16 has the clean parseArgumentStrings using currentArgument = argsList.listIterator() with currentArgument.hasNext(), and parseArgument handling non-dash arguments via currentArgument.previous(); break; -- backing up the iterator for the caller.
Verify the final setArgument catches ArgsException, sets e.setErrorArgumentId(argChar), and re-throws -- enriching the exception with context before propagating.
The Boy Scout Rule Applied
"If you made a mess in a module in the morning, it is easy to clean it up in the afternoon. Better yet, if you made a mess five minutes ago, it's very easy to clean it up right now."
"So the solution is to continuously keep your code as clean and simple as it can be. Never let the rot get started."
Key Metrics to Watch
Instance variable count: The rough draft had 11. The clean version has 4-5. If your class has more than 5-7 instance variables, it likely has too many responsibilities.
Number of places to change for a new variant: In the rough draft, adding a new argument type required changes in 3+ places with duplicated patterns. In the clean version, it requires a new derivative class + one line in parseSchemaElement.
Number of type-checking conditionals: Each instanceof/typeof/switch-on-type is a candidate for polymorphism. The clean version has zero in the set path.
Number of parallel collections: Multiple maps/lists tracking the same entities by different types. Clean code unifies them behind a single abstraction.
Ratio of error-handling code to business logic: If error handling dominates a class, extract it into its own module (like ArgsException).
The "About 30 Tiny Steps" Benchmark
"So now we have completely separated the exception and error code from the Args module. (See Listing 14-13 through Listing 14-16.) This was achieved through a series of about 30 tiny steps, keeping the tests passing between each step."
This is the benchmark Martin sets for a significant refactoring: approximately 30 tiny steps, not 3 big leaps. Each step should be small enough that it either works immediately or breaks in an obvious, quickly-fixable way.
The "Left as an Exercise" Note
Martin notes that the final Listing 14-16 is "within striking distance of the final solution that appeared at the start of this chapter" but is not identical to Listing 14-2. "I'll leave the final transformations to you as an exercise." The remaining differences include: the getBoolean/getString/getInt in Listing 14-16 still use try/catch with ClassCastException, while the final Listing 14-2 uses static getValue methods on the marshalers (like BooleanArgumentMarshaler.getValue(marshalers.get(arg))). Also, Listing 14-16 still has usage() and has() methods that the final version also has, and the Double marshalers are mentioned but derivatives like StringArrayArgumentMarshaler only appear in Listing 14-2.
Programmers Who Fear Refactoring
"They may fear that they don't have time to improve the structure and design of their code, but I disagree." Martin pushes back against the excuse of not having time to refactor, arguing that the cost of NOT refactoring is far higher than the cost of doing it.
The Fundamental Truth
"Nothing has a more profound and long-term degrading effect upon a development project than bad code. Bad schedules can be redone, bad requirements can be redefined. Bad team dynamics can be repaired. But bad code rots and ferments, becoming an inexorable weight that drags the team down. Time and time again I have seen teams grind to a crawl because, in their haste, they created a malignant morass of code that forever thereafter dominated their destiny."
"Of course bad code can be cleaned up. But it's very expensive. As code rots, the modules insinuate themselves into each other, creating lots of hidden and tangled dependencies. Finding and breaking old dependencies is a long and arduous task. On the other hand, keeping code clean is relatively easy."
