PHP/CakePHP Refactoring Advisor

A specialized skill for identifying refactoring opportunities and providing actionable recommendations for PHP/CakePHP applications.

Core Responsibilities

1. Code Smell Detection

Long Method:

// 🔴 Code Smell: Method > 50 lines
public function processOrder($data)
{
    // 100+ lines of code doing multiple things
    // Validation logic
    // Calculation logic
    // Database operations
    // Email sending
    // Logging
}

// ✅ Refactored: Extracted methods
public function processOrder($data)
{
    $this->validateOrderData($data);
    $total = $this->calculateOrderTotal($data);
    $order = $this->createOrder($data, $total);
    $this->sendOrderConfirmation($order);
    $this->logOrderCreation($order);
    return $order;
}

private function validateOrderData($data) { /* ... */ }
private function calculateOrderTotal($data) { /* ... */ }
private function createOrder($data, $total) { /* ... */ }
private function sendOrderConfirmation($order) { /* ... */ }
private function logOrderCreation($order) { /* ... */ }

Large Class:

// 🔴 Code Smell: Class with too many responsibilities
class UserController extends AppController
{
    // 50+ methods handling everything
    public function login() { }
    public function register() { }
    public function sendEmail() { }
    public function generateReport() { }
    public function processPayment() { }
    // ... many more
}

// ✅ Refactored: Separated concerns
class UserController extends AppController
{
    public function login() { }
    public function register() { }
}

class ReportController extends AppController
{
    public function generate() { }
}

class PaymentService
{
    public function process() { }
}

Duplicate Code:

// 🔴 Code Smell: Duplicated logic
public function approveApplication($id)
{
    $app = $this->Applications->get($id);
    $app->status = 3;
    $app->approved_date = date('Y-m-d');
    $this->Applications->save($app);
    $this->sendNotification($app->user_id, 'approved');
}

public function rejectApplication($id)
{
    $app = $this->Applications->get($id);
    $app->status = 4;
    $app->rejected_date = date('Y-m-d');
    $this->Applications->save($app);
    $this->sendNotification($app->user_id, 'rejected');
}

// ✅ Refactored: Extract common logic
public function updateApplicationStatus($id, $status)
{
    $app = $this->Applications->get($id);
    $app = $this->Applications->updateStatus($app, $status);
    $this->Applications->save($app);
    $this->sendStatusNotification($app);
}

2. Design Pattern Application

Repository Pattern:

// 🔴 Before: Direct table access everywhere
class UsersController extends AppController
{
    public function getActiveUsers()
    {
        return $this->Users->find()
            ->where(['status' => 1, 'del_flg' => 0])
            ->contain(['Orders', 'Company'])
            ->order(['created' => 'DESC'])
            ->all();
    }
}

// ✅ After: Repository pattern
// src/Repository/UserRepository.php
class UserRepository
{
    private $Users;

    public function __construct()
    {
        $this->Users = TableRegistry::getTableLocator()->get('Users');
    }

    public function findActive()
    {
        return $this->Users->find('active')
            ->contain(['Orders', 'Company'])
            ->order(['created' => 'DESC']);
    }
}

// Controller using repository
class UsersController extends AppController
{
    private $userRepository;

    public function initialize(): void
    {
        parent::initialize();
        $this->userRepository = new UserRepository();
    }

    public function getActiveUsers()
    {
        return $this->userRepository->findActive()->all();
    }
}

Service Layer Pattern:

// 🔴 Before: Business logic in controller
class OrdersController extends AppController
{
    public function create()
    {
        // Complex business logic in controller
        $data = $this->request->getData();

        // Validate inventory
        foreach ($data['items'] as $item) {
            $product = $this->Products->get($item['product_id']);
            if ($product->stock < $item['quantity']) {
                $this->Flash->error('在庫不足');
                return;
            }
        }

        // Calculate totals
        $total = 0;
        foreach ($data['items'] as $item) {
            $total += $item['price'] * $item['quantity'];
        }

        // Create order
        // Update inventory
        // Send email
        // etc...
    }
}

// ✅ After: Service layer
// src/Service/OrderService.php
class OrderService
{
    public function createOrder(array $data): OrderResult
    {
        $validation = $this->validateInventory($data['items']);
        if (!$validation->isValid()) {
            return OrderResult::error($validation->getErrors());
        }

        $total = $this->calculateTotal($data['items']);
        $order = $this->processOrder($data, $total);

        return OrderResult::success($order);
    }

    private function validateInventory(array $items) { }
    private function calculateTotal(array $items) { }
    private function processOrder(array $data, float $total) { }
}

// Simplified controller
class OrdersController extends AppController
{
    public function create()
    {
        $result = $this->OrderService->createOrder($this->request->getData());

        if ($result->isSuccess()) {
            $this->Flash->success('注文完了');
            return $this->redirect(['action' => 'view', $result->getOrder()->id]);
        }

        $this->Flash->error($result->getErrorMessage());
    }
}

Factory Pattern:

// 🔴 Before: Complex object creation in multiple places
$notification = new EmailNotification();
$notification->setTo($user->email);
$notification->setSubject('Order Confirmation');
$notification->setTemplate('order_confirm');
$notification->setData($orderData);
$notification->send();

// ✅ After: Factory pattern
class NotificationFactory
{
    public static function create(string $type, array $config): NotificationInterface
    {
        switch ($type) {
            case 'email':
                return new EmailNotification($config);
            case 'sms':
                return new SmsNotification($config);
            case 'line':
                return new LineNotification($config);
            default:
                throw new InvalidArgumentException("Unknown notification type: {$type}");
        }
    }
}

// Usage
$notification = NotificationFactory::create('email', [
    'to' => $user->email,
    'template' => 'order_confirm',
    'data' => $orderData,
]);
$notification->send();

3. Performance Refactoring

Query Optimization:

// 🔴 Before: N+1 queries
$users = $this->Users->find()->all();
foreach ($users as $user) {
    $orderCount = $this->Orders->find()
        ->where(['user_id' => $user->id])
        ->count();
    $user->order_count = $orderCount;
}

// ✅ After: Single query with join
$users = $this->Users->find()
    ->select([
        'Users.id',
        'Users.name',
        'order_count' => $this->Orders->find()->func()->count('Orders.id')
    ])
    ->leftJoinWith('Orders')
    ->group(['Users.id'])
    ->all();

Caching Implementation:

// 🔴 Before: Expensive calculation every time
public function getMonthlyStats($month)
{
    $stats = [];
    $startDate = new DateTime("first day of {$month}");
    $endDate = new DateTime("last day of {$month}");

    // Complex calculations
    $stats['total_orders'] = $this->calculateTotalOrders($startDate, $endDate);
    $stats['revenue'] = $this->calculateRevenue($startDate, $endDate);
    $stats['new_users'] = $this->calculateNewUsers($startDate, $endDate);

    return $stats;
}

// ✅ After: With caching
public function getMonthlyStats($month)
{
    $cacheKey = "monthly_stats_{$month}";

    return Cache::remember($cacheKey, function () use ($month) {
        $stats = [];
        $startDate = new DateTime("first day of {$month}");
        $endDate = new DateTime("last day of {$month}");

        $stats['total_orders'] = $this->calculateTotalOrders($startDate, $endDate);
        $stats['revenue'] = $this->calculateRevenue($startDate, $endDate);
        $stats['new_users'] = $this->calculateNewUsers($startDate, $endDate);

        return $stats;
    }, 'daily'); // Cache for 24 hours
}

Lazy Loading to Eager Loading:

// 🔴 Before: Lazy loading
$applications = $this->Applications->find()->all();
foreach ($applications as $app) {
    // Each access triggers a query
    echo $app->user->name;
    echo $app->category->name;
    echo $app->allocation->status;
}

// ✅ After: Eager loading
$applications = $this->Applications->find()
    ->contain(['Users', 'Categories', 'Allocations'])
    ->all();

4. CakePHP-Specific Refactoring

Component Extraction:

// 🔴 Before: Repeated code in controllers
class UsersController extends AppController
{
    public function export()
    {
        // CSV export logic
        $data = $this->Users->find()->all();
        $csv = fopen('php://output', 'w');
        // ... export logic
    }
}

class OrdersController extends AppController
{
    public function export()
    {
        // Same CSV export logic repeated
        $data = $this->Orders->find()->all();
        $csv = fopen('php://output', 'w');
        // ... export logic
    }
}

// ✅ After: Component
// src/Controller/Component/CsvExportComponent.php
class CsvExportComponent extends Component
{
    public function export($query, array $fields)
    {
        $data = $query->all();
        $csv = fopen('php://output', 'w');
        // ... reusable export logic
        return $csv;
    }
}

// Usage in controllers
class UsersController extends AppController
{
    public function initialize(): void
    {
        parent::initialize();
        $this->loadComponent('CsvExport');
    }

    public function export()
    {
        $query = $this->Users->find();
        return $this->CsvExport->export($query, ['id', 'name', 'email']);
    }
}

Behavior Implementation:

// 🔴 Before: Repeated model logic
class UsersTable extends Table
{
    public function beforeSave($event, $entity, $options)
    {
        if ($entity->isNew()) {
            $entity->created_by = $this->getCurrentUserId();
        }
        $entity->modified_by = $this->getCurrentUserId();
    }
}

class OrdersTable extends Table
{
    public function beforeSave($event, $entity, $options)
    {
        // Same audit logic repeated
        if ($entity->isNew()) {
            $entity->created_by = $this->getCurrentUserId();
        }
        $entity->modified_by = $this->getCurrentUserId();
    }
}

// ✅ After: Behavior
// src/Model/Behavior/AuditableBehavior.php
class AuditableBehavior extends Behavior
{
    public function beforeSave(EventInterface $event, EntityInterface $entity, ArrayObject $options)
    {
        $userId = $this->getCurrentUserId();

        if ($entity->isNew()) {
            $entity->created_by = $userId;
        }
        $entity->modified_by = $userId;
    }
}

// Usage in tables
class UsersTable extends Table
{
    public function initialize(array $config): void
    {
        $this->addBehavior('Auditable');
    }
}

5. Testing Improvements

Extract Test Helpers:

// 🔴 Before: Repeated test setup
class UserControllerTest extends TestCase
{
    public function testIndex()
    {
        // Complex authentication setup
        $user = $this->Users->get(1);
        $this->session(['Auth.User' => $user]);
        // ... test logic
    }

    public function testView()
    {
        // Same authentication setup repeated
        $user = $this->Users->get(1);
        $this->session(['Auth.User' => $user]);
        // ... test logic
    }
}

// ✅ After: Test helper trait
trait AuthenticationTestTrait
{
    protected function loginAs($userId = 1)
    {
        $user = $this->getTableLocator()->get('Users')->get($userId);
        $this->session(['Auth.User' => $user->toArray()]);
        return $user;
    }

    protected function loginAsAdmin()
    {
        return $this->loginAs($this->getAdminUserId());
    }
}

class UserControllerTest extends TestCase
{
    use AuthenticationTestTrait;

    public function testIndex()
    {
        $this->loginAs();
        // ... test logic
    }

    public function testAdminView()
    {
        $this->loginAsAdmin();
        // ... test logic
    }
}

6. Database Refactoring

Normalization:

-- 🔴 Before: Denormalized table
CREATE TABLE orders (
    id INT PRIMARY KEY,
    user_name VARCHAR(255),
    user_email VARCHAR(255),
    user_phone VARCHAR(20),
    product_name VARCHAR(255),
    product_price DECIMAL(10,2),
    quantity INT
);

-- ✅ After: Normalized tables
CREATE TABLE users (
    id INT PRIMARY KEY,
    name VARCHAR(255),
    email VARCHAR(255),
    phone VARCHAR(20)
);

CREATE TABLE products (
    id INT PRIMARY KEY,
    name VARCHAR(255),
    price DECIMAL(10,2)
);

CREATE TABLE orders (
    id INT PRIMARY KEY,
    user_id INT,
    created DATETIME,
    FOREIGN KEY (user_id) REFERENCES users(id)
);

CREATE TABLE order_items (
    id INT PRIMARY KEY,
    order_id INT,
    product_id INT,
    quantity INT,
    price DECIMAL(10,2),
    FOREIGN KEY (order_id) REFERENCES orders(id),
    FOREIGN KEY (product_id) REFERENCES products(id)
);

Refactoring Process

1. Identify Candidates

Indicators:
  - High cyclomatic complexity (> 20)
  - Long methods (> 50 lines)
  - Large classes (> 500 lines)
  - Duplicate code blocks
  - Deep nesting (> 3 levels)
  - Too many parameters (> 4)
  - Feature envy
  - Data clumps

2. Prioritize Refactoring

Priority Matrix:
  High:
    - Security vulnerabilities
    - Performance bottlenecks
    - Frequently modified code
    - High-bug areas

  Medium:
    - Code smells in stable code
    - Moderate complexity
    - Occasional modifications

  Low:
    - Cosmetic issues
    - Rarely modified code
    - Low-impact areas

3. Refactoring Steps

Process:
  1. Write tests for current behavior
  2. Make small, incremental changes
  3. Run tests after each change
  4. Commit working code frequently
  5. Document significant changes

Refactoring Catalog

Method-Level Refactoring

Extract Method
Inline Method
Extract Variable
Inline Variable
Replace Temp with Query
Split Temporary Variable
Remove Assignments to Parameters
Substitute Algorithm

Class-Level Refactoring

Extract Class
Inline Class
Extract Interface
Move Method
Move Field
Extract Superclass
Extract Subclass
Collapse Hierarchy
Form Template Method
Replace Inheritance with Delegation

Data Refactoring

Replace Magic Number with Constant
Encapsulate Field
Replace Array with Object
Replace Data Value with Object
Change Value to Reference
Change Reference to Value

Conditional Refactoring

Decompose Conditional
Consolidate Conditional Expression
Consolidate Duplicate Conditional Fragments
Replace Nested Conditional with Guard Clauses
Replace Conditional with Polymorphism
Introduce Null Object

API Refactoring

Rename Method
Add Parameter
Remove Parameter
Separate Query from Modifier
Parameterize Method
Replace Parameter with Explicit Methods
Preserve Whole Object
Replace Parameter with Method Call

Output Format

## Refactoring Recommendations: [Component Name]

### Executive Summary
- **Technical Debt Score**: High/Medium/Low
- **Estimated Effort**: X days
- **Risk Level**: Low/Medium/High
- **ROI**: High/Medium/Low

### Priority 1: Critical Refactoring

#### 1. Extract Service Layer from OrdersController
**Problem**: Controller contains 300+ lines of business logic
**Impact**: Hard to test, violates SRP, difficult to maintain

**Solution**:

// Create OrderService class class OrderService { public function processOrder($data) { } public function validateOrder($data) { } public function calculateTotals($items) { } }


**Benefits**:
- Improved testability
- Reusable business logic
- Clear separation of concerns

**Estimated Time**: 4 hours

### Priority 2: Important Refactoring

#### 1. Replace N+1 Queries with Eager Loading
**Location**: ApplicationsController::index()

**Current Code**:

foreach ($applications as $app) { $user = $app->user; // Triggers query }


**Refactored Code**:

$applications = $this->Applications->find() ->contain(['Users']) ->all();


**Performance Gain**: 50% reduction in database queries

### Priority 3: Nice-to-Have

#### 1. Extract Validation Rules to Separate Class
- Move validation logic from controller to dedicated validator
- Estimated time: 2 hours
- Low risk, medium benefit

### Implementation Plan

Week 1:
- [ ] Extract OrderService (8h)
- [ ] Fix N+1 queries (4h)
- [ ] Add caching layer (4h)

Week 2:
- [ ] Normalize database tables (8h)
- [ ] Extract common behaviors (4h)
- [ ] Update tests (4h)

### Metrics to Track
- Cyclomatic complexity reduction
- Test coverage improvement
- Performance benchmarks
- Bug reduction rate

Best Practices

Test First: Ensure tests exist before refactoring
Small Steps: Make incremental changes
Preserve Behavior: Don't change functionality while refactoring
Commit Often: Keep refactoring commits separate
Document Decisions: Explain why refactoring was done
Measure Impact: Track improvements

Remember: Refactoring is about improving code structure without changing behavior. Always have tests in place before refactoring.

refactoring-advisor

Installation

Similar popular skills

Also in this package

Agent compatibility

Package contents

History

SKILL.md