Как проходит Code Review на текущем месте работы?

Code Review процесс на месте работы

Code review — это не только инструмент контроля качества, но и основной способ знаний обмена и развития в команде. Расскажу о нашем процессе.

1. Основной workflow

Шаг 1: Разработка

# Создаем feature ветку
git checkout -b feature/add-payment-gateway

# Пишем код с тестами (TDD)
# 1. Тесты (RED)
# 2. Реализация (GREEN)
# 3. Рефакторинг (REFACTOR)

git add .
git commit -m "feat: add payment gateway integration"

Шаг 2: Pull Request

# Title
Add payment gateway integration

## Description
Implements Stripe payment processing for subscriptions.

## Changes
- Created PaymentService for processing
- Added Stripe webhook handlers
- Implemented transaction logging
- Added comprehensive tests (92% coverage)

## Testing
- Unit tests: 45 tests pass
- Integration tests: 12 tests pass
- Manual testing: Tested with Stripe test API

## Checklist
- [x] Tests added/updated
- [x] Documentation updated
- [x] No breaking changes
- [x] Passes linting
- [x] Performance impact assessed

2. Процесс Review

Автоматические проверки (CI pipeline):

# .github/workflows/ci.yml
name: CI

on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
        with:
          node-version: '18'
      - run: npm install
      - run: npm run lint      # ESLint + Prettier
      - run: npm run type-check  # TypeScript check

  test:
    runs-on: ubuntu-latest
    steps:
      - run: npm run test -- --coverage  # Jest with coverage
      - name: Check coverage
        if: ${{ github.event_name == 'pull_request' }}
        run: |
          coverage=$(npm run test:coverage | grep 'Lines')
          if [ "$coverage" -lt 90 ]; then
            echo "Coverage is below 90%"
            exit 1
          fi

  security:
    runs-on: ubuntu-latest
    steps:
      - run: npm audit  # Проверка уязвимостей
      - run: npm run security:check  # Sonarqube

Результат: Все PR должны пройти все проверки перед review.

3. Ручной Code Review

Требования для review:

• Минимум 2 одобрения от senior разработчиков
• Все комментарии должны быть решены
• CI/CD pipeline должен быть зеленым

Что мы смотрим:

1. Архитектура и дизайн

// Reviewer comment
❌ Плохо: 
// service.ts
const user = db.query(`SELECT * FROM users WHERE id = ${id}`);

✅ Хорошо:
// userRepository.ts
const user = await userRepository.findById(id);
// service.ts использует repository

2. Performance

// Reviewer comment
❌ Плохо (N+1 query):
const orders = await Order.find();
for (const order of orders) {
  const items = await Item.find({ orderId: order.id });  // Loop query!
}

✅ Хорошо:
const orders = await Order.find()
  .populate('items');  // One query with JOIN

3. Безопасность

// Reviewer comment
❌ Плохо (SQL injection risk):
const result = db.query(
  `SELECT * FROM users WHERE email = '${email}'`
);

✅ Хорошо:
const result = db.query(
  'SELECT * FROM users WHERE email = @email',
  { email }
);

4. Тесты

// Reviewer comment
❌ Плохо (нет тестов happy path):
// test.ts
test('should handle error', () => {
  expect(() => process()).toThrow();
});

✅ Хорошо (полное покрытие):
// test.ts
test('should process valid payment', async () => {
  const result = await processPayment(validData);
  expect(result.status).toBe('success');
});

test('should reject invalid payment', async () => {
  expect(() => processPayment(invalidData)).toThrow();
});

test('should retry on network error', async () => {
  // Mock retry logic
});

5. Code style и читаемость

// Reviewer comment
❌ Плохо (неявная логика):
const x = a && (b || c) ? d : e;

✅ Хорошо (явная логика):
const isUserActive = user.status === 'active';
const hasValidPermissions = user.role === 'admin';
const shouldProcess = isUserActive && hasValidPermissions;
const result = shouldProcess ? processRequest() : rejectRequest();

4. Комментарии в Review

Категории:

MUST (блокирующие)

⛔ MUST: SQL injection risk here. Use parameterized queries.

SHOULD (рекомендации)

⚠️ SHOULD: Consider using memoization for better performance.

NICE TO HAVE (предложения)

💡 NICE TO HAVE: You could extract this logic to a helper function.

QUESTION (уточнения)

❓ QUESTION: Why did you choose Map instead of Object here?

5. Типичный диалог в PR

Reviewer: 
⛔ MUST: This N+1 query will cause performance issues.
See line 45-48.

Author:
You're right. I'll use populate() instead.

Reviewer:
✅ LGTM! Just one more thing:
⚠️ SHOULD: Add test case for edge case when items are empty.

Author:
Added test for empty items. Ready to merge?

Reviewer:
✅ APPROVED

6. Review guidelines

Как быть хорошим reviewer:

// ❌ Плохой review (слишком критичный)
"Это ужасно, полностью переделай"

// ✅ Хороший review (конструктивный)
"Я вижу твой подход. Есть потенциальная проблема с N+1 запросами.
Вот пример как это можно оптимизировать: [ссылка на код].
Что ты думаешь?"

Что мы ценим:

• Вежливый тон
• Конкретные примеры
• Объяснение WHY, а не только что поправить
• Признание хорошего кода (👍 reaction)

7. Типовые issues

Проблема: Слишком медленное review

Процесс:
- Первое review: 4 часа (SLA)
- Second opinion: 8 часов
- Обсуждение comments: до 1 часа
- Merge: 12 часов в среднем

Решение:

• Review queue в Slack
• Приоритет для блокирующих PR
• Async обсуждение в комментариях

Проблема: Большие PR трудно review

❌ PR с 2000 строк кода
✅ PR с 200-300 строк кода

Решение:

• Max 400 строк per PR
• Разбиваем большие фичи на несколько PR

8. Автоматизация Review

// GitHub Actions auto-review bot
name: Auto Review

on: [pull_request]

jobs:
  auto_review:
    runs-on: ubuntu-latest
    steps:
      - name: Check for common issues
        uses: dorny/paths-filter@v2
        with:
          filters: |
            security:
              - '**/.env*'
              - '**/secrets/*'
            database:
              - '**/migrations/*'
            package:
              - 'package.json'
              - 'package-lock.json'
              
      - name: Warn on security files
        if: ${{ steps.filter.outputs.security == 'true' }}
        uses: actions/github-script@v6
        with:
          script: |
            github.rest.issues.createComment({
              issue_number: context.issue.number,
              owner: context.repo.owner,
              repo: context.repo.repo,
              body: '⚠️ This PR modifies security-sensitive files. Extra review required.'
            })

9. Lessons Learned

Что работает:

1. Четкие критерии для MUST vs SHOULD
2. Быстрый turnaround (SLA 4 часа)
3. Автоматизация стиля (linting, formatting)
4. Обучение через review (junior developers растут)
5. Positivity в комментариях

Что не работает:

1. Микромеджмент в комментариях
2. Delay в review (больше суток)
3. Необоснованные требования
4. Отсутствие четких стандартов

10. Metrics

Мы отслеживаем:
- Average review time: 8 часов (target < 12)
- Number of iterations: 2 (target = 1-2)
- Defect escape rate: 0.8% (target < 1%)
- Code coverage: 91% (target > 90%)
- Developer satisfaction: 4.2/5

Code review — это не просто проверка, это инвестиция в качество и в развитие команды. Хороший process экономит часы на debug и делает код более надежным.