Loading...
Loading...
Opinionated constraints for writing maintainable, testable code. Apply MTE principles, avoid over-engineering, guide refactoring, and provide code review checklists. Use when users write code, refactor, or need code review. Triggers on keywords like "code quality", "refactor", "review", "MTE", "代码质量", "重构", "审查".
npx skill4agent add ab300819/skills code-quality| Principle | Description | Checkpoints |
|---|---|---|
| Maintainability | Maintainability | Single responsibility, clear dependencies, easy to understand |
| Testability | Testability | Core logic can be unit tested, dependencies can be mocked |
| Extensibility | Extensibility | Reasonable extension points reserved, interface abstraction |
✅ Correct: A class/function does only one thing
- UserService: User business logic
- UserRepository: User data access
- UserValidator: User data validation
❌ Incorrect: A class does multiple things
- UserManager: Business logic + data access + validation + email sending┌─────────────────────────────────────┐
│ Interface Layer │ ← Thin layer, no business logic
├─────────────────────────────────────┤
│ Service Layer │ ← Business logic (core)
├─────────────────────────────────────┤
│ Domain Layer │ ← Domain model
├─────────────────────────────────────┤
│ Infrastructure Layer │ ← Replaceable
└─────────────────────────────────────┘
Dependency Rules:
- Outer layers depend on inner layers ✅
- Inner layers depend on outer layers ❌
- Depend on interfaces, not implementations ✅// ❌ Too many parameters
function createUser(name, email, age, role, department, manager) {}
// ✅ Use parameter object
function createUser(params: CreateUserParams) {}// ❌ Excessive nesting
if (a) {
if (b) {
if (c) {
// ...
}
}
}
// ✅ Early return
if (!a) return;
if (!b) return;
if (!c) return;
// ...For detailed testing specifications, refer to/testing-guide
Three elements of testable code:
1. Injectable dependencies - External dependencies are passed via parameters
2. Prefer pure functions - Business logic has no side effects
3. Boundary separation - Separate business logic from IO operations// ❌ Hard-coded dependencies
class UserService {
private db = new Database();
private mailer = new EmailService();
}
// ✅ Dependency injection
class UserService {
constructor(
private db: IDatabase,
private mailer: IEmailService
) {}
}// ❌ Has side effects, hard to test
function calculateTotal(items) {
const total = items.reduce((sum, item) => sum + item.price, 0);
console.log(`Total: ${total}`); // Side effect
analytics.track('calculate'); // Side effect
return total;
}
// ✅ Pure function, easy to test
function calculateTotal(items) {
return items.reduce((sum, item) => sum + item.price, 0);
}// ❌ Business logic mixed with IO
async function processOrder(orderId) {
const order = await db.findOrder(orderId); // IO
if (order.total > 1000) { // Business logic
order.discount = 0.1;
}
await db.save(order); // IO
await email.send(order.user, 'confirmed'); // IO
}
// ✅ Separate business logic
function applyDiscount(order) { // Pure business logic, testable
if (order.total > 1000) {
return { ...order, discount: 0.1 };
}
return order;
}
async function processOrder(orderId) {
const order = await db.findOrder(orderId);
const updated = applyDiscount(order); // Call pure function
await db.save(updated);
await email.send(order.user, 'confirmed');
}When writing tests: Refer tofor detailed guidance on assertion quality, mocking strategies, mutation testing, etc./testing-guide
You Aren't Gonna Need It
// ❌ Over-engineering: Adding configurations for hypothetical requirements
const config = {
maxRetries: 3,
retryDelay: 1000,
enableCache: true,
cacheExpiry: 3600,
enableRateLimit: false, // Never used
rateLimitWindow: 60000, // Never used
enableMetrics: false, // Never used
metricsEndpoint: '', // Never used
};
// ✅ Only add configurations needed currently
const config = {
maxRetries: 3,
retryDelay: 1000,
cacheExpiry: 3600,
};❌ Premature abstraction:
- Create an interface when there's only one implementation
- Extract a function when it's only used once
- Create a base class when there are only two similar scenarios
✅ Timely abstraction (Rule of Three):
- Consider interfaces when there are 3+ implementations
- Consider extraction when there are 3+ repetitions
- Consider base classes when there are 3+ similar scenarios// ❌ Over-engineering: Using complex patterns for simple scenarios
class UserFactory {
createAdmin() { return new AdminUser(); }
createMember() { return new MemberUser(); }
createGuest() { return new GuestUser(); }
}
// ✅ Use simple solutions for simple scenarios
function createUser(role: 'admin' | 'member' | 'guest') {
return { role, permissions: getPermissions(role) };
}| Scenario | Recommended Pattern | Usage Conditions |
|---|---|---|
| Object Creation | Factory / Builder | Complex creation logic, multiple types |
| Behavior Extension | Strategy / Template | Replaceable algorithms, variable process steps |
| Structural Adaptation | Facade / Adapter | Simplify interfaces, adapt third-party systems |
| State Management | State / Observer | State-driven, event notifications |
| Issue | Metric | Refactoring Direction |
|---|---|---|
| Long function | > 50 lines | Extract functions |
| Too many parameters | > 5 parameters | Parameter object |
| Excessive nesting | > 3 levels | Early return, extract functions |
| Duplicate code | 3+ occurrences | Extract common logic |
| God class | > 500 lines | Split responsibilities |
| Issue | Description | Refactoring Direction |
|---|---|---|
| Feature envy | Frequent access to other class data | Move method |
| Data clumps | Multiple parameters always appear together | Extract class |
| Shotgun surgery | A single change affects multiple places | Centralize logic |
| Parallel inheritance | Adding a subclass requires adding a matching class | Merge hierarchies |
1. Ensure test coverage
│
├── Has tests → Proceed
└── No tests → Add tests first
│
▼
2. Small-step refactoring
│
├── Only change one thing at a time
├── Ensure code runs after each step
└── Run tests after each step
│
▼
3. Verify unchanged behavior
│
├── All tests pass
└── Manually verify critical paths
│
▼
4. Commit code| Risk Level | Conditions | Strategy |
|---|---|---|
| Low | Complete test coverage, localized changes | Refactor directly |
| Medium | Partial test coverage, affects multiple places | Add tests first |
| High | No test coverage, core logic | Gradually add tests before refactoring |
| Level | Tag | Meaning | Requirements |
|---|---|---|---|
| Blocker | | Must be fixed before merging | Security issues, logical errors |
| Suggestion | | Recommended change but not blocking | Code style, minor optimizations |
| Question | | Requires explanation from author | Unclear design decisions |
| Praise | | Well-written parts | Encourage good practices |