Refactoring Advisor

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**:
```php
// 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

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

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