Какой у вас опыт проведения код-ревью и на что вы обращаете внимание?

Ответ

Регулярно провожу и принимаю участие в код-ревью. Фокус — на качестве, поддерживаемости и предотвращении регрессий.

Ключевые аспекты проверки:

  • Читаемость и ясность:
    • Осмысленные имена переменных, функций, соблюдение code style.
    • Отсутствие избыточной сложности (цикломатическая сложность).
    • Наличие комментариев только для объяснения "почему", а не "что".
  • Архитектура и дизайн:
    • Соблюдение выбранной архитектуры (MVVM, Clean Swift и т.д.).
    • Принципы SOLID, DRY, низкая связанность модулей.
  • Корректность и безопасность:
    • Полная обработка ошибок и опциональных состояний.
    • Отсутствие утечек памяти (сильные ссылочные циклы в Swift).
    • Потокобезопасность при работе с общими ресурсами.
  • Производительность:
    • Эффективные алгоритмы и структуры данных.
    • Избегание избыточных операций (например, в циклах).

Пример конструктивного комментария в ревью:

// Было: Сетевая загрузка без обработки ошибок и сильного захвата self.
func fetchData() {
    API.getData { [weak self] data in
        self?.data = data
        self?.tableView.reloadData()
    }
}

// Комментарий ревьюера:
// 1. Добавь обработку возможных ошибок (Result<Data, Error>).
// 2. Вынеси обновление UI в отдельный метод (e.g., `updateUI(with:)`) для лучшей читаемости.
// 3. Для гарантированного выполнения UI-обновлений на главном потоке используй `DispatchQueue.main.async` или аннотацию `@MainActor`.

Ответ 18+ 🔞

А, вот ты про код-ревью заговорил! Ну это ж святое, блядь, без этого вообще никуда. Сидишь, смотришь на чужой код, как на картину в галерее, только вместо красок — функции, а вместо кисти — клавиатура. И иногда такое увидишь, что волосы дыбом, ёпта, и думаешь: «Мужик, ты это серьёзно написал или просто тестил, выдержит ли мой монитор такой пиздец?»

Так вот, на что я там обычно пялюсь, пока кофе остывает:

На что смотрю, пока не ослеп:

  • Читается или как китайская грамота:

    • Имена переменных. Если вижу a, b, c или temp1, temp2 — сразу подозрение ёбать чувствую. Это ж не черновик на салфетке, блядь. userProfile или cachedImageData — вот это дело. И да, стиль кода чтоб был единый, а то один пишет с отступами в 2 пробела, другой — в 4, третий вообще табуляцией — пиздец, а не проект.
    • Сложность. Если функция похожа на лабиринт Минотавра и в ней дохуя вложенных if — это повод задуматься. Может, её на несколько мелких разбить?
    • Комментарии. Если они объясняют, что делает x += 1 — это мусор. А вот если поясняют, почему тут такой ебанутый костыль из-за кривого API бэкенда — это золото. «Почему» — важно, «что» — и так видно.
  • Архитектура, или «А где тут у тебя логика, чувак?»:

    • Чтоб всё по полочкам лежало. Если у тебя ViewController знает про базу данных, сетевые запросы и ещё парсит JSON — это не архитектура, это помойка, блядь. MVVM там, VIPER, Clean — что угодно, главное, чтоб не было этой спагетти-каши.
    • Принципы эти, SOLID, DRY. Повторяющийся код — зло. Класс, который делает всё на свете — тоже зло. Низкая связанность — это когда можно один модуль выкинуть и заменить другим, а всё остальное не развалится.
  • Не упадёт ли всё к херам:

    • Ошибки. Если их просто игноришь или пишешь print("error") — это путь в никуда. Что делать, если сеть легла? Если файл не найден? Если сервер прислал хуйню вместо JSON? На всё должен быть план, блядь.
    • Память. Особенно в Swift с этими циклами сильных ссылок. [weak self] — наш друг, а не враг. Утечки — они как грибы: незаметно появляются и потом весь проект гниёт.
    • Потоки. Обновляешь UI не из главного потока? Приготовься к крашу, ебать твою в сраку. DispatchQueue.main — мать твою, не забывай.
  • Не тормозит ли как черепаха в сиропе:

    • Алгоритмы. Если для поиска элемента в массиве из 10 элементов ты используешь двойной цикл — ладно, пронесёт. А если массив на 10 тысяч элементов? Пиздец, пользователь успеет сходить за кофе.
    • Лишние операции. Не надо в цикле на каждой итерации вычислять одно и то же константное значение. Вынеси наружу, экономь ресурсы, они не безграничные.

А вот пример, как я обычно на ревью пишу, когда вижу лажу:

// Было: Выглядит просто, но пахнет проблемами.
func fetchData() {
    API.getData { [weak self] data in
        self?.data = data
        self?.tableView.reloadData()
    }
}

// Мой комментарий будет примерно таким:
// Слушай, а что будет, если запрос ебнулся с ошибкой? Приложение просто зависнет в ожидании, которое никогда не наступит.
// И потом, `self?.tableView` — это слабая ссылка, но если `self` уже умер, то `reloadData()` просто не вызовется, и интерфейс не обновится, даже если данные пришли.
// Давай сделаем по-взрослому:
// 1. Обработаем ошибку (добавим в колбэк `Result<Data, Error>`).
// 2. Вынесем обновление UI в отдельный метод, который гарантированно запустится на главном потоке. Можно через `@MainActor` пометить, чтоб наверняка.
// 3. И да, `[weak self]` — это правильно, респект, что хоть это учёл.

Вот так, примерно. Главное — не просто тыкать пальцем и говорить «фигово», а объяснить, почему фигово и как сделать лучше. Чтобы человек не обиделся, а реально понял и в следующий раз не наступил на те же грабли. А то иначе какой смысл, блядь?