Какие знаешь принципы при проведении Code Review?

Какие знаешь принципы при проведении Code Review?

Code Review — это один из самых важных процессов в разработке. Это не просто проверка ошибок, это обмен знаниями, поддержание качества и защита от регрессий. Давайте разберём принципы, которые я использую.

1. Принцип конструктивности (Constructive Feedback)

Плохо:

❌ "Этот код выглядит ужасно, переписывай"
❌ "Ты не понимаешь Java"
❌ "Это ошибка новичка"

Хорошо:

✅ "Можно ли использовать Stream API вместо цикла? Это будет чище:"
✅ "Я заметил, что здесь может быть N+1 problem. Давай обсудим оптимизацию"
✅ "Эта логика сложная, может быть, выделить её в отдельный метод для читаемости?"

Применение:

• Предлагай варианты, а не только критику
• Объясняй ЧЕМ и ПОЧЕМУ лучше
• Признавай хорошие решения

2. Принцип фокуса на коде, а не на человеке

Плохо:

❌ "Ты всегда пишешь медленный код"
❌ "Это типично для тебя"
❌ "Ты должен был знать"

Хорошо:

✅ "В этом месте можно улучшить производительность. Текущая сложность O(n²)"
✅ "Этот паттерн может привести к ошибкам. Лучше использовать..."
✅ "Может показаться непонятным без документации, добавим комментарий?"

Применение:

• Критикуй решение, а не решающего
• Фокусируйся на коде, а не на человеке
• Предполагай добрые намерения

3. Принцип объективности (Code > Opinion)

Плохо:

❌ "Мне не нравятся длинные переменные"
❌ "По моему мнению, это неправильно"

Хорошо:

✅ "Это нарушает стиль guide нашего проекта (см. ссылку)"
✅ "Согласно SOLID принципу Single Responsibility, это лучше разделить"
✅ "Это может вызвать проблемы с потокобезопасностью в многопоточном контексте"

Применение:

• Используй стандарты (Style Guide, Architecture Decision Records)
• Ссылайся на принципы SOLID, Clean Code
• Покажи реальные последствия, а не вкус

4. Принцип приоритизации (Know What Matters)

Критические (MUST HAVE):

// 1. Безопасность
❌ SQL Injection:
sql = "SELECT * FROM users WHERE id = " + userId;

✅ Правильно:
sql = "SELECT * FROM users WHERE id = ?";  // parameterized query

// 2. Корректность
if (list.size() > 0) {
    firstElement = list.get(0);
} else {
    firstElement = null;  // Лучше: list.isEmpty() или Optional
}

// 3. Производительность (если есть проблема)
for (User user : users) {  // O(n)
    for (Order order : orders) {  // O(m) → O(n*m)
        if (user.id == order.userId) { ... }
    }
}
// Лучше: Map<Integer, List<Order>> ordersByUserId = ...

Важные (SHOULD HAVE):

// 4. Читаемость
int x = a + b * c;

// Лучше:
int totalPrice = basePrice + (itemCount * unitPrice);

// 5. Тестируемость
public class UserService {
    // Плохо: зависимость захардкодена
    private final Database db = new Database();
    
    // Хорошо: инъекция зависимостей
    public UserService(Database db) {
        this.db = db;
    }
}

// 6. Соответствие архитектуре
// ✅ Repository паттерн для доступа к данным
// ❌ SQL запросы напрямую в Service

Мелочи (NICE TO HAVE):

// 7. Стиль кода (скобки, отступы, именование)
varName    ✓ camelCase для переменных
VARNAME    ✗ Используется только для констант

// 8. Комментарии
// Если код сложный, добавь комментарий
// Если очевидный, комментарий не нужен

5. Принцип обучаемости (Teach, Don't Judge)

Плохо:

❌ Просто минусуешь PR без объяснения

Хорошо:

✅ "Это хорошее решение! Кстати, есть Stream API который может сделать это чище:
   
   // Вместо этого:
   List<String> names = new ArrayList<>();
   for (User user : users) {
       names.add(user.getName());
   }
   
   // Можно написать:
   List<String> names = users.stream()
       .map(User::getName)
       .collect(Collectors.toList());
   
   Для следующего PR это хорошо помнить!"

Применение:

• Объясняй, почему одно решение лучше другого
• Предлагай ссылки на документацию
• Помни, что не все знают всё

6. Принцип своевременности (Timely Feedback)

Тайм-код review:

✅ < 1 часа        : Невероятно быстро
✅ 1-4 часа        : Хорошо
⚠️  4-24 часа      : Приемлемо
❌ > 24 часов      : Блокирует разработчика

Применение:

• Не забывай про review
• Если нет времени, лучше сказать, чем молчать
• Срочные PR/hotfixes — приоритет

7. Принцип автоматизации (Automate What You Can)

// Не пиши в review:
// ❌ "Здесь неправильный отступ"
// ❌ "Переменная не следует нашему стилю"

// Настрой инструменты:

# .github/workflows/lint.yml
name: Code Quality

on: [pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - run: ./gradlew checkstyleMain  # Автоматические checks
      - run: ./gradlew spotbugs         # Потенциальные баги
      - run: ./gradlew test             # Unit тесты

Применение:

• Используй SonarQube, SpotBugs, Checkstyle
• Настрой GitHub Actions/GitLab CI
• Review сосредоточься на логике, не на форматировании

8. Принцип контекста (Understand the Context)

Не критикуй в вакууме!

❌ "Почему используешь if, а не switch?"

✅ Контекст:
   - Это legacy code, который поддерживается годами?
   - Это новый код, который должен следовать современным паттернам?
   - Это горячий fix, где качество менее критично?
   - Это performance-critical код?

Применение:

• Знай историю проекта
• Понимай constraints (deadline, legacy systems)
• Адаптируй требования к контексту

9. Принцип согласия (Agree on Standards)

Перед первым review нужно договориться:

1. Architecture Decision Records (ADR)
   - Как мы организуем код?
   - Какой паттерн используем?
   - На какой версии Java/Spring?

2. Code Style Guide
   - Нейминг конвенции
   - Структура проекта
   - Обработка ошибок

3. Definition of Done
   - Нужны ли тесты?
   - Какой coverage?
   - Нужна ли документация?
   - Performance requirements?

Применение:

• Документируй стандарты
• Все должны их знать
• Не придумывай новые требования на ходу

10. Принцип баланса (Balance Speed and Quality)

Спектр подходов:

⚡ AGILE (быстро):
Approving review за 15 минут → Много багов в production

🐢 WATERFALL (медленно):
Review 3 дня → Никто не может работать, моральный ущерб

⚖️ BALANCED:
Важные вещи → тщательный review (безопасность, архитектура)
Мелочи → быстрое одобрение

Применение:

• Не задерживай PR без причины
• Не одобряй PR со слепыми глазами
• Найди баланс

Практический пример: Code Review session

// Разработчик предлагает:
public class UserService {
    private final UserRepository repo;
    
    public UserService(UserRepository repo) {
        this.repo = repo;
    }
    
    public List<User> getActiveUsers() {
        List<User> result = new ArrayList<>();
        for (User user : repo.findAll()) {
            if (user.isActive()) {
                result.add(user);
            }
        }
        return result;
    }
}

Мой review (хороший):

✅ Плюсы:
- Инъекция зависимостей правильная
- Методы явно названы

💡 Предложение к улучшению:
Можно ли переместить фильтрацию в БД? Сейчас загружаются ВСЕ пользователи.

// Вариант 1: В repository
public class UserRepository {
    public List<User> findAllActive() {
        // SELECT * FROM users WHERE is_active = true
    }
}

// Вариант 2: Stream API
public List<User> getActiveUsers() {
    return repo.findAll().stream()
        .filter(User::isActive)
        .collect(Collectors.toList());
}

Что вы думаете?

Критерии хорошей code review:

1. ✅ Указаны конкретные проблемы, а не общие замечания
2. ✅ Предложены решения или варианты
3. ✅ Объяснены причины
4. ✅ Признаны хорошие решения
5. ✅ Тон конструктивный и добрый
6. ✅ Не блокирует без веского повода
7. ✅ Фокус на безопасность, производительность и читаемость

Code Review — это не инструмент контроля, а инструмент для совместного развития команды и качества кода!