code-smells

# Code Smells Detection Comprehensive catalog of code smells for detecting anti-patterns and identifying refactoring opportunities. Based on the [Refactoring.Guru Code Smells Catalog](https://refactoring.guru/refactoring/smells). <IMPORTANT> This skill is a **reference catalog** for code smell detection. It is used by: - `pr-review` Agent 3 (Code Smells Agent) during PR review - `planning-refactor` when identifying improvement opportunities - Standalone code quality analysis When detecting smells, report findings in structured format with: - File:Line location - Smell name and category - Severity score - Suggested refactoring </IMPORTANT> ## When to Use | Context | Trigger | |---------|---------| | **PR Review** | Invoked by `pr-review` Code Smells Agent | | **Refactoring Planning** | Before `planning-refactor` to identify targets | | **Code Quality Audit** | User asks to analyze code quality or find anti-patterns | | **Technical Debt Assessment** | Identifying areas needing improvement | ## Severity Levels | Severity | Score | Impact | Examples | |----------|-------|--------|----------| | **Critical** | 100 | Causes bugs, crashes, security issues | Feature Envy causing null dereference | | **Must Fix** | 80 | Significantly harms maintainability | God Class, Shotgun Surgery | | **Should Fix** | 50 | Reduces code quality | Long Method, Duplicate Code | | **Nitpick** | 20 | Minor improvement opportunity | Lazy Class, Comments | ## Output Format When reporting code smells, use this structured format: ```markdown | File:Line | Smell | Category | Severity | Suggested Refactoring | |-----------|-------|----------|----------|----------------------| | handler.cpp:120-195 | Long Method (75 lines) | Bloater | Should Fix (50) | Extract Method: split into validateRequest(), processData(), buildResponse() | | config.cpp:45 | Magic Number | Bloater | Should Fix (50) | Replace Magic Number: `const int MAX_RETRIES = 42;` | ``` --- ## Category 1: Bloaters Code that has grown to excessive proportions, making it hard to work with. ### Long Method **Threshold:** >10 lines warrants questions, >50 lines is definite smell **Signs:** - Method contains too many lines of code - You need to scroll to see the whole method - Method does multiple things **Why it's bad:** - Hard to understand and maintain - Hides duplicate code - Makes testing difficult - Long methods accumulate like "Hotel California" - easy to add, hard to remove **Detection criteria:** ``` - Lines > 50: Should Fix (50) - Lines > 100: Must Fix (80) - Method does more than one thing (multiple responsibilities) - Requires extensive comments to explain ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Extract Method | Split into smaller methods with descriptive names | | Replace Temp with Query | Local variables interfering with extraction | | Introduce Parameter Object | Many parameters passed between methods | | Decompose Conditional | Complex if/else chains | | Replace Method with Method Object | When extraction is too complex | --- ### Large Class **Threshold:** Too many fields, methods, or lines (>500 lines warrants review) **Signs:** - Class contains excessive fields/methods/lines - Class has multiple unrelated responsibilities - You struggle to summarize what the class does in one sentence **Why it's bad:** - Cognitive overload (too many attributes to track) - Code duplication within the class - Violates Single Responsibility Principle **Detection criteria:** ``` - Lines > 500: Should Fix (50) - Lines > 1000: Must Fix (80) - >10 public methods: Review for SRP violation - >15 fields: Likely doing too much - Multiple unrelated method groups ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Extract Class | Separate distinct behavioral components | | Extract Subclass | Behavior used only in some cases | | Extract Interface | Define contract for operations | | Duplicate Observed Data | GUI classes with domain logic | --- ### Primitive Obsession **Signs:** - Using primitives instead of small objects (money, phone numbers, ranges) - Constants encoding information (`USER_ADMIN_ROLE = 1`) - String constants as field names in arrays - Using `int` for IDs instead of typed wrapper **Why it's bad:** - No type safety (passing wrong int to function) - Related operations scattered across codebase - No place to add validation or behavior - Duplicate code for handling the primitive **Detection criteria:** ``` - Multiple functions operating on same primitive group: Should Fix (50) - Constants simulating types: Should Fix (50) - No domain objects for business concepts (money, date ranges, coordinates): Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Replace Data Value with Object | Group primitives into dedicated classes | | Introduce Parameter Object | Primitives in method parameters | | Preserve Whole Object | Passing extracted values instead of object | | Replace Type Code with Class/Subclasses/State | Constants simulating types | | Replace Array with Object | Using arrays with string keys | --- ### Long Parameter List **Threshold:** >3-4 parameters **Signs:** - Method has more than 3-4 parameters - Parameters are passed through multiple method calls - Hard to remember parameter order - Boolean parameters for mode switching **Why it's bad:** - Difficult to understand method signature - Easy to pass wrong arguments - Often indicates the method is doing too much - Creates tight coupling **Detection criteria:** ``` - 4-5 parameters: Should Fix (50) - 6+ parameters: Must Fix (80) - Boolean "flag" parameters: Should Fix (50) - Parameters from same object: Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Replace Parameter with Method Call | Parameter is result of another call | | Preserve Whole Object | Passing multiple fields from same object | | Introduce Parameter Object | Group related parameters | **When to ignore:** Creating dependencies between classes may be worse than long parameter list. --- ### Data Clumps **Signs:** - Same group of variables appears in multiple places - Parameters that always travel together - Removing one variable makes others meaningless **Why it's bad:** - Code duplication - No single place for operations on the data - Easy to pass incomplete groups **Detection criteria:** ``` - Same 3+ parameters in multiple method signatures: Should Fix (50) - Same fields grouped in multiple classes: Should Fix (50) - Database connection parameters passed separately: Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Extract Class | For class fields | | Introduce Parameter Object | For method parameters | | Preserve Whole Object | Pass object instead of extracting values | --- ## Category 2: Object-Orientation Abusers Incomplete or incorrect application of object-oriented programming principles. ### Switch Statements **Signs:** - Complex `switch` operator or `if-else` chain based on type - Same switch logic scattered across multiple methods - Adding a new case requires changes in many places **Why it's bad:** - Violates Open/Closed Principle - Duplicate switch logic across codebase - Easy to forget updating all switches - Rare use of `switch` is hallmark of good OOP **Detection criteria:** ``` - Switch on type/enum with behavior: Should Fix (50) - Same switch in multiple places: Must Fix (80) - Switch with >5 cases with different behavior: Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Replace Conditional with Polymorphism | When switch determines behavior | | Replace Type Code with Subclasses | Type codes with distinct behavior | | Replace Type Code with State/Strategy | Behavior varies at runtime | | Replace Parameter with Explicit Methods | Simple value-based switches | | Introduce Null Object | Null checks in conditionals | **When to ignore:** Factory patterns legitimately use switch to create objects. --- ### Temporary Field **Signs:** - Fields only used in certain circumstances - Fields remain empty/null most of the time - Field existence confuses readers **Why it's bad:** - Developers expect fields to always be meaningful - Requires null checks scattered through code - Hides actual dependencies **Detection criteria:** ``` - Field used in <25% of methods: Should Fix (50) - Field only set in one method and used in another: Should Fix (50) - Fields for algorithm-specific data: Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Extract Class | Move field and related code to separate class | | Replace Method with Method Object | Algorithm needs many temp fields | | Introduce Null Object | Replace null checks with null object pattern | --- ### Refused Bequest **Signs:** - Subclass uses only some parent methods/properties - Inherited methods throw exceptions or do nothing - Subclass doesn't follow Liskov Substitution Principle **Why it's bad:** - Confusing hierarchy (Dog inherits from Chair?) - Breaking substitutability - Indicates wrong abstraction **Detection criteria:** ``` - Override methods with empty body or exception: Should Fix (50) - Subclass ignores >50% of parent interface: Should Fix (50) - Inheritance for code reuse, not "is-a": Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Replace Inheritance with Delegation | Wrong hierarchy, use composition | | Extract Superclass | Create proper abstraction for shared behavior | --- ### Alternative Classes with Different Interfaces **Signs:** - Two classes do the same thing with different method names - Parallel implementations that could be unified - Duplicate functionality discovered during review **Why it's bad:** - Code duplication across classes - Confusion about which class to use - Maintenance burden **Detection criteria:** ``` - Classes with same purpose, different interface: Should Fix (50) - Duplicate logic in differently-named methods: Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Rename Method | Align method names to common interface | | Move Method | Consolidate implementations | | Extract Superclass | Share common behavior | | Delete redundant class | After consolidation | **When to ignore:** Classes in different libraries with independent versioning. --- ## Category 3: Change Preventers Issues that require changes in multiple places when a single change is needed. ### Divergent Change **Signs:** - Single class needs changes for multiple unrelated reasons - Adding new feature requires changing many methods in one class - Class has multiple "areas" of responsibility **Why it's bad:** - Single class handles too many concerns - Any change risks breaking unrelated functionality - Violates Single Responsibility Principle **Detection criteria:** ``` - Class changes for >2 unrelated reasons: Should Fix (50) - Class has distinct "sections" of functionality: Should Fix (50) - Methods naturally group into separate concerns: Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Extract Class | Separate each concern into its own class | | Extract Superclass/Subclass | When classes share some behavior | --- ### Shotgun Surgery **Signs:** - Single change requires edits to many different classes - Adding a feature means touching 5+ files - Opposite of Divergent Change **Why it's bad:** - Easy to miss a required change - High chance of introducing bugs - Expensive maintenance **Detection criteria:** ``` - Feature addition touches 5+ classes: Must Fix (80) - Single responsibility scattered across classes: Must Fix (80) - "Overzealous" previous separation causing this: Must Fix (80) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Move Method/Field | Consolidate scattered responsibility | | Inline Class | After consolidation, remove empty shells | --- ### Parallel Inheritance Hierarchies **Signs:** - Creating subclass in one hierarchy requires subclass in another - Class prefixes match across hierarchies - Mirrored class structures **Why it's bad:** - Duplicate maintenance - Growing complexity as hierarchies expand - Easy to forget creating matching class **Detection criteria:** ``` - Two hierarchies with matching structures: Should Fix (50) - Adding to one requires adding to another: Should Fix (50) - Prefixes like "XHandler/XFactory": Should Fix (50) ``` **Refactoring:** | Technique | When to Use | |-----------|-------------| | Move Method/Field | Make one hierarchy reference the other | | Eliminate redundant hierarchy | After consolidation | **When to ignore:** If deduplication makes code uglier, keep the parallel structure. --- ## Category 4: Dispensables Pointless and unneeded elements whose absence would make code cleaner.