clean-code-ch15-junit-internals

name	clean-code-ch15-junit-internals
description	Code quality checker based on Clean Code Ch15: JUnit Internals — checks conditional encapsulation, negative-to-positive conditionals, naming precision, dead code removal, proper scoping, hidden temporal coupling, consistent conventions, and systematic refactoring technique

Clean Code Chapter 15: JUnit Internals -- Code Quality Checker

This skill encodes every refactoring move and principle demonstrated in Chapter 15 of Clean Code, which performs a systematic case-study refactoring of JUnit's ComparisonCompactor class. JUnit was created by Kent Beck and Eric Gamma on a plane to Atlanta; ComparisonCompactor is its clever utility for identifying string comparison errors (given ABCDE and ABXDE, it generates <...B[X]D...>). The chapter embodies the Boy Scout Rule: leave the code a little better than you found it -- even when the code is already pretty good.

Chapter Listings Reference

• Listing 15-1: ComparisonCompactorTest.java -- the test suite with 100% code coverage (every line, if, for loop exercised)
• Listing 15-2: ComparisonCompactor.java (Original) -- the starting code with f-prefixed members, unencapsulated conditionals, fSuffix+1 noise
• Listing 15-3: ComparisonCompator.java (defactored) -- the anti-pattern showing what compressed/cryptic code looks like
• Listing 15-4: ComparisonCompactor.java (interim) -- after suffixIndex->suffixLength, charFromEnd, suffixOverlapsPrefix extractions
• Listing 15-5: ComparisonCompactor.java (final) -- the fully refactored result with analysis/synthesis separation, StringBuilder, topological ordering

When to Use

Apply this skill when:

• Reviewing or refactoring existing code that already works and has tests passing
• Performing code review on any module, especially utility/framework code
• Looking for incremental improvements in code that appears "good enough"
• Cleaning up code before committing (Boy Scout Rule)
• Evaluating whether variable names, method names, and conditionals are as clear as possible
• Checking for hidden temporal couplings between methods
• Looking for dead code, redundant conditionals, or unnecessary complexity
• Wanting a systematic, test-driven approach to incremental refactoring

Foundational Principles

P1: The Boy Scout Rule Applied to Clean Code

Even code that is already well-structured, nicely partitioned, reasonably expressive, and simple can be improved. No module is immune from improvement. Always leave the code a little better than you found it.

P2: Tests Enable Fearless Refactoring

Before refactoring anything, ensure 100% code coverage. Every line of code, every if statement, every for loop must be executed by the tests. This gives a high degree of confidence that refactoring will not break behavior. Run tests after every change.

P3: Refactoring Is Iterative Trial and Error

Refactoring is not a linear process. Often one refactoring leads to another that leads to undoing the first. Extracted methods may get inlined back. The sense of boolean expressions may be flipped back and forth. This is normal and expected -- keep iterating until you converge on something professional. The chapter demonstrates this concretely: in the final version (Listing 15-5), the author reversed several intermediate decisions -- compactExpectedAndActual() was inlined back into formatCompactedComparison, and the positive canBeCompacted() was replaced with shouldBeCompacted() delegating to !shouldNotBeCompacted(). These reversals were deliberate improvements discovered through continued iteration.

P4: Topological Ordering of Functions

The final module should be separated into analysis functions and synthesis functions. Each function's definition should appear just after it is used. All analysis functions appear first, and all synthesis functions appear last. This is the "newspaper metaphor" for function ordering.

P5: Small Changes, Continuous Verification

Make one small change at a time. Run all tests after each change. Never batch multiple unrelated refactorings into a single step. This keeps the code working at every stage.

Checklist

When reviewing code, check EVERY item below. Each maps to a specific refactoring demonstrated in the chapter:

Naming

• [N6] No scope-encoding prefixes: Remove Hungarian notation and scope-encoding prefixes like f on member variables (e.g., fContextLength -> contextLength, fExpected -> expected). Modern IDEs make this encoding redundant.
• [N7] Names describe behavior accurately: Function names must describe what the function actually does, not a subset of its behavior. If compact() returns a formatted message (not just compacted strings), rename to formatCompactedComparison(). The name should not hide side effects.
• [N1] Names reflect what variables actually represent: If a variable holds a length but is named "index," rename it. If "index" and "length" are synonymous in context, prefer the one that matches how the variable is actually used (e.g., suffixIndex -> suffixLength and prefixIndex -> prefixLength when the values represent lengths, not zero-based indices). Apply the rename consistently -- the chapter renames BOTH prefix and suffix variables to use "length."
• [N4] Disambiguate variables in the same scope: When local variables shadow or match member variable names, make them unambiguous. If expected is both a member and a local, rename the local to compactExpected to clarify which is which.

Conditionals

• [G28] Encapsulate conditionals: Complex boolean expressions in if statements should be extracted into well-named methods. if (expected == null || actual == null || areStringsEqual()) becomes if (shouldNotCompact()) or if (canBeCompacted()).
• [G29] Prefer positive conditionals over negatives: Negatives are slightly harder to understand than positives. Invert negative conditionals: if (shouldNotCompact()) becomes if (canBeCompacted()) with the if/else branches swapped. The extracted boolean method uses != and && instead of == and ||.

Function Structure

• [G30] Functions should do one thing: If a function both decides whether to act AND performs the action, split it. The formatCompactedComparison function should only format; extract the actual compacting work into compactExpectedAndActual().
• [G11] Consistent conventions in return values: If some functions use side effects to set member variables and others return values, pick one convention. When findCommonPrefix() and findCommonSuffix() set member variables as side effects but compactString() returns a value, change the find* methods to return values for consistency.
• [G9] Remove dead/redundant conditionals: If refactoring reveals that a conditional guard can never be false (e.g., if (suffixLength > 0) when suffixLength can never be less than 1), remove it. Comment out the conditional, run tests -- if they pass, delete it permanently.
• Extract method for complex composition: When compactString has multiple inline expressions doing substring extraction with context and delimiters, extract each piece into its own named method (startingEllipsis(), startingContext(), delta(), endingContext(), endingEllipsis()). Use a builder pattern for clarity.

Temporal Coupling

• [G31] Expose hidden temporal couplings: If function B depends on function A being called first (because A sets state that B reads), make that dependency explicit. Options:
  • Pass the output of A as an argument to B: findCommonSuffix(prefixIndex) -- but this is arbitrary [G32] and fragile. Another programmer might undo it because there is no indication that the parameter is really needed for ordering.
  • Better: combine into a single method findCommonPrefixAndSuffix() that calls A internally before doing B's work. This makes the ordering impossible to violate.

Scope and Visibility

• Promote locals to members only when necessary: When extracting methods forces you to promote local variables to member variables, do so -- but be aware this is a tradeoff. Only promote when the variable genuinely needs to be shared across methods. Note: the chapter promoted compactExpected and compactActual to members in an intermediate step, then reversed this in the final version (Listing 15-5) where they remain locals in formatCompactedComparison. This reversal illustrates that member promotion should be a last resort.
• Keep variables at the narrowest necessary scope: Do not make something a member variable if it can remain local. The prefixLength and suffixLength member variables are needed because multiple methods reference them, but contextStart, contextEnd, deltaStart, deltaEnd are kept as locals within their respective methods.

Code Clarity

• Replace magic index arithmetic with named methods: When you see s.charAt(s.length() - i - 1), extract it to charFromEnd(s, i) -- the name explains the intent that the arithmetic obscures.
• Replace +1/-1 index adjustments with clearer abstractions: If changing a variable from zero-based to one-based eliminates scattered +1 adjustments throughout the code (as happened with suffixIndex -> suffixLength), make that change. Eliminate the root cause of +1 noise, not just the symptoms.
• Use StringBuilder with fluent chaining for string assembly: When a method composes a string from multiple fragments, use new StringBuilder().append(x).append(y)...toString() rather than string concatenation with intermediate variables. Each .append() call should receive a well-named method.

Violations to Detect

VIOLATION: Scope-Encoding Prefixes on Member Variables

Smell: Member variables with f, m_, _ prefixes encoding their scope. Example: private String fExpected; private int fContextLength; Fix: Remove the prefix. private String expected; private int contextLength; Reference: [N6] -- environments make scope encoding redundant.

VIOLATION: Unencapsulated Complex Conditionals

Smell: An if statement with multiple || or && conditions mixing null checks and business logic. Example: if (expected == null || actual == null || areStringsEqual()) Fix: Extract to a method with an intent-revealing name: if (shouldNotCompact()) or better, if (canBeCompacted()). Reference: [G28] -- encapsulate conditionals.

VIOLATION: Negative Conditional as Primary Path

Smell: The primary logic flow is gated by a negative check like shouldNotCompact(), forcing the reader to mentally invert. Example:

if (shouldNotCompact())
  return earlyResult;
// main logic here

Fix: Invert to positive: if (canBeCompacted()) { // main logic } else { return earlyResult; }. The extracted method uses positive logic: return expected != null && actual != null && !areStringsEqual(); Reference: [G29] -- avoid negative conditionals.

VIOLATION: Function Name Does Not Match Full Behavior

Smell: A function named compact() that also formats a comparison message, not just compacting strings. Example: public String compact(String message) that returns Assert.format(message, compactExpected, compactActual). Fix: Rename to formatCompactedComparison(String message) -- the name now describes the full behavior: it formats a comparison after compacting. Reference: [N7] -- names should describe side effects.

VIOLATION: Function Does Multiple Things

Smell: A single function that checks preconditions, computes values, AND formats output. Example: formatCompactedComparison that calls findCommonPrefixAndSuffix(), calls compactString() for both inputs, AND calls Assert.format(). Fix: Extract the computation into compactExpectedAndActual(). The public method should only coordinate: check canBeCompacted(), call compactExpectedAndActual(), then format. Reference: [G30] -- functions should do one thing.

VIOLATION: Ambiguous Variable Names in Same Scope

Smell: Local variables with the same name as member variables, causing confusion about which is being referenced. Example: Member expected and local String expected = compactString(this.expected) requiring this. disambiguation. Fix: Rename the local to compactExpected and the member stays expected. Each name is now unambiguous without needing this.. Reference: [N4] -- unambiguous names.

VIOLATION: Inconsistent Function Conventions

Smell: Some functions set member variables as side effects while similar functions return values. Example: findCommonPrefix() sets this.prefix as void, but compactString() returns a value. Fix: Change findCommonPrefix() and findCommonSuffix() to return their computed values: prefixIndex = findCommonPrefix(); suffixIndex = findCommonSuffix(); Reference: [G11] -- be consistent with conventions.

VIOLATION: Hidden Temporal Coupling Between Functions

Smell: Function B silently depends on function A having been called first, because A sets a member variable that B reads, with no compile-time enforcement. Example: findCommonSuffix() reads prefixIndex which is only valid after findCommonPrefix() has run. Calling them out of order silently produces wrong results. Fix options (from worst to best):

1. Pass the dependency as an argument: findCommonSuffix(prefixIndex) -- establishes ordering but is arbitrary [G32] and fragile. Another programmer might undo it because there is no indication the parameter is really needed for ordering purposes.
2. Combine into findCommonPrefixAndSuffix() which calls findCommonPrefix() first internally -- makes the ordering structurally impossible to violate. Reference: [G31] -- hidden temporal couplings; [G32] -- don't be arbitrary.

VIOLATION: Variable Name Misrepresents Its Nature (Index vs Length)

Smell: A variable named suffixIndex that actually represents a length (is 1-based, not 0-based). Similarly, prefixIndex represents a length. Example: suffixIndex is 1-based and used as a length throughout the code, causing spurious +1 adjustments everywhere. Fix: Rename both: suffixIndex -> suffixLength, prefixIndex -> prefixLength. Change the suffix loop to start at suffixLength = 0 (zero-based length). Extract charFromEnd(s, i) to absorb the -1 adjustment cleanly. Update suffixOverlapsPrefix() to use <= instead of <. The chapter notes that "index" and "length" are synonymous in the prefix case, but "length" is more consistent and accurate for both. Reference: [N1] -- choose names that accurately describe what the variable represents.

VIOLATION: Scattered +1/-1 Index Arithmetic

Smell: Multiple +1 or -1 adjustments scattered across methods, all compensating for a naming/representation mismatch. Example: fSuffix + 1 appearing in compactString, computeCommonSuffix, etc. Fix: Fix the root cause (rename index to length, adjust base), then the +1s disappear. Extract charFromEnd(String s, int i) { return s.charAt(s.length() - i - 1); } to absorb remaining arithmetic. Reference: [G33] -- encapsulate boundary conditions.

VIOLATION: Redundant/Dead Conditional Guards (and Latent Bug Discovery)

Smell: An if guard that can never be false given the current logic, left over from a previous version of the code. Example: if (suffixLength > 0) when suffixLength starts at 0 and is only incremented, so after the loop it is always >= 0 -- but the code below the if works correctly for 0 too. Fix: Comment it out, run all tests. If they pass, the guard is dead -- delete it. Simplify the function to just return the composed result without conditionals. Key insight: The chapter reveals that the original if (fSuffix > 0) was "probably a bug" -- because fSuffix was 1-based, it could never be less than one, making the guard nonfunctional. After renaming to zero-based suffixLength, the > 0 would have made semantic sense, but by then the guard was provably unnecessary. This demonstrates how refactoring can expose latent bugs that were previously invisible due to naming/representation mismatches. Reference: [G9] -- dead code / unnecessary complexity.

VIOLATION: Opaque String Assembly

Smell: A method that builds a string through a series of substring calls with complex index arithmetic, making it hard to understand what each piece represents. Example:

String result = DELTA_START + source.substring(prefixLength, source.length() - suffixLength) + DELTA_END;
if (prefixLength > 0)
  result = computeCommonPrefix() + result;
if (suffixLength > 0)
  result = result + computeCommonSuffix();

Fix: Use StringBuilder with well-named methods for each fragment:

return new StringBuilder()
  .append(startingEllipsis())
  .append(startingContext())
  .append(DELTA_START)
  .append(delta(s))
  .append(DELTA_END)
  .append(endingContext())
  .append(endingEllipsis())
  .toString();

Each method is tiny and self-documenting.

How to Fix

Systematic Refactoring Protocol (demonstrated step-by-step in the chapter)

Step 1: Ensure full test coverage Verify 100% code coverage. Every line, every if, every for loop is exercised. If not, write additional tests before refactoring.

Step 2: Remove encoding noise Strip scope-encoding prefixes (f, m_, _) from all member variables. This is purely mechanical and risk-free.

Step 3: Encapsulate conditionals Find complex boolean expressions in if statements. Extract each into a method with an intent-revealing name. Start with the negative form if that is what the logic requires, then...

Step 4: Convert negatives to positives Invert the conditional to prefer the positive form. Swap the if/else branches accordingly. The extracted method should express the positive condition using != and &&.

Step 5: Rename functions to match full behavior Read each function's body. Does the name describe everything it does? If it formats AND compacts, the name should say so. If naming the function accurately requires "And" in the name, the function may be doing too much (proceed to Step 6).

Step 6: Extract to enforce single responsibility If a function does two things, extract one into a new method. The public method coordinates; private methods do the work. Each function should do one thing.

Step 7: Disambiguate variables When extraction causes local-vs-member name collisions, rename to make each variable unambiguous without this. qualifiers.

Step 8: Enforce consistent conventions If some helper functions return values and others set member variables, pick one convention. Prefer returning values when the result is used in exactly one place. Use side effects (setting member variables) only when multiple methods need the computed value.

Step 9: Expose temporal couplings Find functions that must be called in a specific order. Make the ordering structurally enforced, either through argument passing or by combining into a single orchestrating method that calls them in the correct sequence.

Step 10: Fix naming/representation mismatches If a variable's name says "index" but it behaves like a "length" (or vice versa), rename it. Fix the base (0-based vs 1-based) to match the new name. This may cascade changes through multiple methods -- that is expected and usually simplifies them.

Step 11: Extract boundary arithmetic into named methods Replace s.charAt(s.length() - i - 1) with charFromEnd(s, i). Replace complex loop conditions with suffixOverlapsPrefix(suffixLength). Each piece of boundary arithmetic gets a name that explains what it computes.

Step 12: Eliminate dead conditionals After renaming and restructuring, some if guards may be provably always-true or always-false. Comment them out, run tests. If tests pass, delete permanently.

Step 13: Decompose string assembly into named fragments Replace imperative string building (with intermediate variables and conditional appends) with a fluent builder where each .append() receives a well-named method that returns its fragment (or empty string when not applicable).

Step 14: Topologically order the final result Arrange methods so that each function's definition appears just after it is first used. Group analysis functions (finding, computing) before synthesis functions (formatting, composing). The code reads like a newspaper: high-level first, details below.

Step 15: Verify and reflect Run all tests one final time. Review whether any earlier refactoring should be undone in light of later changes. Refactoring is iterative -- some intermediate extractions may need to be inlined back if the final structure makes them unnecessary.

Self-Improvement Protocol

After Every Refactoring Session

1. Did I run tests after every single change? If not, adopt smaller steps next time.
2. Did I undo any refactoring that a later step made unnecessary? Good -- that is normal.
3. Is every function name accurate to its full behavior?
4. Are there any hidden temporal couplings remaining?
5. Are all conditionals expressed positively?
6. Is every variable name accurate to what it represents (not what it was copied from)?
7. Does the code read top-to-bottom like a narrative?

Refactoring Quality Metrics

• Coverage: Must remain 100% throughout. If any test breaks, immediately revert the last change.
• Method length: After refactoring, most methods should be 1-5 lines.
• Conditional complexity: No method should contain a raw multi-clause boolean expression. All should be encapsulated in named methods.
• Naming accuracy: Every function name should be verifiable by reading only its body. No hidden side effects beyond what the name declares.
• Temporal safety: No function should silently depend on another having been called first without structural enforcement.

Key Insight from the Chapter

The original ComparisonCompactor was already pretty good code -- nicely partitioned, reasonably expressive, simple in structure. Yet the chapter found over a dozen improvements. The lesson: there is always room to improve, and the discipline of looking carefully at working code with fresh eyes is what separates professional craftsmanship from "good enough."

The Final Version (Listing 15-5) -- Key Structural Reversals

The final ComparisonCompactor.java differs from the intermediate refactoring steps in several important ways that demonstrate P3 (iterative trial and error):

1. shouldBeCompacted() wraps !shouldNotBeCompacted(): Rather than the intermediate canBeCompacted() with positive logic (!= null && != null && !areStringsEqual()), the final version keeps the negative-logic method shouldNotBeCompacted() (which uses == null || == null || expected.equals(actual)) and wraps it with a positive-named shouldBeCompacted() that simply returns !shouldNotBeCompacted(). This was a deliberate reversal.
2. compactExpectedAndActual() was inlined back: The intermediate extraction of compactExpectedAndActual() was reversed. In the final version, formatCompactedComparison directly contains the calls to findCommonPrefixAndSuffix(), compact(expected), and compact(actual). The compactExpected and compactActual remain local variables in formatCompactedComparison, NOT promoted to member variables (the member promotion was an intermediate step that was undone).
3. compactString() was renamed to compact(): The final method that builds the string via StringBuilder is called compact(String s), not compactString(String source). This is a cleaner, shorter name.
4. prefixIndex was renamed to prefixLength: Just as suffixIndex became suffixLength, the final version also uses prefixLength (not prefixIndex) throughout -- both member variable and local uses.

The Defactored Anti-Pattern (Listing 15-3)

The chapter shows what the code would look like if "defactored" -- compressed into fewer methods with short cryptic names (ctxt, s1, s2, pfx, sfx, cmp1, cmp2). This demonstrates that:

• Abbreviating names destroys readability
• Inlining methods destroys separation of concerns
• Compact code is not the same as clean code
• The defactored version is shorter but far harder to understand, modify, or debug