Code Review Skill — систематическое ревью кода

Методология глубокого и структурированного code review с акцентом на безопасность, производительность и поддерживаемость.

Когда использовать

• Review pull request или изменений
• Оценка качества существующего кода
• Pre-merge проверка
• Аудит безопасности
• Проверка соответствия стандартам

Методология Code Review

Структура Review

Code review проходит в 4 этапа:

1. Quick Scan — первый взгляд (2-5 мин)
2. Deep Analysis — глубокий анализ (15-30 мин)
3. Security & Performance — проверка критичных аспектов (10-15 мин)
4. Recommendations — выводы и рекомендации (5 мин)

Этап 1: Quick Scan (Быстрый взгляд)

Цель: Получить общее впечатление от изменений

Что проверяем

✅ Scope изменений

• Сколько файлов изменено?
• Какой объём кода (строки добавлены/удалены)?
• Соответствует ли scope заявленной задаче?

✅ Структура

• Логичная ли организация файлов?
• Нет ли смешанных concerns?
• Правильное ли место для нового кода?

✅ Первое впечатление

• Код читаемый?
• Naming понятный?
• Есть ли очевидные проблемы?

Красные флаги на Quick Scan

🚩 STOP и спроси:

• Изменено >500 строк в одном PR (слишком большой)
• Изменения в несвязанных модулях (scope creep)
• Отсутствуют тесты для новой логики
• Commented-out код или debug statements

Вывод Quick Scan:

## Quick Scan

**Scope:** [Описание изменений]
**Файлов изменено:** N
**Строк:** +XXX / -YYY

**Первое впечатление:**
- ✅ [Что хорошо]
- ⚠️ [Что требует внимания]
- 🚩 [Критичные проблемы]

Этап 2: Deep Analysis (Глубокий анализ)

Цель: Детально понять логику и качество кода

Чеклист Deep Analysis

📖 Читаемость (Readability)

• Naming
  • Переменные названы осмысленно?
  • Функции отражают что делают?
  • Константы выделены и названы правильно?
• Функции
  • Размер функций разумный (<50 строк)?
  • Каждая функция делает одну вещь?
  • Уровень абстракции единый?
• Комментарии
  • Код самодокументируемый?
  • Комментарии объясняют "почему", не "что"?
  • Нет устаревших/противоречивых комментариев?

🧱 Архитектура (Architecture)

• Separation of Concerns
  • Бизнес-логика отделена от UI?
  • Нет ли дублирования кода?
  • Правильное ли разделение на модули?
• Coupling & Cohesion
  • Модули слабо связаны (low coupling)?
  • Внутренняя связность высокая (high cohesion)?
  • Легко ли тестировать изолированно?
• SOLID принципы
  • Single Responsibility соблюдён?
  • Open/Closed — можно расширять без изменений?
  • Dependency Inversion — зависимости от абстракций?

🔄 Логика (Logic)

• Корректность
  • Логика работает правильно?
  • Обработаны edge cases?
  • Нет ли off-by-one errors?
• Error Handling
  • Все ошибки обработаны?
  • Правильные типы исключений?
  • Есть ли fallback стратегии?
• Null Safety
  • Проверки на null/undefined?
  • Optional chaining где нужно?
  • Defensive programming?

✅ Тестирование (Testing)

• Покрытие
  • Есть ли unit tests для новой логики?
  • Критичные paths покрыты?
  • Edge cases протестированы?
• Качество тестов
  • Тесты независимые?
  • Понятные test names?
  • Arrange-Act-Assert структура?

Вывод Deep Analysis:

## Deep Analysis

### Читаемость: ⭐⭐⭐⭐☆ (4/5)
- ✅ Naming понятный
- ⚠️ Функция `processData` слишком большая (80 строк)

### Архитектура: ⭐⭐⭐☆☆ (3/5)
- ✅ Separation of concerns соблюдён
- ❌ Дублирование логики в файлах X и Y

### Логика: ⭐⭐⭐⭐⭐ (5/5)
- ✅ Корректная обработка ошибок
- ✅ Edge cases учтены

### Тестирование: ⭐⭐☆☆☆ (2/5)
- ❌ Отсутствуют unit tests для новых функций
- ⚠️ Integration test только happy path

Этап 3: Security & Performance

Цель: Проверить критичные аспекты безопасности и производительности

🔒 Security Checklist

Input Validation

• Валидация всех входных данных?
• Sanitization перед использованием?
• Type checking на границах?

Authentication & Authorization

• Проверка прав доступа?
• Токены безопасно хранятся?
• Нет hardcoded credentials?

Common Vulnerabilities

• SQL Injection — используются prepared statements?
• XSS — экранирование пользовательского ввода?
• CSRF — защита от CSRF атак?
• Path Traversal — проверка путей файлов?
• DoS — лимиты на ресурсы (rate limiting)?

Data Protection

• Чувствительные данные не логируются?
• Пароли хешируются (bcrypt/argon2)?
• HTTPS для передачи данных?

⚡ Performance Checklist

Algorithms & Data Structures

• Сложность алгоритмов приемлема (O(n) vs O(n²))?
• Правильные структуры данных (Map vs Array)?
• Нет ли ненужных вложенных циклов?

Database

• Запросы оптимизированы?
• Используются индексы?
• Нет N+1 проблемы?
• Pagination для больших списков?

Memory & Resources

• Нет утечек памяти?
• Ресурсы освобождаются (close connections)?
• Нет ли избыточного копирования данных?

Network & I/O

• Минимизированы сетевые запросы?
• Используется кеширование?
• Асинхронные операции где возможно?

Вывод Security & Performance:

## Security & Performance

### 🔒 Security: ⚠️ ISSUES FOUND
- ❌ **CRITICAL:** SQL запрос без параметризации (`db.query(${userId})`)
  Файл: `src/users/repository.ts:42`
  Риск: SQL Injection
  Fix: Использовать prepared statements

- ⚠️ **WARNING:** Пароль логируется в debug mode
  Файл: `src/auth/login.ts:15`
  Риск: Утечка credentials
  Fix: Убрать из логов

### ⚡ Performance: ⚠️ NEEDS IMPROVEMENT
- ⚠️ N+1 проблема при загрузке связанных данных
  Файл: `src/posts/service.ts:67`
  Impact: Медленные запросы при >100 записей
  Fix: Использовать eager loading или dataloader

Этап 4: Recommendations (Рекомендации)

Цель: Дать конструктивную обратную связь

Категории комментариев

Используй префиксы для приоритизации:

• 🚨 BLOCKER — нельзя мержить без исправления
• ❌ CRITICAL — нужно исправить (security, bug, breaking)
• ⚠️ MAJOR — серьёзная проблема (performance, architecture)
• 💡 SUGGESTION — улучшение (refactoring, best practice)
• 📝 NITPICK — мелочь (style, naming)
• ✅ POSITIVE — отметь хорошие решения

Структура комментария

### [Категория] [Файл:строка] — [Краткое описание]

**Проблема:**
[Что не так]

**Почему это важно:**
[Impact или риск]

**Как исправить:**
[Конкретное решение или пример кода]

**Пример:**
```language
// ❌ Текущий код
bad code here

// ✅ Предлагаемый вариант
good code here

### Итоговый отчёт

```markdown
# Code Review Summary

## Overall Rating: ⭐⭐⭐☆☆ (3/5)

**Можно мержить:** ❌ НЕТ (есть блокеры)

---

## 🚨 Blockers (2)
1. [SQL Injection в user repository](файл:строка)
2. [Missing authentication check](файл:строка)

## ❌ Critical Issues (1)
1. [Password logging in debug mode](файл:строка)

## ⚠️ Major Issues (3)
1. [N+1 query problem](файл:строка)
2. [Code duplication across modules](файл:строка)
3. [Missing unit tests](файл:строка)

## 💡 Suggestions (5)
1. Refactor large function into smaller pieces
2. Consider using custom hook for state management
3. Add TypeScript strict mode
4. Extract magic numbers to constants
5. Improve error messages for better UX

## ✅ What went well
- Clean separation of concerns
- Good naming conventions
- Comprehensive integration tests
- Well-documented complex logic

---

## Action Items

**Before merge:**
- [ ] Fix SQL injection vulnerability
- [ ] Add authentication middleware
- [ ] Remove password from logs

**After merge (create tasks):**
- [ ] Add unit tests for new service methods
- [ ] Refactor duplicated code into shared utility
- [ ] Optimize database queries (N+1 problem)

**Future improvements:**
- [ ] Consider migration to TypeScript strict mode
- [ ] Extract business logic into domain services

Инструменты Code Review

Поиск паттернов проблем

# Поиск SQL injection рисков
rg "\.query\(.*\$\{" --type ts

# Поиск захардкоженных credentials
rg -i "password.*=.*['\"]" --type js

# Поиск console.log/debug statements
rg "console\.(log|debug|warn)" --type js

# Поиск TODO/FIXME
rg "TODO|FIXME|HACK|XXX" -i

# Поиск commented out code
rg "^\\s*//.*;" --type js

Анализ тестов

# Найти файлы без тестов
# (сравнить src/ с tests/)

# Проверить покрытие
npm run test:coverage

# Найти тесты для конкретного файла
rg "describe.*MyComponent|test.*myFunction"

Best Practices Code Review

DO ✅

1. Будь конструктивным
   • Объясняй "почему", не только "что"
   • Предлагай конкретные решения
   • Отмечай хорошие решения
2. Будь объективным
   • Фокусируйся на коде, не на человеке
   • Используй факты и примеры
   • Ссылайся на стандарты проекта
3. Приоритизируй
   • Сначала critical issues
   • Потом major improvements
   • Nitpicks в конце
4. Используй примеры
   • Покажи как исправить
   • Дай ссылки на документацию
   • Укажи похожие места в кодовой базе

DON'T ❌

1. Не будь субъективным
   • ❌ "Мне не нравится этот код"
   • ✅ "Этот код нарушает принцип SRP"
2. Не перфекционизируй
   • Мелкие стилистические правки — низкий приоритет
   • Не блокируй PR из-за nitpicks
   • Лучше done чем perfect
3. Не игнорируй context
   • Учитывай deadlines
   • Понимай constraints проекта
   • Помни о technical debt trade-offs

Шаблоны запросов

Полный review:

Проревьюй этот код, проверь security и performance

Фокусированный review:

Проверь только безопасность этого SQL query

Pre-merge check:

Сделай quick review перед merge в main

Review изменений:

Проанализируй изменения в этом PR

Интеграция с другими skills

• Перед review → используй research для понимания контекста
• После review → используй debugging если нашёл баги
• После review → предложи refactoring для улучшений
• Результаты review → сохрани в комментарии к PR или в Todoist