refactoring-advisor

PHP/CakePHP Refactoring Advisor

Safety Notice

This listing is imported from skills.sh public index metadata. Review upstream SKILL.md and repository scripts before running.

Copy this and send it to your AI assistant to learn

Install skill "refactoring-advisor" with this command: npx skills add masanao-ohba/claude-manifests/masanao-ohba-claude-manifests-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); }

  1. 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();

  1. 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();

  1. 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'); } }

  1. 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
}

}

  1. 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
  1. 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

  1. 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

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.

Source Transparency

This detail page is rendered from real SKILL.md content. Trust labels are metadata-based hints, not a safety guarantee.

Open in GitHubOpen in ClawHub

Related Skills

Related by shared tags or category signals.

General

requirement-analyzer

No summary provided by upstream source.

Repository SourceNeeds Review
15-masanao-ohba
General

test-case-designer

No summary provided by upstream source.

Repository SourceNeeds Review
14-masanao-ohba
General

functional-designer

No summary provided by upstream source.

Repository SourceNeeds Review
11-masanao-ohba
General

test-validator

No summary provided by upstream source.

Repository SourceNeeds Review
11-masanao-ohba