Refactoring

Reference: See reference.md for complete decision tree and all patterns. Examples: See examples.md for real-world refactoring case studies.

IMPORTANT: This skill operates autonomously - no user confirmation needed.

POTENTIAL TYPES TO DISCOVER:

1. Data being parsed from strings → Parse* types Example: ParseCommandResult(), ParseLogEntry()

2. Scattered validation logic → Validated types Example: Email, Port, IPAddress types

3. Data that always travels together → Aggregate types Example: UserCredentials, ServerConfig

4. Complex conditions → State/status types Example: DeploymentStatus with IsReady(), CanProceed()

5. Repeated string manipulation → Types with methods Example: FilePath with Dir(), Base(), Ext()

Architectural Issues:

• noglobals/gochecknoglobals: Global variable usage → Dependency rejection pattern
• gochecknoinits: Init function usage → Extract initialization logic
• Static/singleton patterns: Hidden dependencies → Inject dependencies

Design Smells:

• dupl: Code duplication → Extract common logic/types
• goconst: Magic strings/numbers → Extract constants or types
• ineffassign: Ineffective assignments → Simplify logic

For Architectural Failures (noglobals, singletons):

1. Dependency Rejection → Incremental bottom-up approach
2. Extract Type with dependency injection
3. Push global access up call chain one level
4. Iterate until globals only at entry points (main, handlers)

For Design Smells (dupl, goconst):

1. Extract Type → For repeated values or validation
2. Extract Function → For code duplication
3. Extract Constant → For magic strings/numbers

// AFTER - Story-like func ProcessOrder(order Order) error { if err := validateOrder(order); err != nil { return err } if err := saveToDatabase(order); err != nil { return err } return notifyCustomer(order) }

func validateOrder(order Order) error { /* ... / } func saveToDatabase(order Order) error { / ... / } func notifyCustomer(order Order) error { / ... */ }

</pattern>

<pattern name="extract_type" signal="Primitive obsession or unstructured data">
<juiciness_test version="2">
**BEHAVIORAL JUICINESS** (rich behavior):
- Complex validation rules (regex, ranges, business rules)
- Multiple meaningful methods (≥2 methods)
- State transitions or transformations
- Format conversions (different representations)

**STRUCTURAL JUICINESS** (organizing complexity):
- Parsing unstructured data into fields
- Grouping related data that travels together
- Making implicit structure explicit
- Replacing map[string]interface{} with typed fields

**USAGE JUICINESS** (simplifies code):
- Used in multiple places
- Significantly simplifies calling code
- Makes tests cleaner and more focused

**Score**: Need "yes" in at least ONE category to create the type
</juiciness_test>

```go
// NOT JUICY - Don't create type
func ValidateUserID(id string) error {
    if id == "" {
        return errors.New("empty id")
    }
    return nil
}
// Just use: if userID == ""

// JUICY (Behavioral) - Complex validation
type Email string

func ParseEmail(s string) (Email, error) {
    if s == "" {
        return "", errors.New("empty email")
    }
    if !emailRegex.MatchString(s) {
        return "", errors.New("invalid format")
    }
    if len(s) > 255 {
        return "", errors.New("too long")
    }
    return Email(s), nil
}

func (e Email) Domain() string { /* extract domain */ }
func (e Email) LocalPart() string { /* extract local */ }

// JUICY (Structural) - Parsing complex data
type CommandResult struct {
    FailedFiles  []string
    SuccessFiles []string
    Message      string
    ExitCode     int
}

func ParseCommandResult(output string) (CommandResult, error) {
    // Parse unstructured output into structured fields
}

Warning Signs of Over-Engineering:

• Type with only one trivial method
• Simple validation (just empty check)
• Type that's just a wrapper without behavior
• Good variable naming would be clearer

See Example 2 for complete case study.

// AFTER - Extracted functions func CreateUser(data map[string]interface{}) error { user, err := validateAndParseUser(data) if err != nil { return err } if err := saveUser(user); err != nil { return err } if err := sendWelcomeEmail(user); err != nil { return err } logUserCreation(user) return nil }

</pattern>

<pattern name="early_returns" signal="Deep nesting > 2 levels">
```go
// BEFORE - Deeply nested
func ProcessItem(item Item) error {
    if item.IsValid() {
        if item.IsReady() {
            if item.HasPermission() {
                // Process
                return nil
            } else {
                return errors.New("no permission")
            }
        } else {
            return errors.New("not ready")
        }
    } else {
        return errors.New("invalid")
    }
}

// AFTER - Early returns
func ProcessItem(item Item) error {
    if !item.IsValid() {
        return errors.New("invalid")
    }
    if !item.IsReady() {
        return errors.New("not ready")
    }
    if !item.HasPermission() {
        return errors.New("no permission")
    }
    // Process
    return nil
}

// AFTER - Extracted handlers func HandleRequest(reqType string, data interface{}) error { switch reqType { case "create": return handleCreate(data) case "update": return handleUpdate(data) case "delete": return handleDelete(data) default: return errors.New("unknown type") } }

func handleCreate(data interface{}) error { /* ... / } func handleUpdate(data interface{}) error { / ... / } func handleDelete(data interface{}) error { / ... */ }

</pattern>

<pattern name="dependency_rejection" signal="noglobals linter fails or global/singleton usage">
**Goal**: Create "islands of clean code" by incrementally pushing dependencies up the call chain

**Strategy**: Work from bottom-up, rejecting dependencies one level at a time
- DON'T do massive refactoring all at once
- Start at deepest level (furthest from main)
- Extract clean type with dependency injected
- Push global access up one level
- Repeat until global only at entry points

```go
// BEFORE - Global accessed deep in code
func PublishEvent(event Event) error {
    conn, err := nats.Connect(env.Configs.NATsAddress)
    // ... complex logic
}

// AFTER - Dependency rejected up one level
type EventPublisher struct {
    natsAddress string  // injected, not global
}

func NewEventPublisher(natsAddress string) *EventPublisher {
    return &EventPublisher{natsAddress: natsAddress}
}

func (p *EventPublisher) Publish(event Event) error {
    conn, err := nats.Connect(p.natsAddress)
    // ... same logic, now testable
}

// Caller pushed up (closer to main)
func SetupMessaging() *EventPublisher {
    return NewEventPublisher(env.Configs.NATsAddress)  // Global only here
}

Result: EventPublisher is now 100% testable without globals

Key Principles:

• Incremental: One type at a time, one level at a time
• Bottom-up: Start at deepest code, work toward main
• Pragmatic: Accept globals at entry points (main, handlers)
• Testability: Each extracted type is an island (testable in isolation)

See Example 3 for complete case study.

1. Does this read like a story?

   • No → Extract functions for different abstraction levels

2. Can this be broken into smaller pieces?

   • By responsibility? → Extract functions
   • By task? → Extract functions
   • By category? → Extract functions

3. Does logic run on primitives?

   • Yes → Is this primitive obsession? → Extract type

4. Is function long due to switch statement?

   • Yes → Extract case handlers

5. Are there deeply nested if/else?

   • Yes → Use early returns or extract functions

ALWAYS invoke @testing skill to write tests for:

• Isolated types: Types with injected dependencies (testable islands)
• Value object types: Types that may depend on other value objects but are still isolated
• Extracted functions: New functions created during refactoring
• Parse functions: Functions that transform unstructured data

Examples of testable islands:

• NATSClient with injected natsAddress string (no other dependencies)
• Email type with validation logic (no dependencies)
• ServiceEndpoint that uses Port value object (both are testable islands)
• OrderService with injected Repository and EventPublisher (all testable)

Note: Islands can depend on other value objects and still be isolated!

Warning Signs of Over-Engineering:

• Creating types with only one method
• Functions that just call another function
• More abstraction layers than necessary
• Code becomes harder to understand
• Diminishing returns on complexity reduction

Pragmatic Approach: IF linter passes AND code is readable: STOP - Don't extract more EVEN IF you could theoretically extract more: STOP - Avoid abstraction bloat

Function: CreateUser (user/service.go:45) Role: Core user creation orchestration Called by:

• api/handler.go:89 (HTTP endpoint)
• cmd/user.go:34 (CLI command)

Potential types spotted:

• Email: Complex validation logic scattered
• UserID: Generation and validation logic

REFACTORING APPLIED

Patterns Successfully Applied:

1. Early Returns: Reduced nesting from 4 to 2 levels
2. Storifying: Extracted validate(), save(), notify()
3. Extract Type: Created Email and PhoneNumber types

Patterns Tried but Insufficient:

• Extract Function alone: Still too complex, needed types

Types Created (with Juiciness Score):

Email type (JUICY - Behavioral + Usage):

• Behavioral: ParseEmail(), Domain(), LocalPart() methods
• Usage: Used in 5+ places across codebase
• Island: Testable in isolation
• → Invoke @testing skill to write tests

Types Considered but Rejected (NOT JUICY):

• UserID: Only empty check, good naming sufficient
• Status: Just string constants, enum adequate

METRICS

Complexity Reduction:

• Cyclomatic: 18 → 7
• Cognitive: 25 → 8
• LOC: 120 → 45
• Nesting: 4 → 2

FILES MODIFIED

Modified:

• user/service.go (+15, -75 lines)

Created (Islands of Clean Code):

• user/email.go (new, +45 lines) → Ready for @testing skill

AUTOMATION COMPLETE

Stopping Criteria Met:

• Linter passes (0 issues)
• All functions < 50 LOC
• Max nesting = 2 levels
• Code reads like a story
• No more juicy abstractions

Ready for: @pre-commit-review phase

</output_format>

<integration>
**Invoked By (Automatic Triggering)**:
- **@linter-driven-development**: Automatically invokes when linter fails (Phase 3)
- **@pre-commit-review**: Automatically invokes when design issues detected (Phase 4)

**Iterative Loop**:
1. Linter fails → invoke @refactoring
2. Refactoring complete → re-run linter
3. Linter passes → invoke @pre-commit-review
4. Review finds design debt → invoke @refactoring again
5. Repeat until both linter AND review pass

**Invokes (When Needed)**:
- **@code-designing**: When refactoring creates new types, validate design
- **@testing**: Automatically invoked to write tests for new types/functions
- **@pre-commit-review**: Validates design quality after linting passes

See [reference.md](./reference.md) for complete refactoring patterns and decision tree.
</integration>

<success_criteria>
Refactoring is complete when ALL of the following are true:

- [ ] Linter passes (0 issues)
- [ ] All functions < 50 LOC
- [ ] Max nesting ≤ 2 levels
- [ ] Code reads like a story (single abstraction level per function)
- [ ] No more "juicy" abstractions to extract
- [ ] Tests written for new types/functions (via @testing skill)
- [ ] Ready for @pre-commit-review phase
</success_criteria>