Code quality checker based on Clean Code Ch3: Functions — checks function size, single responsibility, abstraction levels, argument counts, side effects, command-query separation, error handling, and DRY
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 Ch3: Functions — checks function size, single responsibility, abstraction levels, argument counts, side effects, command-query separation, error handling, and DRY
Reviewing any function/method in a pull request or code review
Writing new functions or refactoring existing ones
Encountering functions that feel hard to read, test, or understand
Performing code quality audits on a module or file
A function exceeds 20 lines, has more than 2 arguments, or mixes abstraction levels
Before committing code that adds or modifies functions
Checklist
Run through EVERY item below for each function under review. A function must pass ALL checks to be considered clean.
1. SIZE: Is the function small enough?
Function is 20 lines or fewer (ideal: 2-5 lines, as in Kent Beck's Sparkle program)
Lines are no longer than 150 characters (prefer shorter)
Function fits on a single screen without scrolling
Blocks inside if, else, while, for, switch, and try/catch are one line long -- typically a function call with a descriptive name
Indentation level is no greater than one or two (no deeply nested structures)
The function cannot be meaningfully shortened further without mere restatement
Book reference: "The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that." In the eighties, the rule was that a function should be no bigger than a screen-full (VT100 screens: 24 lines by 80 columns, with 4 lines for admin). Today with 150 characters per line and 100+ lines on screen, functions should still be very small. "Lines should not be 150 characters long. Functions should not be 100 lines long. Functions should hardly ever be 20 lines long." Martin visited Kent Beck in 1999 and saw a Java/Swing program (Sparkle) where every function was just two, three, or four lines long. Each was transparently obvious, told a story, and led to the next in a compelling order.
2. DO ONE THING: Does the function do exactly one thing?
The function can be described as a brief TO paragraph: "TO [FunctionName], we [single coherent action]."
All statements in the function are one level of abstraction below the stated name of the function
You cannot extract another function from it with a name that is NOT merely a restatement of its implementation
The function cannot be reasonably divided into sections (e.g., "declarations," "initializations," "sieve" as in the generatePrimes function in Listing 4-7) -- if it can be divided into sections, it does more than one thing
The "three things" test (Listing 3-3): Does this re-refactored function do one thing or three?
It arguably does three things: (1) determining whether the page is a test page, (2) if so including setups and teardowns, (3) rendering the page in HTML. But notice that all three steps are one level of abstraction below the function name. The function is doing one thing: including setups and teardowns into a test page. A function named includeSetupsAndTeardownsIfTestPage would merely be a restatement of the code, not a meaningful extraction -- confirming it does one thing.
Book reference -- the TO paragraph test:
TO RenderPageWithSetupsAndTeardowns, we check to see whether the page is a test page and if so, we include the setups and teardowns. In either case we render the page in HTML.
If the steps described are one level below the function name, the function is doing one thing. If they jump across multiple abstraction levels, it is doing too much.
3. ONE LEVEL OF ABSTRACTION: Are all statements at the same abstraction level?
No mixing of high-level concepts (e.g., getHtml()) with intermediate details (e.g., String pagePathName = PathParser.render(pagePath)) and low-level operations (e.g., .append("\n")) in the same function
Each function introduces the next, reading like a top-down narrative
Once details are mixed with essential concepts, more and more details tend to accrete within the function (the "broken windows" effect)
The Stepdown Rule: Code should read like a top-down set of TO paragraphs, each describing the current level and referencing the next level down:
To include the setups and teardowns, we include setups, then we include the test page content, and then we include the teardowns.
To include the setups, we include the suite setup if this is a suite, then we include the regular setup.
To include the suite setup, we search the parent hierarchy for the "SuiteSetUp" page and add an include statement with the path of that page.
To search the parent...
4. SWITCH STATEMENTS: Are switch/if-else chains handled properly?
Switch statements (and equivalent if/else chains) appear at most once for a given type discrimination
They are buried in a low-level class (typically an Abstract Factory) and never seen by the rest of the system
They are used only to create polymorphic objects -- all subsequent behavior is dispatched polymorphically through an interface/abstract class
They do NOT violate the Single Responsibility Principle (more than one reason to change)
They do NOT violate the Open/Closed Principle (must be modified when new types are added)
Bad -- switch in business logic (Listing 3-4):
public Money calculatePay(Employee e)throws InvalidEmployeeType {
switch (e.type) {
case COMMISSIONED: return calculateCommissionedPay(e);
case HOURLY: return calculateHourlyPay(e);
case SALARIED: return calculateSalariedPay(e);
default: thrownewInvalidEmployeeType(e.type);
}
}
Problem: isPayday(Employee e, Date date) and deliverPay(Employee e, Money pay) and unlimited other functions would need the same switch structure.
Good -- switch buried in factory (Listing 3-5):
publicabstractclassEmployee {
publicabstractbooleanisPayday();
publicabstract Money calculatePay();
publicabstractvoiddeliverPay(Money pay);
}
publicinterfaceEmployeeFactory {
public Employee makeEmployee(EmployeeRecord r)throws InvalidEmployeeType;
}
publicclassEmployeeFactoryImplimplementsEmployeeFactory {
public Employee makeEmployee(EmployeeRecord r)throws InvalidEmployeeType {
switch (r.type) {
case COMMISSIONED: returnnewCommissionedEmployee(r);
case HOURLY: returnnewHourlyEmployee(r);
case SALARIED: returnnewSalariedEmployee(r);
default: thrownewInvalidEmployeeType(r.type);
}
}
}
5. DESCRIPTIVE NAMES: Is the name clear and complete?
The function name says what it does -- a reader should get "pretty much what you expected" (Ward's principle)
Long descriptive names are preferred over short enigmatic ones
Long descriptive names are preferred over long descriptive comments
Naming convention supports multiple words that are easy to read (camelCase, snake_case, etc.)
Names are consistent across the module -- same phrases, nouns, and verbs throughout (e.g., includeSetupAndTeardownPages, includeSetupPages, includeSuiteSetupPage, includeSetupPage -- consistent verb/noun phrasing)
Spend time choosing the name; try several; read the code with each in place before committing
Choosing descriptive names will clarify the design of the module in your mind and help you improve it -- hunting for a good name often results in a favorable restructuring of the code
Changing a name to improve clarity is always worth the effort -- modern IDEs (Eclipse, IntelliJ) make this trivial
Consistency test: if you have includeTeardownPages, includeSuiteTeardownPage, and includeTeardownPage, readers see the pattern and ask "What happened to includeSetupPages, includeSuiteSetupPage, and includeSetupPage?" -- that's "pretty much what you expected"
6. FUNCTION ARGUMENTS: Are there as few arguments as possible?
Argument count rules (strict):
Niladic (0 args): Ideal. Prefer this form.
Monadic (1 arg): Good. Acceptable and common.
Dyadic (2 args): Acceptable with caution. Harder to understand than monadic.
Triadic (3 args): Avoid. Requires very special justification.
Polyadic (4+ args): Never. Refactor immediately.
Why arguments are costly: Arguments take conceptual power. They force readers to interpret details at a different level of abstraction than the function name. includeSetupPage() is easier to understand than includeSetupPageInto(newPageContent) -- the StringBuffer argument forces you to know a detail that isn't important at that point. Arguments are also harder from a testing perspective: with zero arguments, testing is trivial; with one, it's manageable; with two or more, testing every combination of appropriate values can be daunting.
6a. Common Monadic Forms (1 argument)
Three legitimate patterns for single-argument functions:
Asking a question:boolean fileExists("MyFile") -- input arg, boolean return
Transforming:InputStream fileOpen("MyFile") -- input arg, transformed return value. The transformation should appear as the return value, NOT as an output argument. StringBuffer transform(StringBuffer in) is better than void transform(StringBuffer out) even if the implementation simply returns the input.
Event:void passwordAttemptFailedNtimes(int attempts) -- input arg, no return, alters system state. Use carefully; make it clear from name/context that it is an event.
Avoid monadic functions that do not follow these forms, e.g., void includeSetupPageInto(StringBuffer pageText) -- using an output argument for a transformation is confusing.
6b. Flag Arguments
No boolean arguments -- flag args loudly proclaim the function does more than one thing (one thing when true, another when false)
Split into two functions instead: renderForSuite() and renderForSingleTest() rather than render(boolean isSuite)
render(true) is confusing to any reader -- render(boolean isSuite) is only marginally better
Note: Even in Listing 3-7 Martin admits the flag argument was a compromise because callers were already passing the boolean and he wanted to limit the scope of refactoring. The ideal would have been renderForSuite() and renderForSingleTest()
6c. Dyadic Functions (2 arguments)
Dyads are acceptable when arguments have natural ordering or cohesion: Point(0, 0), assertEquals(expected, actual)
Even natural dyads like assertEquals(expected, actual) are problematic -- readers frequently swap the order
Convert to monads when possible: make one arg a member variable, make the function a method on one arg's class, or extract a new class (e.g., FieldWriter taking outputStream in constructor, then calling write(name))
writeField(outputStream, name) is harder to understand than outputStream.writeField(name)
6d. Triads (3 arguments)
Think very carefully before creating a triad
assertEquals(message, expected, actual) -- readers consistently pause, read message as expected, and must learn to ignore the first argument
assertEquals(1.0, amount, .001) is more tolerable because the floating-point comparison delta is a well-known convention
6e. Argument Objects
When a function needs 2-3 arguments, wrap related args into a class/struct
makeCircle(double x, double y, double radius) becomes makeCircle(Point center, double radius)
This is NOT cheating -- when variables are passed together, they likely represent a concept (like "point") that deserves its own name
6f. Argument Lists (Varargs)
Variable argument lists that are all treated identically count as a single argument of type List
String.format("%s worked %.2f hours.", name, hours) is effectively dyadic: format(String, Object...)
Same niladic/monadic/dyadic/triadic rules apply after collapsing the varargs:
void monad(Integer... args);
void dyad(String name, Integer... args);
void triad(String name, int count, Integer... args);
6g. Verbs and Keywords
Monad: function and argument form a verb/noun pair -- write(name), writeField(name)
Use the keyword form to encode argument names in the function name: assertExpectedEqualsActual(expected, actual) instead of assertEquals(expected, actual) -- eliminates ordering confusion
7. SIDE EFFECTS: Does the function do only what its name promises?
"Side effects are lies." Your function promises to do one thing, but it also does other hidden things.
No hidden changes to class member variables beyond what the name implies
No hidden changes to function parameters
No hidden changes to system globals
No temporal couplings hidden inside the function
Bad -- hidden side effect (Listing 3-6, UserValidator.java):
publicclassUserValidator {
private Cryptographer cryptographer;
publicbooleancheckPassword(String userName, String password) {
Useruser= UserGateway.findByName(userName);
if (user != User.NULL) {
StringcodedPhrase= user.getPhraseEncodedByPassword();
Stringphrase= cryptographer.decrypt(codedPhrase, password);
if ("Valid Password".equals(phrase)) {
Session.initialize(); // SIDE EFFECT! Name says "check" not "initialize"returntrue;
}
}
returnfalse;
}
}
The call to Session.initialize() is a side effect. The name checkPassword does not imply session initialization. A caller who believes what the name of the function says runs the risk of erasing the existing session data when he or she decides to check the validity of the user. This side effect creates a temporal coupling: checkPassword can only be called at certain times (when it is safe to initialize the session). If called out of order, session data may be inadvertently lost. If the temporal coupling is truly needed, rename to checkPasswordAndInitializeSession (though this violates "do one thing").
7a. Output Arguments
Avoid output arguments entirely -- arguments are naturally interpreted as inputs
appendFooter(s) is confusing -- does it append s as the footer, or append a footer to s? Having to look at the signature (void appendFooter(StringBuffer report)) to understand is a cognitive break
In OO languages, use this/self instead: report.appendFooter() rather than appendFooter(report)
If a function must change state, have it change the state of its owning object
8. COMMAND QUERY SEPARATION: Does the function either do something OR answer something, never both?
Functions that change state (commands) should return void
Functions that return information (queries) should not change state
Never combine both -- it creates ambiguity
Bad:
publicbooleanset(String attribute, String value);
// Usage becomes ambiguous:if (set("username", "unclebob"))...
// Is this asking "was username previously set to unclebob"?// Or "did setting username to unclebob succeed"?
The author intended set to be a verb, but in the if statement context it feels like an adjective -- "If the username attribute was previously set to unclebob." One could try renaming to setAndCheckIfExists, but that doesn't much help readability. The real solution is to separate command from query so ambiguity cannot occur.
Good -- separate command and query:
if (attributeExists("username")) {
setAttribute("username", "unclebob");
...
}
9. ERROR HANDLING: Exceptions over error codes, properly separated?
9a. Prefer Exceptions to Returning Error Codes
Do NOT return error codes -- they force callers into deeply nested if structures and violate command-query separation
Error codes promote commands being used as expressions in if predicates: if (deletePage(page) == E_OK) -- this is a subtle CQS violation
Bad -- error codes produce nesting:
if (deletePage(page) == E_OK) {
if (registry.deleteReference(page.name) == E_OK) {
if (configKeys.deleteKey(page.name.makeKey()) == E_OK) {
logger.log("page deleted");
} else {
logger.log("configKey not deleted");
}
} else {
logger.log("deleteReference from registry failed");
}
} else {
logger.log("delete failed");
return E_ERROR;
}
Good -- exceptions separate happy path from error handling:
The delete function is all about error processing. deletePageAndAllReferences is all about the processes of fully deleting a page. Error handling can be ignored. Clean separation.
9c. Error Handling Is One Thing
A function that handles errors should do nothing else
If try exists in a function, it should be the first word in the function
There should be nothing after the catch/finally blocks
9d. Error Enum / Dependency Magnet
Do NOT use error code enums/classes -- they are dependency magnets
Error code classes force recompilation and redeployment of all importing classes when they change, creating pressure to reuse old codes rather than add new ones
Many other classes must import and use this enum. When it changes, all those classes must be recompiled and redeployed. Programmers resist adding new error codes because of the rebuild/redeploy cost, so they reuse old ones instead.
Use exceptions (which are derivatives of the exception class) instead -- new exceptions can be added without forcing recompilation of existing code (Open/Closed Principle)
10. DON'T REPEAT YOURSELF (DRY)
No duplicated algorithm appearing in multiple places (even if not uniformly duplicated or intermixed with other code)
Duplication is identified and extracted into a single function
Four-fold duplication means four places to maintain and four opportunities for errors of omission
Book reference: In Listing 3-1, the same algorithm for finding and including pages was repeated four times for SetUp, SuiteSetUp, TearDown, and SuiteTearDown. In the refactored Listing 3-7, this was remedied by the single include method, dramatically improving readability.
Duplication may be the root of all evil in software. Structured programming, Aspect Oriented Programming, Component Oriented Programming, Codd's database normal forms, and OOP itself are all, in part, strategies for eliminating duplication.
11. STRUCTURED PROGRAMMING (Dijkstra's Rules)
In small functions: multiple return, break, and continue statements are acceptable and can be more expressive than the single-entry, single-exit rule
In large functions (which should not exist): one entry, one exit -- single return, no break/continue in loops, never goto
goto should never be used (only makes sense in large functions, which should not exist)
Violations to Detect
When reviewing code, scan for these specific anti-patterns. Each is keyed to a checklist section above.
#
Violation
Detection Signal
Severity
V1
Long function
>20 lines of code
HIGH
V2
Deep nesting
Indent level >2
HIGH
V3
Multiple responsibilities
Function can be described with "and" or divided into sections
HIGH
V4
Mixed abstraction levels
High-level calls next to low-level string/byte manipulation in same function
HIGH
V5
Exposed switch/if-else chain
Switch on type not in a factory, or repeated in multiple functions
HIGH
V6
Vague or misleading name
Name does not describe what function actually does; requires reading body to understand
MEDIUM
V7
Inconsistent naming
Related functions use different verb/noun conventions (e.g., fetch in one, get in another, retrieve in a third for the same concept)
MEDIUM
V8
Too many arguments
3+ args (triadic or worse)
HIGH
V9
Flag argument
Boolean parameter that controls branching behavior
HIGH
V10
Output argument
Argument used to return/modify data instead of using return value or owning object
MEDIUM
V11
Hidden side effect
Function does something not described by its name (e.g., checkPassword initializes session)
CRITICAL
V12
Temporal coupling
Function can only be called at certain times due to hidden side effects, but this is not reflected in the name
HIGH
V13
Command-query mix
Function both changes state AND returns a value describing state
HIGH
V14
Error code return
Returning error enum/int instead of throwing exception
MEDIUM
V15
Unextracted try/catch
try/catch mixed with business logic in the same function; code after catch block
MEDIUM
V16
Error code enum
Centralized error code class/enum acting as dependency magnet
MEDIUM
V17
Duplicated logic
Same algorithm/structure appears in 2+ places (even non-uniformly intermixed)
HIGH
V18
Long blocks in control structures
Body of if/else/while/for is more than one line (should be a function call)
MEDIUM
V19
Sections within function
Function body can be divided into labeled sections (declarations, init, work, cleanup)
MEDIUM
V20
Non-stepdown reading order
Called functions defined before calling functions; reader cannot read top-down
LOW
How to Fix
Fix for V1 (Long Function)
Extract smaller functions. Each extracted function should do one thing and have a descriptive name. The refactoring of testableHtml (Listing 3-1, ~70 lines) into renderPageWithSetupsAndTeardowns (Listing 3-2, ~12 lines) and finally (Listing 3-3, ~5 lines) demonstrates the progression.
Fix for V2 (Deep Nesting)
Extract the nested block into a named function. Blocks within if/else/while should be one line long -- a function call. This simultaneously reduces nesting and documents intent through the function name.
Fix for V3 (Multiple Responsibilities)
Apply the TO paragraph test. Write: "TO [functionName], we [description]." If the description contains "and" or multiple steps at different abstraction levels, split into multiple functions. Each extracted function should pass the TO test independently.
Fix for V4 (Mixed Abstraction Levels)
Group statements by abstraction level. Extract low-level details into helper functions with descriptive names. The result should read like a top-down narrative where each function introduces the next (The Stepdown Rule).
// Each function at one consistent abstraction levelprivatevoidincludeSetupPages()throws Exception {
if (isSuite)
includeSuiteSetupPage();
includeSetupPage();
}
privatevoidincludeSuiteSetupPage()throws Exception {
include(SuiteResponder.SUITE_SETUP_NAME, "-setup");
}
privatevoidinclude(String pageName, String arg)throws Exception {
WikiPageinheritedPage= findInheritedPage(pageName);
if (inheritedPage != null) {
StringpagePathName= getPathNameForPage(inheritedPage);
buildIncludeDirective(pagePathName, arg);
}
}
Fix for V5 (Exposed Switch Statement)
Create an abstract class or interface defining the polymorphic methods
Create an Abstract Factory with the switch statement buried inside
The factory returns concrete subclass instances
All client code calls methods on the abstract type -- the switch is never seen again
Tolerate switch only when: it appears once, creates polymorphic objects, and is hidden behind an inheritance relationship
Fix for V6-V7 (Bad/Inconsistent Names)
Replace vague names with descriptive ones: testableHtml becomes SetupTeardownIncluder.render
Use consistent verb/noun families: if you have includeSetupAndTeardownPages, name siblings includeSetupPages, includeSuiteSetupPage, includeSetupPage (all use "include" + descriptive noun)
Try several names; read the code with each; pick the one where every routine is "pretty much what you expected"
A long descriptive name is BETTER than a short enigmatic name
A long descriptive name is BETTER than a long descriptive comment
Fix for V8 (Too Many Arguments)
Wrap related args into an object:makeCircle(double x, double y, double radius) becomes makeCircle(Point center, double radius)
Make args instance variables: If an arg is passed to every function in a class, make it a field (as in SetupTeardownIncluder where pageData, isSuite, testPage, newPageContent, pageCrawler are all fields)
Extract a class: Create a new class that takes some args in its constructor and provides a method for the operation
Use the keyword form for unavoidable multi-arg functions: assertExpectedEqualsActual(expected, actual)
Fix for V9 (Flag Argument)
Split the function into two with descriptive names:
render(boolean isSuite) becomes renderForSuite() and renderForSingleTest()
Each function does exactly one thing without branching on a flag
Fix for V10 (Output Argument)
Use return for transformations: StringBuffer transform(StringBuffer in) not void transform(StringBuffer out)
In OO languages, call the method on the owning object: report.appendFooter() not appendFooter(report)
If a function must change state, change the state of its owning object
Fix for V11-V12 (Hidden Side Effects / Temporal Coupling)
Best: Remove the side effect entirely -- extract it into its own function
If coupling is necessary: Rename the function to include both responsibilities: checkPasswordAndInitializeSession -- at least the caller knows what will happen (though this violates "do one thing")
Never hide state changes behind a name that does not describe them
Extract try/catch into its own function where try is the first word and nothing follows catch/finally
Create the happy-path logic in a separate function called from within try
Replace error code enums with exception class hierarchies -- new exceptions can be added without recompilation of dependents (Open/Closed Principle)
Fix for V17 (Duplicated Logic)
Extract the duplicated algorithm into a single function parameterized for the variations. In Listing 3-7, the four instances of the include-page algorithm (SetUp, SuiteSetUp, TearDown, SuiteTearDown) were collapsed into a single include(String pageName, String arg) method.
Fix for V18-V19 (Long Blocks / Sections in Functions)
Extract blocks into named functions. If a function has identifiable sections (declarations, initializations, sieve), it is doing more than one thing. Each section should become its own function.
Fix for V20 (Non-Stepdown Order)
Reorder functions so that callers appear above callees. The reader should be able to read the file top-to-bottom, encountering each function just after the function that calls it, descending one abstraction level at a time.
The Writing Process
Functions do NOT emerge perfectly on the first draft. Martin's own process:
First draft: Long functions with lots of indenting and nested loops, long argument lists, arbitrary names, duplicated code
Unit tests: A suite of tests covering every line of the clumsy first draft
Refine: Massage and refine the code -- split functions, change names, eliminate duplication, shrink methods, reorder. Sometimes break out whole classes.
Keep tests passing throughout the entire process
End result: Functions that follow all the rules in this chapter
"I don't write them that way to start. I don't think anyone could."
Conclusion: Functions as Language Design
Every system is built from a domain-specific language designed by its programmers. Functions are the verbs of that language; classes are the nouns. Master programmers think of systems as stories to be told rather than programs to be written. They use their programming language to construct a richer, more expressive domain-specific language that tells that story. The hierarchy of functions describes all the actions that take place within the system -- and in an artful act of recursion, those actions are written to use the very domain-specific language they define to tell their own small part of the story.
Your real goal is never just to make functions follow rules. It is to make functions tell the story of the system -- to fit cleanly together into a clear and precise language.
The Gold Standard: SetupTeardownIncluder (Listing 3-7)
The complete refactored class (Listing 3-7) demonstrates every principle simultaneously:
This class demonstrates every principle simultaneously:
Small functions: Every method is 1-4 lines (longest is include at 5 lines of logic)
One thing each:render() renders, isTestPage() checks test status, includeSetupAndTeardownPages() orchestrates includes, include() does a single include
One abstraction level:includeSetupAndTeardownPages calls includeSetupPages, includePageContent, includeTeardownPages, updatePageContent -- all at the same level
Descriptive names: Every function name says exactly what it does
Few arguments: Most are niladic (zero args); include takes 2 (pageName, arg); the private helper buildIncludeDirective takes 2
No side effects: Each function does only what its name says
DRY: The single include method replaces the four-fold duplication in the original
Instance variables over arguments:pageData, isSuite, testPage, newPageContent, pageCrawler are fields, not passed as arguments everywhere
Self-Improvement Protocol
After each review using this skill:
Track violation frequency: Note which violations appear most often in the codebase. Prioritize fixing the most common ones.
Before/after comparison: For each refactored function, verify the refactored version passes every checklist item that the original failed.
Test coverage gate: Never refactor without a passing test suite. Run tests after every change.
Name quality audit: After refactoring, re-read function names top-to-bottom. Does the code read like a top-down narrative? Can you understand the module without reading function bodies?
Argument count trend: Track the distribution of niladic/monadic/dyadic/triadic functions across the codebase over time. The ratio should shift toward fewer arguments.
Duplication scan: After fixing one instance of duplication, search the entire codebase for the same pattern. Duplication often appears in multiple files.
Incremental application: When encountering a massive legacy function, do not attempt a full rewrite. Apply one rule at a time (extract one sub-function, rename one thing, remove one flag argument) and verify tests pass after each step.
Review feedback loop: When a reviewer flags a function issue, map it to a specific violation number (V1-V20) and the corresponding fix. This builds a shared vocabulary for the team.
