Code Review Analysis

代码审查分析

Overview

概述

Systematic code review process covering code quality, security, performance, maintainability, and best practices following industry standards.

遵循行业标准，涵盖代码质量、安全性、性能、可维护性及最佳实践的系统化代码审查流程。

When to Use

适用场景

Reviewing pull requests and merge requests
Analyzing code quality before merging
Identifying security vulnerabilities
Providing constructive feedback to developers
Ensuring coding standards compliance
Mentoring through code review

审查pull requests和merge requests
合并前分析代码质量
识别安全漏洞
为开发者提供建设性反馈
确保符合编码标准
通过代码审查提供指导

Instructions

操作步骤

1. Initial Assessment

1. 初始评估

bash

undefined

bash

undefined

Check the changes

git diff main...feature-branch

Review file changes

git diff --stat main...feature-branch

Check commit history

git log main...feature-branch --oneline


**Quick Checklist:**
- [ ] PR description is clear and complete
- [ ] Changes match the stated purpose
- [ ] No unrelated changes included
- [ ] Tests are included
- [ ] Documentation is updated

git log main...feature-branch --oneline


**快速检查清单：**
- [ ] PR描述清晰完整
- [ ] 变更内容与声明的目的一致
- [ ] 未包含无关变更
- [ ] 包含测试代码
- [ ] 文档已更新

2. Code Quality Analysis

2. 代码质量分析

Readability

可读性

python

undefined

python

undefined

❌ Poor readability

def p(u,o): return u['t']*o['q'] if u['s']=='a' else 0

✅ Good readability

def calculate_order_total(user: User, order: Order) -> float: """Calculate order total with user-specific pricing.""" if user.status == 'active': return user.tier_price * order.quantity return 0

undefined

def calculate_order_total(user: User, order: Order) -> float: """Calculate order total with user-specific pricing.""" if user.status == 'active': return user.tier_price * order.quantity return 0

undefined

Complexity

复杂度

javascript

// ❌ High cognitive complexity
function processData(data) {
  if (data) {
    if (data.type === 'user') {
      if (data.status === 'active') {
        if (data.permissions && data.permissions.length > 0) {
          // deeply nested logic
        }
      }
    }
  }
}

// ✅ Reduced complexity with early returns
function processData(data) {
  if (!data) return null;
  if (data.type !== 'user') return null;
  if (data.status !== 'active') return null;
  if (!data.permissions?.length) return null;

  // main logic at top level
}

javascript

// ❌ High cognitive complexity
function processData(data) {
  if (data) {
    if (data.type === 'user') {
      if (data.status === 'active') {
        if (data.permissions && data.permissions.length > 0) {
          // deeply nested logic
        }
      }
    }
  }
}

// ✅ Reduced complexity with early returns
function processData(data) {
  if (!data) return null;
  if (data.type !== 'user') return null;
  if (data.status !== 'active') return null;
  if (!data.permissions?.length) return null;

  // main logic at top level
}

3. Security Review

3. 安全审查

Common Vulnerabilities

常见漏洞

SQL Injection

python

undefined

SQL注入

python

undefined

❌ Vulnerable to SQL injection

query = f"SELECT * FROM users WHERE email = '{user_email}'"

✅ Parameterized query

query = "SELECT * FROM users WHERE email = ?" cursor.execute(query, (user_email,))


**XSS Prevention**
```javascript
// ❌ XSS vulnerable
element.innerHTML = userInput;

// ✅ Safe rendering
element.textContent = userInput;
// or use framework escaping: {{ userInput }} in templates

Authentication & Authorization

typescript

// ❌ Missing authorization check
app.delete('/api/users/:id', async (req, res) => {
  await deleteUser(req.params.id);
  res.json({ success: true });
});

// ✅ Proper authorization
app.delete('/api/users/:id', requireAuth, async (req, res) => {
  if (req.user.id !== req.params.id && !req.user.isAdmin) {
    return res.status(403).json({ error: 'Forbidden' });
  }
  await deleteUser(req.params.id);
  res.json({ success: true });
});

query = "SELECT * FROM users WHERE email = ?" cursor.execute(query, (user_email,))


**XSS防护**
```javascript
// ❌ XSS vulnerable
element.innerHTML = userInput;

// ✅ Safe rendering
element.textContent = userInput;
// or use framework escaping: {{ userInput }} in templates

身份验证与授权

typescript

// ❌ Missing authorization check
app.delete('/api/users/:id', async (req, res) => {
  await deleteUser(req.params.id);
  res.json({ success: true });
});

// ✅ Proper authorization
app.delete('/api/users/:id', requireAuth, async (req, res) => {
  if (req.user.id !== req.params.id && !req.user.isAdmin) {
    return res.status(403).json({ error: 'Forbidden' });
  }
  await deleteUser(req.params.id);
  res.json({ success: true });
});

4. Performance Review

4. 性能审查

javascript

// ❌ N+1 query problem
const users = await User.findAll();
for (const user of users) {
  user.orders = await Order.findAll({ where: { userId: user.id } });
}

// ✅ Eager loading
const users = await User.findAll({
  include: [{ model: Order }]
});

python

undefined

javascript

// ❌ N+1 query problem
const users = await User.findAll();
for (const user of users) {
  user.orders = await Order.findAll({ where: { userId: user.id } });
}

// ✅ Eager loading
const users = await User.findAll({
  include: [{ model: Order }]
});

python

undefined

❌ Inefficient list operations

result = [] for item in large_list: if item % 2 == 0: result.append(item * 2)

✅ List comprehension

result = [item * 2 for item in large_list if item % 2 == 0]

undefined

result = [item * 2 for item in large_list if item % 2 == 0]

undefined

5. Testing Review

5. 测试审查

Test Coverage

javascript

describe('User Service', () => {
  // ✅ Tests edge cases
  it('should handle empty input', () => {
    expect(processUser(null)).toBeNull();
  });

  it('should handle invalid data', () => {
    expect(() => processUser({})).toThrow(ValidationError);
  });

  // ✅ Tests happy path
  it('should process valid user', () => {
    const result = processUser(validUserData);
    expect(result.id).toBeDefined();
  });
});

Check for:

Unit tests for new functions
Integration tests for new features
Edge cases covered
Error cases tested
Mock/stub usage is appropriate

测试覆盖率

javascript

describe('User Service', () => {
  // ✅ Tests edge cases
  it('should handle empty input', () => {
    expect(processUser(null)).toBeNull();
  });

  it('should handle invalid data', () => {
    expect(() => processUser({})).toThrow(ValidationError);
  });

  // ✅ Tests happy path
  it('should process valid user', () => {
    const result = processUser(validUserData);
    expect(result.id).toBeDefined();
  });
});

检查项：

为新函数添加单元测试
为新功能添加集成测试
覆盖边缘场景
测试错误场景
合理使用Mock/Stub

6. Best Practices

6. 最佳实践

Error Handling

错误处理

typescript

// ❌ Silent failures
try {
  await saveData(data);
} catch (e) {
  // empty catch
}

// ✅ Proper error handling
try {
  await saveData(data);
} catch (error) {
  logger.error('Failed to save data', { error, data });
  throw new DataSaveError('Could not save data', { cause: error });
}

typescript

// ❌ Silent failures
try {
  await saveData(data);
} catch (e) {
  // empty catch
}

// ✅ Proper error handling
try {
  await saveData(data);
} catch (error) {
  logger.error('Failed to save data', { error, data });
  throw new DataSaveError('Could not save data', { cause: error });
}

Resource Management

资源管理

python

undefined

python

undefined

❌ Resources not closed

file = open('data.txt') data = file.read() process(data)

✅ Proper cleanup

with open('data.txt') as file: data = file.read() process(data)

undefined

with open('data.txt') as file: data = file.read() process(data)

undefined

Review Feedback Template

审查反馈模板

markdown

undefined

markdown

undefined

Code Review: [PR Title]

代码审查: [PR标题]

Summary

摘要

Brief overview of changes and overall assessment.

变更内容及整体评估的简要概述。

✅ Strengths

✅ 优点

Well-structured error handling
Comprehensive test coverage
Clear documentation

错误处理结构清晰
测试覆盖率全面
文档表述清晰

🔍 Issues Found

🔍 发现的问题

🔴 Critical (Must Fix)

🔴 严重（必须修复）

Security: SQL injection vulnerability in user query (line 45)

python

# Current code
query = f"SELECT * FROM users WHERE id = '{user_id}'"

# Suggested fix
query = "SELECT * FROM users WHERE id = ?"
cursor.execute(query, (user_id,))

安全问题: 用户查询中存在SQL注入漏洞（第45行）

python

# 当前代码
query = f"SELECT * FROM users WHERE id = '{user_id}'"

# 建议修复方案
query = "SELECT * FROM users WHERE id = ?"
cursor.execute(query, (user_id,))

🟡 Moderate (Should Fix)

🟡 中等（应该修复）

Performance: N+1 query problem (lines 78-82)
- Suggest using eager loading to reduce database queries

性能问题: N+1查询问题（第78-82行）
- 建议使用预加载减少数据库查询次数

🟢 Minor (Consider)

🟢 轻微（可考虑）

Style: Consider extracting this function for better testability
Naming:
```
proc_data
```
could be more descriptive as
```
processUserData
```

代码风格: 考虑提取此函数以提升可测试性
命名规范:
```
proc_data
```
可更改为更具描述性的
```
processUserData
```

💡 Suggestions

💡 建议

Consider adding input validation
Could benefit from additional edge case tests
Documentation could include usage examples

考虑添加输入验证
可补充更多边缘场景测试
文档可包含使用示例

📋 Checklist

📋 检查清单

Security vulnerabilities addressed
Tests added and passing
Documentation updated
No console.log or debug statements
Error handling is appropriate

Verdict

结论

✅ Approved with minor suggestions | ⏸️ Needs changes | ❌ Needs major revision

undefined

✅ 通过，附带轻微建议 | ⏸️ 需要修改 | ❌ 需要重大修订

undefined

Common Issues Checklist

常见问题检查清单

Security

安全

No SQL injection vulnerabilities
XSS prevention in place
CSRF protection where needed
Authentication/authorization checks
No exposed secrets or credentials
Input validation implemented
Output encoding applied

Code Quality

代码质量

Performance

性能

Testing

测试

Maintainability

可维护性

Tools

工具

Linters: ESLint, Pylint, RuboCop
Security: Snyk, OWASP Dependency Check, Bandit
Code Quality: SonarQube, Code Climate
Coverage: Istanbul, Coverage.py
Static Analysis: TypeScript, Flow, mypy

代码检查器: ESLint, Pylint, RuboCop
安全工具: Snyk, OWASP Dependency Check, Bandit
代码质量工具: SonarQube, Code Climate
覆盖率工具: Istanbul, Coverage.py
静态分析工具: TypeScript, Flow, mypy

Best Practices

最佳实践

✅ DO

✅ 建议做法

Be constructive and respectful
Explain the "why" behind suggestions
Provide code examples
Ask questions if unclear
Acknowledge good practices
Focus on important issues
Consider the context
Offer to pair program on complex issues

保持建设性和尊重的态度
解释建议背后的原因
提供代码示例
若有疑问及时询问
认可良好的实践
关注重要问题
考虑上下文场景
针对复杂问题提供结对编程支持

❌ DON'T

❌ 避免做法

Be overly critical or personal
Nitpick minor style issues (use automated tools)
Block on subjective preferences
Review too many changes at once (>400 lines)
Forget to check tests
Ignore security implications
Rush the review

过于苛刻或人身攻击
挑剔 minor 风格问题（使用自动化工具处理）
因主观偏好阻碍审查
一次审查过多变更（超过400行）
忘记检查测试
忽略安全影响
仓促完成审查

Examples

示例

See the refactor-legacy-code skill for detailed refactoring examples that often apply during code review.

有关详细的重构示例，可参考refactor-legacy-code技能，这些示例通常适用于代码审查过程。

code-review-analysis

Original

Translation

Code Review Analysis

代码审查分析

Overview

概述

When to Use

适用场景

Instructions

操作步骤

1. Initial Assessment

1. 初始评估

Check the changes

Check the changes

Review file changes

Review file changes

Check commit history

Check commit history

2. Code Quality Analysis

2. 代码质量分析

Readability

可读性

❌ Poor readability

❌ Poor readability

✅ Good readability

✅ Good readability

Complexity

复杂度

3. Security Review

3. 安全审查

Common Vulnerabilities

常见漏洞

❌ Vulnerable to SQL injection

❌ Vulnerable to SQL injection

✅ Parameterized query

✅ Parameterized query

4. Performance Review

4. 性能审查

❌ Inefficient list operations

❌ Inefficient list operations

✅ List comprehension

✅ List comprehension

5. Testing Review

5. 测试审查

6. Best Practices

6. 最佳实践

Error Handling

错误处理

Resource Management

资源管理

❌ Resources not closed

❌ Resources not closed

✅ Proper cleanup

✅ Proper cleanup

Review Feedback Template

审查反馈模板

Code Review: [PR Title]

代码审查: [PR标题]

Summary

摘要

✅ Strengths

✅ 优点

🔍 Issues Found

🔍 发现的问题

🔴 Critical (Must Fix)

🔴 严重（必须修复）

🟡 Moderate (Should Fix)

🟡 中等（应该修复）

🟢 Minor (Consider)

🟢 轻微（可考虑）

💡 Suggestions

💡 建议

📋 Checklist

📋 检查清单

Verdict

结论

Common Issues Checklist

常见问题检查清单

Security