clean-code-ch14-successive-refinement

name	clean-code-ch14-successive-refinement
description	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

Clean Code Chapter 14: Successive Refinement -- Code Quality Checker

Core Philosophy

"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
14-14	ArgsExceptionTest.java	Tests error message formatting independently: testUnexpectedMessage, testMissingStringMessage, testInvalidIntegerMessage, testMissingIntegerMessage, testInvalidDoubleMessage, testMissingDoubleMessage
14-15	ArgsException.java (final)	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:

1. 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
2. "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."
3. The rough draft is shown (Listing 14-8) -- the messy working version after Boolean + String + Integer were added
4. The Boolean-only version (Listing 14-9) -- showing the code was once clean before it degraded
5. The Boolean+String version (Listing 14-10) -- showing the mess starting to grow
6. "So I Stopped" -- the critical decision point
7. "On Incrementalism" -- the philosophy of tiny steps
8. The complete refactoring journey (Listings 14-11 through 14-12 and inline code) -- every step shown
9. Adding double via TDD -- proving the refactored code is easy to extend
10. Separating ArgsException -- the final cleanup
11. The completely separated final code (Listings 14-13 through 14-16)
12. 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."

V2: Big-Bang Refactoring (Massive Structural Changes)

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:

1. First, gather all marshalling into the base class
2. Create abstract set(Iterator) method in ArgumentMarshaler
3. Implement set(Iterator) in each derivative
4. Replace each type-case branch with m.set(currentArgument) one at a time
5. 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."

V7: Null-Checking Helper Methods Masking Missing Abstractions

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

1. Add an empty ArgumentMarshaler class with a booleanValue field and getter/setter at the end of the file (this breaks nothing)
2. Add empty derivative classes: BooleanArgumentMarshaler, StringArgumentMarshaler, IntegerArgumentMarshaler (all extending ArgumentMarshaler, all empty)
3. Change HashMap<Character, Boolean> to HashMap<Character, ArgumentMarshaler> for booleanArgs
4. Fix the code that broke: parseBooleanSchemaElement creates new BooleanArgumentMarshaler(), setBooleanArg calls .setBoolean(), getBoolean calls .getBoolean()
5. Run tests -- some fail due to NullPointerException (the null check moved)
6. Fix null check in getBoolean to check ArgumentMarshaler instead of Boolean
7. 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.

25. 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."
26. Change args.length to argsList.size(), args[currentArgument] to currentArgument.next()
27. Change ArrayIndexOutOfBoundsException catches to NoSuchElementException
28. 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."
29. Pass currentArgument iterator to setBooleanArg(m, currentArgument) -- even though setBooleanArg does not use it (setBooleanArg just calls m.set("true"))
30. 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.
31. 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.
32. Replace type-case in setArgument with single m.set(currentArgument) -- "And so the coup de grace: The type-case can be removed! Touche!"
33. "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
34. 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:

1. public abstract void set(Iterator<String> currentArgument) throws ArgsException; -- the new one
2. 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

1. Ask: "Will I need to add more variants/types/cases to this later?"
2. If yes, design for polymorphism from the start -- but do not over-engineer. Start simple, refine when the second case arrives.
3. Write tests first (or at least alongside) so you have a safety net for future refactoring.

While Adding Features

1. After each feature addition, assess: "Is the code still clean, or is complexity growing faster than functionality?"
2. Watch the warning signs: more instance variables, duplicated patterns, longer methods, more type-checking.
3. If you sense the tipping point approaching (like Args after adding Integer), STOP adding features and REFACTOR.
4. "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

1. Never refactor without tests. Period.
2. Make one tiny change. Run tests. Make the next tiny change. Run tests.
3. If tests break, fix immediately -- do not accumulate broken tests.
4. Accept that the code may get temporarily worse (more files, more parameters, duplicate structures) on the way to better.
5. Keep the system running at all times. "Using TDD, I am not allowed to make a change to the system that breaks that system."
6. Aim for approximately 30 tiny steps for a significant refactoring, not 3 big steps.

After Refactoring

1. 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."
2. 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."
3. Verify each class has a single responsibility: Args parses arguments, ArgsException handles errors, each Marshaler handles its type.
4. Verify there are no leftover null-checking helper methods, boolean flags, or manual index tracking.
5. 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.
6. 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."