refactor

name	refactor
description	Surgical code refactoring to improve maintainability without changing behavior. Covers extracting functions, renaming variables, breaking down god functions, improving type safety, eliminating code smells, and applying design patterns. Less drastic than repo-rebuilder; use for gradual improvements.
license	MIT

Refactor

Overview

Improve code structure and readability without changing external behavior. Refactoring is gradual evolution, not revolution. Use this for improving existing code, not rewriting from scratch.

When to Use

Use this skill when:

• Code is hard to understand or maintain
• Functions/classes are too large
• Code smells need addressing
• Adding features is difficult due to code structure
• User asks "clean up this code", "refactor this", "improve this"

---

Refactoring Principles

The Golden Rules

1. Behavior is preserved - Refactoring doesn't change what the code does, only how
2. Small steps - Make tiny changes, test after each
3. Version control is your friend - Commit before and after each safe state
4. Tests are essential - Without tests, you're not refactoring, you're editing
5. One thing at a time - Don't mix refactoring with feature changes

When NOT to Refactor

- Code that works and won't change again (if it ain't broke...)
- Critical production code without tests (add tests first)
- When you're under a tight deadline
- "Just because" - need a clear purpose

---

🧱 ACADEMYHUB CLEAN ARCHITECTURE ENFORCEMENT (NON-NEGOTIABLE)

These architectural directives are STRICTLY ENFORCED during refactoring. Violations will be rejected immediately.

STRICT CLEAN ARCHITECTURE & DEPENDENCY INVERSION

• ALL services MUST receive DTOs, NEVER domain models directly from handlers
• Services must depend on simple Data Transfer Objects (DTOs) for input parameters
• Domain entities must NEVER be passed directly from HTTP handlers to services
• This enforces proper separation between delivery mechanism and business logic layers

✅ CORRECT PATTERN EXAMPLE

// Service interface receives DTOs
type UserService interface {
    Create(ctx context.Context, req *dto.CreateUserRequest) (*dto.UserResponse, error)
}

// Handler binds HTTP request to Request DTO
func (h *UserHandler) Create(c *gin.Context) {
    var req dto.CreateUserRequest
    if err := c.ShouldBindJSON(&req); err != nil {
        c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request"})
        return
    }
    
    // Pass standard context and DTO to service
    result, err := h.service.Create(c.Request.Context(), &req)
    if err != nil {
        // Handle error
        return
    }
    
    c.JSON(http.StatusCreated, result)
}

❌ INCORRECT PATTERN EXAMPLE

// WRONG: Service receiving domain model directly from handler
type UserService interface {
    CreateStudent(ctx context.Context, student *domain.Student) error  // VIOLATION!
}

// WRONG: Handler passes domain model directly to service  
func (h *StudentHandler) Create(c *gin.Context) {
    var student domain.Student  // VIOLATION: Creating domain model in handler
    if err := c.ShouldBindJSON(&student); err != nil {
        // handle error
    }
    
    // VIOLATION: Passing domain model directly to service
    err := h.service.CreateStudent(c.Request.Context(), &student)
    // ...
}

REPOSITORY INTERFACES & IMPORT CYCLE PREVENTION

• ALL Repository interfaces MUST be declared in the Domain layer (e.g., /domain/repository.go)
• NEVER declare repository interfaces inside delivery, service, or repository implementation folders
• This prevents Golang's fatal import cycle not allowed error when other layers need to mock repositories

✅ CORRECT PATTERN EXAMPLE

// File: internal/domain/student_repository.go
package domain

import "context"

type StudentRepository interface {
    FindByID(ctx context.Context, id uint) (*Student, error)
    Create(ctx context.Context, student *Student) error
}

// File: internal/repository/student_repository.go  
package repository

import (
    "context"
    "gorm.io/gorm"
    "github.com/example-org/academyhub-backend/internal/domain"
)

type studentRepository struct {
    db *gorm.DB
}

func NewStudentRepository(db *gorm.DB) domain.StudentRepository {
    return &studentRepository{db: db}
}

DTO ISOLATION & REUSABILITY

• DTOs MUST be defined in separate dto directory with clear organization
• DTOs must NEVER be defined inline in service or handler files
• Request and Response DTOs must be properly separated and reusable

✅ CORRECT PATTERN EXAMPLE

// File: internal/dto/user.go
package dto

// Request DTO
type CreateUserRequest struct {
    Name  string `json:"name" validate:"required,min=3"`
    Email string `json:"email" validate:"required,email"`
}

// Response DTO  
type UserResponse struct {
    ID    uint   `json:"id"`
    Name  string `json:"name"`
    Email string `json:"email"`
}

SERVICE LAYER PURITY (FRAMEWORK AGNOSTIC)

• Services MUST be completely framework-agnostic
• Services must NEVER receive gin.Context or other framework-specific types
• Services must use only standard Go context.Context as first parameter
• Business logic must be isolated from HTTP/web framework concerns

---

📱 ACADEMYHUB MOBILE FAT SCREEN PREVENTION (MANDATORY)

FILE SIZE LIMITS ARE NON-NEGOTIABLE DURING REFACTORING:

• Screens: MUST be <300 lines of code
• Feature Hooks: MUST be <400 lines of code
• Violations: Files exceeding limits MUST be refactored immediately

STRICT SEPARATION OF CONCERNS

MUST: Screens are DUMB - only contain UI layout, JSX, and visual components
MUST: Feature Hooks are SMART - handle ALL business logic, state management, data transformation, and API orchestrations
MUST: Screens call Feature Hooks → Feature Hooks call TanStack Query → Queries call Services
MUST NOT: Screens call Services directly
MUST NOT: Screens have complex useEffect logic
MUST NOT: Screens have more than two useState hooks
NEVER: Allow Fat Screens with >300 lines of code

✅ CORRECT PATTERN EXAMPLE

// ✅ CORRECT - Screen component (DUMB)
import { StudentListScreen } from '@features/student/screens';
import { useStudentList } from '@features/student/hooks/use-student-list';

export const StudentListScreen = () => {
  const { 
    students, 
    isLoading, 
    error, 
    onSearch, 
    onFilter, 
    onLoadMore 
  } = useStudentList();
  
  if (isLoading) return <LoadingIndicator />;
  if (error) return <ErrorMessage error={error} />;
  
  return (
    <StudentList 
      students={students}
      onSearch={onSearch}
      onFilter={onFilter}
      onLoadMore={onLoadMore}
    />
  );
};

// ✅ CORRECT - Feature Hook (SMART)
import { useStudentListQuery } from '@core/states/queries';
import { useMemo, useState, useCallback } from 'react';

export const useStudentList = () => {
  const [keyword, setKeyword] = useState('');
  const [filter, setFilter] = useState<StudentFilterValue>(defaultStudentFilter);
  const [page, setPage] = useState(1);
  
  // Data fetching with proper query parameters
  const { data: studentListResponse, isFetching: isFetchingStudents } = useStudentListQuery({
    keyword,
    filter,
    page,
    limit: 20
  });
  
  // Business logic for data transformation
  const students = useMemo(() => {
    return studentListResponse?.data || [];
  }, [studentListResponse?.data]);
  
  // Event handlers with proper cleanup
  const onSearch = useCallback((searchTerm: string) => {
    setKeyword(searchTerm);
    setPage(1);
  }, []);
  
  const onFilter = useCallback((newFilter: StudentFilterValue) => {
    setFilter(newFilter);
    setPage(1);
  }, []);
  
  const onLoadMore = useCallback(() => {
    if (hasNextPage) {
      setPage(prev => prev + 1);
    }
  }, [hasNextPage]);
  
  return {
    students,
    isLoading: isFetchingStudents && page === 1,
    error: studentListResponse?.error,
    onSearch,
    onFilter,
    onLoadMore
  };
};

❌ INCORRECT PATTERN EXAMPLE

// ❌ WRONG - Fat Screen with business logic
export const StudentListScreen = () => {
  const [keyword, setKeyword] = useState(''); // ❌ Too many useState hooks
  const [filter, setFilter] = useState<StudentFilterValue>(defaultStudentFilter);
  const [page, setPage] = useState(1);
  
  // ❌ Direct service calls in screen
  const { data: studentListResponse, isFetching: isFetchingStudents } = useStudentListQuery({
    keyword,
    filter,
    page,
    limit: 20
  });
  
  // ❌ Complex useEffect logic in screen
  const handleRefresh = useCallback(async () => {
    setRefreshing(true);
    setPage(1);
    await refetchStudents(); // ❌ Direct query manipulation
    setRefreshing(false);
  }, [refetchStudents]);
  
  // ❌ Business logic for data transformation in screen
  const studentData = useMemo(() => {
    return (studentListResponse?.data || []) as readonly any[];
  }, [studentListResponse?.data]);
  
  // ❌ Complex event handlers with business logic
  const handleEndReached = useCallback(() => {
    if (isLoading || !hasNextPage) return;
    setPage(prev => prev + 1); // ❌ Pagination logic in screen
  }, [isLoading, hasNextPage]);
  
  return (
    // ... JSX with inline logic
  );
};

BUSINESS LOGIC EXTRACTION RULES

• Extract to Feature Hooks: Data fetching, filtering, sorting, pagination, form validation, API orchestration
• Extract to Utility Functions: Date formatting, string manipulation, mathematical calculations, data transformation
• Extract to Custom Hooks: Reusable logic patterns, complex state management, side effects with cleanup
• Keep in Screens: Only UI rendering, simple event propagation, basic conditional rendering

---

⚡ ACADEMYHUB FLASHLIST PERFORMANCE REQUIREMENTS (ZERO TOLERANCE)

NEVER use FlatList or ScrollView.map for lists. ALWAYS use FlashList from @shopify/flash-list.

MUST: Use FlashList for ALL list implementations
MUST: Include estimatedItemSize prop for EVERY FlashList
MUST NOT: Use FlatList under any circumstances
MUST NOT: Use ScrollView.map for lists
NEVER: Render lists without proper virtualization

✅ CORRECT PATTERN EXAMPLE

// ✅ CORRECT - Proper FlashList implementation
import { FlashList } from '@shopify/flash-list';
import { StudentListItem } from './components';

const StudentListScreen = () => {
  const { students, isLoading, onLoadMore } = useStudentList();
  
  if (isLoading) return <LoadingIndicator />;
  
  return (
    <FlashList
      data={students}
      renderItem={renderStudentItem} // Memoized renderItem function
      estimatedItemSize={80} // ✅ MANDATORY - prevents layout thrashing
      onEndReached={onLoadMore}
      keyExtractor={(item) => item.id.toString()}
    />
  );
};

// ✅ CORRECT - Memoized renderItem function
const renderStudentItem = React.memo(({ item }: { item: Student }) => {
  return <StudentListItem student={item} />;
});

// ✅ CORRECT - Memoized list item component
export const StudentListItem = React.memo(({ student }: Props) => {
  const handlePress = useCallback(() => {
    navigateToStudentDetail(student.id);
  }, [student.id]);
  
  return (
    <Surface elevation={1} style={styles.itemSurface}>
      <TouchableRipple borderless onPress={handlePress}>
        <View style={styles.content}>
          <Text>{student.name}</Text>
          <Text variant="bodySmall">{student.className}</Text>
        </View>
      </TouchableRipple>
    </Surface>
  );
});

❌ INCORRECT PATTERN EXAMPLE

// ❌ WRONG - Using FlatList instead of FlashList
import { FlatList } from 'react-native';

const BadStudentList = () => {
  return (
    <FlatList
      data={students}
      renderItem={({ item }) => <StudentListItem student={item} />} // ❌ Non-memoized inline function
      // ❌ Missing estimatedItemSize equivalent
    />
  );
};

MEMOIZATION REQUIREMENTS (MANDATORY)

ALL functions passed as props to child components MUST be wrapped in useCallback to prevent unnecessary re-renders.

ESTIMATED ITEM SIZE (CRITICAL)

The estimatedItemSize prop is MANDATORY for ALL FlashList components. Missing this prop causes severe performance issues and layout thrashing.

---

💰 ACADEMYHUB DATABASE TRANSACTION SAFETY (ACID COMPLIANCE)

For financial and stock operations requiring ACID compliance during refactoring, use proper transaction patterns with error handling.

✅ CORRECT ACID TRANSACTION PATTERN

// Financial transaction example with ACID properties
func (s *PaymentService) ProcessPayment(ctx context.Context, req *dto.ProcessPaymentRequest) error {
    return s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
        // 1. Verify sufficient balance (Read)
        wallet, err := s.walletRepo.FindByUserID(tx, req.UserID)
        if err != nil {
            return fmt.Errorf("failed to find wallet: %w", err)
        }
        
        if wallet.Balance < req.Amount {
            return ErrInsufficientBalance
        }
        
        // 2. Create payment record (Write)
        payment := &domain.Payment{
            UserID:     req.UserID,
            Amount:     req.Amount,
            Status:     "pending",
            CreatedAt:  time.Now(),
        }
        if err := s.paymentRepo.Create(tx, payment); err != nil {
            return fmt.Errorf("failed to create payment: %w", err)
        }
        
        // 3. Update wallet balance (Write)
        wallet.Balance -= req.Amount
        if err := s.walletRepo.UpdateBalance(tx, wallet); err != nil {
            return fmt.Errorf("failed to update wallet: %w", err)
        }
        
        // 4. Mark payment as completed
        payment.Status = "completed"
        if err := s.paymentRepo.Update(tx, payment); err != nil {
            return fmt.Errorf("failed to complete payment: %w", err)
        }
        
        return nil // Commit transaction
    })
}

✅ PESSIMISTIC LOCKING FOR HIGH-CONCURRENCY SCENARIOS

// PostgreSQL pessimistic locking example
func (r *inventoryRepository) ReserveStock(ctx context.Context, productID uint, quantity uint) error {
    return r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
        // Acquire exclusive lock on inventory row
        var inventory domain.Inventory
        if err := tx.Clauses(clause.Locking{
            Strength: "UPDATE",
            Options:  "NOWAIT", // Fail immediately if locked instead of waiting
        }).Where("product_id = ? AND available >= ?", productID, quantity).
            First(&inventory).Error; err != nil {
            if errors.Is(err, gorm.ErrRecordNotFound) {
                return ErrInsufficientStock
            }
            return fmt.Errorf("failed to lock inventory: %w", err)
        }
        
        // Update inventory within same transaction
        inventory.Available -= quantity
        inventory.Reserved += quantity
        
        if err := tx.Save(&inventory).Error; err != nil {
            return fmt.Errorf("failed to update inventory: %w", err)
        }
        
        return nil
    })
}

❌ INCORRECT PATTERN EXAMPLES

// ❌ WRONG - No transaction for financial operations
func (s *PaymentService) ProcessPayment(ctx context.Context, req *dto.ProcessPaymentRequest) error {
    // Multiple separate DB calls without transaction
    wallet, _ := s.walletRepo.FindByUserID(s.db, req.UserID)
    s.paymentRepo.Create(s.db, payment) // Could fail after wallet updated
    s.walletRepo.UpdateBalance(s.db, wallet) // Partial state possible
    return nil
}

// ❌ WRONG - Missing error handling in transactions
func (s *PaymentService) ProcessPayment(ctx context.Context, req *dto.ProcessPaymentRequest) error {
    s.db.Transaction(func(tx *gorm.DB) error {
        // Operations without proper error checking
        s.walletRepo.UpdateBalance(tx, wallet)
        s.paymentRepo.Create(tx, payment)
        return nil // Always succeeds even if operations fail
    })
    return nil
}

REFACTORING GUIDELINES FOR TRANSACTIONS

• Identify Critical Paths: Look for operations involving money, inventory, or user data that must be atomic
• Wrap Related Operations: Group all related reads/writes in a single transaction
• Handle Errors Properly: Return meaningful errors that can be handled by calling code
• Use Appropriate Locking: Apply pessimistic locking for high-concurrency scenarios
• Avoid Long Transactions: Keep transactions short to minimize lock contention

---

🎨 ACADEMYHUB ELEVATED MD3 DESIGN SYSTEM (MANDATORY)

This project follows MD3 elevated design with zero tolerance for inline styles. ALL styling MUST use design tokens from @core/styles.

ZERO TOLERANCE FOR INLINE STYLES

MUST: Use @core/styles tokens for all styling (colors, spacing, fontSizes, borderRadius)
MUST: Never hardcode values like style={{ margin: 10 }} or backgroundColor: '#FFFFFF'
MUST NOT: Use inline styles for any reason, including edge cases
NEVER: Hardcode design values anywhere in the codebase

✅ CORRECT PATTERN EXAMPLE

// ✅ CORRECT - Using design tokens exclusively
import { colors, spacing, fontSizes, borderRadius } from '@core/styles';

const styles = StyleSheet.create({
  container: {
    padding: spacing.md,        // ✅ Design token
    backgroundColor: colors.surface, // ✅ Design token
    fontSize: fontSizes.md,    // ✅ Design token
    borderRadius: borderRadius.md, // ✅ Design token
  },
  item: {
    marginHorizontal: spacing.md, // ✅ Design token
    gap: spacing.sm,           // ✅ Design token for flex spacing
  }
});

// ✅ CORRECT - Component using design tokens
<View style={[styles.container, appStyles.flexRow]}>
  <Text variant="bodyMedium" style={{ color: colors.onSurface }}>
    Content here
  </Text>
</View>

❌ INCORRECT PATTERN EXAMPLE

// ❌ WRONG - Inline styles and hardcoded values
const BadComponent = () => {
  return (
    <View style={{ 
      padding: 16,              // ❌ Hardcoded value
      backgroundColor: '#FFFFFF', // ❌ Hardcoded color
      fontSize: 14,             // ❌ Hardcoded font size
      borderRadius: 8           // ❌ Hardcoded border radius
    }}>
      <Text style={{ color: '#000000' }}> // ❌ Inline style
        Content here
      </Text>
    </View>
  );
};

ELEVATED COMPONENT PATTERNS (MANDATORY)

Surface + TouchableRipple Pattern (CRITICAL)

// ✅ CORRECT - Elevated list item with proper pattern
<Surface elevation={1} style={styles.itemSurface}>
  <TouchableRipple borderless onPress={handlePress} style={styles.touchable}>
    <View style={styles.content}>
      {/* List item content */}
    </View>
  </TouchableRipple>
</Surface>

// Styles:
itemSurface: {
  borderRadius: borderRadius.md,
  overflow: 'hidden', // Ensures elevation border radius works properly
  marginBottom: spacing.xs,
}

StandardCard Pattern (elevation=4)

// ✅ CORRECT - Always use StandardCard for primary content containers
import { StandardCard } from '@features/_global/components/StandardCard';

<StandardCard 
  title="Section Title" 
  subtitle="Description" 
  elevation={4} // Default elevation for primary content
>
  {children}
</StandardCard>

StandardListItem Pattern (elevation=1)

// ✅ CORRECT - Always use StandardListItem for list entries
import { StandardListItem } from '@features/_global/components/StandardListItem';

<StandardListItem 
  title={item.title}
  subtitle={item.subtitle}
  elevation={1} // Default elevation for list items
  onPress={handlePress}
/>

GLOBAL COMPONENT ENFORCEMENT (CRITICAL)

• ALWAYS use @features/_global/components/Image wrapper - NEVER use raw Image from react-native
• ALWAYS use @features/_global/components/webview - NEVER use raw WebView from react-native-webview
• ALWAYS use @features/_global/components/form-layout for forms with keyboard-aware scrolling
• ALWAYS use @features/_global/components/base-layout for basic screen structure

---

📡 ACADEMYHUB MOBILE-SPECIFIC API ENDPOINTS (MANDATORY)

Mobile applications MUST use mobile-specific API endpoints located at /api/mobile/v1/ instead of the standard /api/v1/ endpoints.

• Mobile Endpoint Base URL: /api/mobile/v1/
• Mobile Handler Location: @academyhub-backend/internal/handler/mobile/
• Mobile Route Registration: In @academyhub-backend/cmd/server/main.go under mobileApiGroup

CURRENT MOBILE-SPECIFIC HANDLERS

• Auth Handler: auth_handler.go → /api/mobile/v1/auth/*
• Profile Handler: profile_handler.go → /api/mobile/v1/profile/*
• Canteen Handler: canteen_handler.go → /api/mobile/v1/canteen/*

✅ CORRECT PATTERN EXAMPLE

// ✅ CORRECT - Mobile service using mobile-specific endpoints
import { apiClient } from '@core/services/api-client';

export const authService = {
  login: async (credentials: LoginCredentials) => {
    // Uses /api/mobile/v1/auth/login instead of /api/v1/auth/login
    const response = await apiClient.post('/mobile/v1/auth/login', credentials);
    return response.data;
  },
  
  getProfile: async () => {
    // Uses /api/mobile/v1/profile instead of /api/v1/profile  
    const response = await apiClient.get('/mobile/v1/profile');
    return response.data;
  }
};

❌ INCORRECT PATTERN EXAMPLE

// ❌ WRONG - Mobile service using web endpoints
export const authService = {
  login: async (credentials: LoginCredentials) => {
    // Uses /api/v1/auth/login - WRONG for mobile!
    const response = await apiClient.post('/v1/auth/login', credentials);
    return response.data;
  },
  
  getProfile: async () => {
    // Uses /api/v1/profile - WRONG for mobile!
    const response = await apiClient.get('/v1/profile');
    return response.data;
  }
};

REFACTORING GUIDELINES FOR API ENDPOINTS

• Identify Mobile vs Web Usage: During refactoring, verify if the mobile app should use mobile-specific endpoints
• Update Service Calls: Change endpoint URLs from /api/v1/ to /api/mobile/v1/ for auth, profile, and canteen features
• Verify Handler Existence: Ensure corresponding mobile handlers exist in backend before changing endpoints
• Test Endpoint Compatibility: Mobile endpoints may have different response formats or authentication requirements

MUST: Mobile services call /api/mobile/v1/ endpoints
MUST NOT: Mobile services call /api/v1/ endpoints for auth, profile, or canteen functionality
NEVER: Mix mobile and web endpoints in the same service

---

✅ ACADEMYHUB REFACTORING VERIFICATION (ORIGINAL BEST PRACTICES PRESERVED)

All original refactoring best practices from the base skill are PRESERVED and ENHANCED with AcademyHub architectural constraints:

ORIGINAL PRINCIPLES MAINTAINED

• Behavior Preservation: Refactoring doesn't change what the code does, only how it's structured
• Small Steps: Make tiny changes, test after each step
• Version Control: Commit before and after each safe state
• Test Essential: Without tests, you're not refactoring, you're editing
• One Thing at a Time: Don't mix refactoring with feature changes

SURGICAL FOCUS ENHANCED

The original "surgical code refactoring" focus is enhanced (not diluted) by adding AcademyHub-specific guardrails:

• Backend Surgical Focus: Clean Architecture patterns make refactoring more precise and safer
• Mobile Surgical Focus: Fat Screen prevention ensures refactoring targets the right layer
• Performance Surgical Focus: FlashList requirements target specific performance bottlenecks
• Security Surgical Focus: Design token enforcement removes hardcoded security risks

REFACTORING CHECKLIST (ACADEMYHUB ENHANCED)

Code Quality

• Functions are small (< 50 lines)
• Functions do one thing
• No duplicated code
• Descriptive names (variables, functions, classes)
• No magic numbers/strings
• Dead code removed
• AcademyHub: Clean Architecture compliance verified
• AcademyHub: Fat Screen limits respected (<300 lines screens, <400 lines hooks)

Structure

• Related code is together
• Clear module boundaries
• Dependencies flow in one direction
• No circular dependencies
• AcademyHub: Repository interfaces in domain layer only
• AcademyHub: DTOs properly isolated in separate directory

Type Safety

• Types defined for all public APIs
• No any types without justification
• Nullable types explicitly marked
• AcademyHub: Mobile endpoints use correct /mobile/v1/ paths

Performance & UI

• AcademyHub: FlashList used instead of FlatList/ScrollView.map
• AcademyHub: estimatedItemSize included for all FlashLists
• AcademyHub: Zero inline styles - all design tokens used
• AcademyHub: Proper elevated MD3 components (StandardCard, StandardListItem)

Testing

• Refactored code is tested
• Tests cover edge cases
• All tests pass
• AcademyHub: Transaction safety verified for financial operations

---

Common Code Smells & Fixes

1. Long Method/Function

# BAD: 200-line function that does everything
- async function processOrder(orderId) {
-   // 50 lines: fetch order
-   // 30 lines: validate order
-   // 40 lines: calculate pricing
-   // 30 lines: update inventory
-   // 20 lines: create shipment
-   // 30 lines: send notifications
- }

# GOOD: Broken into focused functions
+ async function processOrder(orderId) {
+   const order = await fetchOrder(orderId);
+   validateOrder(order);
+   const pricing = calculatePricing(order);
+   await updateInventory(order);
+   const shipment = await createShipment(order);
+   await sendNotifications(order, pricing, shipment);
+   return { order, pricing, shipment };
+ }

2. Duplicated Code

# BAD: Same logic in multiple places
- function calculateUserDiscount(user) {
-   if (user.membership === 'gold') return user.total * 0.2;
-   if (user.membership === 'silver') return user.total * 0.1;
-   return 0;
- }
-
- function calculateOrderDiscount(order) {
-   if (order.user.membership === 'gold') return order.total * 0.2;
-   if (order.user.membership === 'silver') return order.total * 0.1;
-   return 0;
- }

# GOOD: Extract common logic
+ function getMembershipDiscountRate(membership) {
+   const rates = { gold: 0.2, silver: 0.1 };
+   return rates[membership] || 0;
+ }
+
+ function calculateUserDiscount(user) {
+   return user.total * getMembershipDiscountRate(user.membership);
+ }
+
+ function calculateOrderDiscount(order) {
+   return order.total * getMembershipDiscountRate(order.user.membership);
+ }

3. Large Class/Module

# BAD: God object that knows too much
- class UserManager {
-   createUser() { /* ... */ }
-   updateUser() { /* ... */ }
-   deleteUser() { /* ... */ }
-   sendEmail() { /* ... */ }
-   generateReport() { /* ... */ }
-   handlePayment() { /* ... */ }
-   validateAddress() { /* ... */ }
-   // 50 more methods...
- }

# GOOD: Single responsibility per class
+ class UserService {
+   create(data) { /* ... */ }
+   update(id, data) { /* ... */ }
+   delete(id) { /* ... */ }
+ }
+
+ class EmailService {
+   send(to, subject, body) { /* ... */ }
+ }
+
+ class ReportService {
+   generate(type, params) { /* ... */ }
+ }
+
+ class PaymentService {
+   process(amount, method) { /* ... */ }
+ }

4. Long Parameter List

# BAD: Too many parameters
- function createUser(email, password, name, age, address, city, country, phone) {
-   /* ... */
- }

# GOOD: Group related parameters
+ interface UserData {
+   email: string;
+   password: string;
+   name: string;
+   age?: number;
+   address?: Address;
+   phone?: string;
+ }
+
+ function createUser(data: UserData) {
+   /* ... */
+ }

# EVEN BETTER: Use builder pattern for complex construction
+ const user = UserBuilder
+   .email('[e-mail]')
+   .password('secure123')
+   .name('Test User')
+   .address(address)
+   .build();

5. Feature Envy

# BAD: Method that uses another object's data more than its own
- class Order {
-   calculateDiscount(user) {
-     if (user.membershipLevel === 'gold') {
+       return this.total * 0.2;
+     }
+     if (user.accountAge > 365) {
+       return this.total * 0.1;
+     }
+     return 0;
+   }
+ }

# GOOD: Move logic to the object that owns the data
+ class User {
+   getDiscountRate(orderTotal) {
+     if (this.membershipLevel === 'gold') return 0.2;
+     if (this.accountAge > 365) return 0.1;
+     return 0;
+   }
+ }
+
+ class Order {
+   calculateDiscount(user) {
+     return this.total * user.getDiscountRate(this.total);
+   }
+ }

6. Primitive Obsession

# BAD: Using primitives for domain concepts
- function sendEmail(to, subject, body) { /* ... */ }
- sendEmail('[e-mail]', 'Hello', '...');

- function createPhone(country, number) {
-   return `${country}-${number}`;
- }

# GOOD: Use domain types
+ class Email {
+   private constructor(public readonly value: string) {
+     if (!Email.isValid(value)) throw new Error('Invalid email');
+   }
+   static create(value: string) { return new Email(value); }
+   static isValid(email: string) { return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email); }
+ }
+
+ class PhoneNumber {
+   constructor(
+     public readonly country: string,
+     public readonly number: string
+   ) {
+     if (!PhoneNumber.isValid(country, number)) throw new Error('Invalid phone');
+   }
+   toString() { return `${this.country}-${this.number}`; }
+   static isValid(country: string, number: string) { /* ... */ }
+ }
+
+ // Usage
+ const email = Email.create('[e-mail]');
+ const phone = new PhoneNumber('1', '555-1234');

7. Magic Numbers/Strings

# BAD: Unexplained values
- if (user.status === 2) { /* ... */ }
- const discount = total * 0.15;
- setTimeout(callback, 86400000);

# GOOD: Named constants
+ const UserStatus = {
+   ACTIVE: 1,
+   INACTIVE: 2,
+   SUSPENDED: 3
+ } as const;
+
+ const DISCOUNT_RATES = {
+   STANDARD: 0.1,
+   PREMIUM: 0.15,
+   VIP: 0.2
+ } as const;
+
+ const ONE_DAY_MS = 24 * 60 * 60 * 1000;
+
+ if (user.status === UserStatus.INACTIVE) { /* ... */ }
+ const discount = total * DISCOUNT_RATES.PREMIUM;
+ setTimeout(callback, ONE_DAY_MS);

8. Nested Conditionals

# BAD: Arrow code
- function process(order) {
-   if (order) {
-     if (order.user) {
-       if (order.user.isActive) {
-         if (order.total > 0) {
-           return processOrder(order);
+         } else {
+           return { error: 'Invalid total' };
+         }
+       } else {
+         return { error: 'User inactive' };
+       }
+     } else {
+       return { error: 'No user' };
+     }
+   } else {
+     return { error: 'No order' };
+   }
+ }

# GOOD: Guard clauses / early returns
+ function process(order) {
+   if (!order) return { error: 'No order' };
+   if (!order.user) return { error: 'No user' };
+   if (!order.user.isActive) return { error: 'User inactive' };
+   if (order.total <= 0) return { error: 'Invalid total' };
+   return processOrder(order);
+ }

# EVEN BETTER: Using Result type
+ function process(order): Result<ProcessedOrder, Error> {
+   return Result.combine([
+     validateOrderExists(order),
+     validateUserExists(order),
+     validateUserActive(order.user),
+     validateOrderTotal(order)
+   ]).flatMap(() => processOrder(order));
+ }

9. Dead Code

# BAD: Unused code lingers
- function oldImplementation() { /* ... */ }
- const DEPRECATED_VALUE = 5;
- import { unusedThing } from './somewhere';
- // Commented out code
- // function oldCode() { /* ... */ }

# GOOD: Remove it
+ // Delete unused functions, imports, and commented code
+ // If you need it again, git history has it

10. Inappropriate Intimacy

# BAD: One class reaches deep into another
- class OrderProcessor {
-   process(order) {
-     order.user.profile.address.street;  // Too intimate
-     order.repository.connection.config;  // Breaking encapsulation
+   }
+ }

# GOOD: Ask, don't tell
+ class OrderProcessor {
+   process(order) {
+     order.getShippingAddress();  // Order knows how to get it
+     order.save();  // Order knows how to save itself
+   }
+ }

---

Extract Method Refactoring

Before and After

# Before: One long function
- function printReport(users) {
-   console.log('USER REPORT');
-   console.log('============');
-   console.log('');
-   console.log(`Total users: ${users.length}`);
-   console.log('');
-   console.log('ACTIVE USERS');
-   console.log('------------');
-   const active = users.filter(u => u.isActive);
-   active.forEach(u => {
-     console.log(`- ${u.name} (${u.email})`);
-   });
-   console.log('');
-   console.log(`Active: ${active.length}`);
-   console.log('');
-   console.log('INACTIVE USERS');
-   console.log('--------------');
-   const inactive = users.filter(u => !u.isActive);
-   inactive.forEach(u => {
-     console.log(`- ${u.name} (${u.email})`);
-   });
-   console.log('');
-   console.log(`Inactive: ${inactive.length}`);
- }

# After: Extracted methods
+ function printReport(users) {
+   printHeader('USER REPORT');
+   console.log(`Total users: ${users.length}\n`);
+   printUserSection('ACTIVE USERS', users.filter(u => u.isActive));
+   printUserSection('INACTIVE USERS', users.filter(u => !u.isActive));
+ }
+
+ function printHeader(title) {
+   const line = '='.repeat(title.length);
+   console.log(title);
+   console.log(line);
+   console.log('');
+ }
+
+ function printUserSection(title, users) {
+   console.log(title);
+   console.log('-'.repeat(title.length));
+   users.forEach(u => console.log(`- ${u.name} (${u.email})`));
+   console.log('');
+   console.log(`${title.split(' ')[0]}: ${users.length}`);
+   console.log('');
+ }

---

Introducing Type Safety

From Untyped to Typed

# Before: No types
- function calculateDiscount(user, total, membership, date) {
-   if (membership === 'gold' && date.getDay() === 5) {
-     return total * 0.25;
-   }
-   if (membership === 'gold') return total * 0.2;
-   return total * 0.1;
- }

# After: Full type safety
+ type Membership = 'bronze' | 'silver' | 'gold';
+
+ interface User {
+   id: string;
+   name: string;
+   membership: Membership;
+ }
+
+ interface DiscountResult {
+   original: number;
+   discount: number;
+   final: number;
+   rate: number;
+ }
+
+ function calculateDiscount(
+   user: User,
+   total: number,
+   date: Date = new Date()
+ ): DiscountResult {
+   if (total < 0) throw new Error('Total cannot be negative');
+
+   let rate = 0.1; // Default bronze
+
+   if (user.membership === 'gold' && date.getDay() === 5) {
+     rate = 0.25; // Friday bonus for gold
+   } else if (user.membership === 'gold') {
+     rate = 0.2;
+   } else if (user.membership === 'silver') {
+     rate = 0.15;
+   }
+
+   const discount = total * rate;
+
+   return {
+     original: total,
+     discount,
+     final: total - discount,
+     rate
+   };
+ }

---

Design Patterns for Refactoring

Strategy Pattern

# Before: Conditional logic
- function calculateShipping(order, method) {
-   if (method === 'standard') {
-     return order.total > 50 ? 0 : 5.99;
-   } else if (method === 'express') {
-     return order.total > 100 ? 9.99 : 14.99;
+   } else if (method === 'overnight') {
+     return 29.99;
+   }
+ }

# After: Strategy pattern
+ interface ShippingStrategy {
+   calculate(order: Order): number;
+ }
+
+ class StandardShipping implements ShippingStrategy {
+   calculate(order: Order) {
+     return order.total > 50 ? 0 : 5.99;
+   }
+ }
+
+ class ExpressShipping implements ShippingStrategy {
+   calculate(order: Order) {
+     return order.total > 100 ? 9.99 : 14.99;
+   }
+ }
+
+ class OvernightShipping implements ShippingStrategy {
+   calculate(order: Order) {
+     return 29.99;
+   }
+ }
+
+ function calculateShipping(order: Order, strategy: ShippingStrategy) {
+   return strategy.calculate(order);
+ }

Chain of Responsibility

# Before: Nested validation
- function validate(user) {
-   const errors = [];
-   if (!user.email) errors.push('Email required');
+   else if (!isValidEmail(user.email)) errors.push('Invalid email');
+   if (!user.name) errors.push('Name required');
+   if (user.age < 18) errors.push('Must be 18+');
+   if (user.country === 'blocked') errors.push('Country not supported');
+   return errors;
+ }

# After: Chain of responsibility
+ abstract class Validator {
+   abstract validate(user: User): string | null;
+   setNext(validator: Validator): Validator {
+     this.next = validator;
+     return validator;
+   }
+   validate(user: User): string | null {
+     const error = this.doValidate(user);
+     if (error) return error;
+     return this.next?.validate(user) ?? null;
+   }
+ }
+
+ class EmailRequiredValidator extends Validator {
+   doValidate(user: User) {
+     return !user.email ? 'Email required' : null;
+   }
+ }
+
+ class EmailFormatValidator extends Validator {
+   doValidate(user: User) {
+     return user.email && !isValidEmail(user.email) ? 'Invalid email' : null;
+   }
+ }
+
+ // Build the chain
+ const validator = new EmailRequiredValidator()
+   .setNext(new EmailFormatValidator())
+   .setNext(new NameRequiredValidator())
+   .setNext(new AgeValidator())
+   .setNext(new CountryValidator());

---

Refactoring Steps

Safe Refactoring Process

1. PREPARE
   - Ensure tests exist (write them if missing)
   - Commit current state
   - Create feature branch

2. IDENTIFY
   - Find the code smell to address
   - Understand what the code does
   - Plan the refactoring

3. REFACTOR (small steps)
   - Make one small change
   - Run tests
   - Commit if tests pass
   - Repeat

4. VERIFY
   - All tests pass
   - Manual testing if needed
   - Performance unchanged or improved

5. CLEAN UP
   - Update comments
   - Update documentation
   - Final commit

---

Refactoring Checklist

Code Quality

• Functions are small (< 50 lines)
• Functions do one thing
• No duplicated code
• Descriptive names (variables, functions, classes)
• No magic numbers/strings
• Dead code removed

Structure

• Related code is together
• Clear module boundaries
• Dependencies flow in one direction
• No circular dependencies

Type Safety

• Types defined for all public APIs
• No any types without justification
• Nullable types explicitly marked

Testing

• Refactored code is tested
• Tests cover edge cases
• All tests pass

---

Common Refactoring Operations

Operation	Description
Extract Method	Turn code fragment into method
Extract Class	Move behavior to new class
Extract Interface	Create interface from implementation
Inline Method	Move method body back to caller
Inline Class	Move class behavior to caller
Pull Up Method	Move method to superclass
Push Down Method	Move method to subclass
Rename Method/Variable	Improve clarity
Introduce Parameter Object	Group related parameters
Replace Conditional with Polymorphism	Use polymorphism instead of switch/if
Replace Magic Number with Constant	Named constants
Decompose Conditional	Break complex conditions
Consolidate Conditional	Combine duplicate conditions
Replace Nested Conditional with Guard Clauses	Early returns
Introduce Null Object	Eliminate null checks
Replace Type Code with Class/Enum	Strong typing
Replace Inheritance with Delegation	Composition over inheritance