Code Review. 80 lvl +15



Любой программный продукт, будь то веб-сайт или мобильное приложение, основан на коде. Чем согласованнее и целостнее эта база, тем удобнее с ней будет работать, например, при необходимости доработки проекта, передачи на сопровождение другой команде.

Основными критериями качественного кода являются следующие: простота восприятия, гибкость для модификаций, возможность обновления, понятность, тестируемость. Однако зачастую работа над проектом ведется в спешке, под давлением и код пишется людьми с разным уровнем квалификации (с разным мышлением). И даже опытные разработчики не всегда пишут код самого высокого качества. Поэтому для повышения качества кода проводится процедура code review.

Code review - это процесс, в котором другие разработчики программного обеспечения проверяют код конкретного инженера с целью проверить его соответствие стандартам, выявить ошибки, выявить несоответствия в стиле кодирования и проверить соответствие написанного кода поставленным задачам, и в целом характеристикам качественного кода.

Правильно проведенный процесс code review дает несколько преимуществ, которые важны как в целом, так и по отдельности. Укажем некоторые из них:

  • Возможность исправить ошибки, которые могут вызвать проблемы в будущем;

  • Соблюдение стандартов и стиля кода в команде;

  • Улучшение архитектуры и дизайна решения;

  • Проверка соответствия кода поставленной задаче;

  • Повышение общего качества кода;

  • Повышение квалификации и обучение программистов.

Благодаря code review уменьшается так называемый bus factor, поскольку при передаче проекта другой команде или привлечении к работе другого человека им будет легче понять, как все работает.

Еще можно отметить психологический фактор, программист, который заранее знает, что его работа будет проверена коллегами, как правило, пишет аккуратный и структурированный код, и уже один этот факт повышает его качество.

Несмотря на все вспоминаемые плюсы, у code review есть и недостатки. Первый - это длительность процесса, потому что некоторым людям приходится тратить время на просмотр кода, а другим - на его исправление. Другой недостаток - повышенная стоимость реализации проекта, так как разработчикам приходится оплачивать время.

Но, тут важно понять одно, что минусы code review видны только в краткосрочной перспективе, но вклад процесса code review в долгую неоценим.

Давайте посмотрим, на что обычно смотрит разработчик при проведении code review.

Гигиенический минимум при код ревью
Гигиенический минимум при код ревью
  • Форматирование, стиль, наименование - Где находятся пробелы, переносы строк? Использованы табы или пробелы? Правильно ли расположены фигурные скобки? Как называются переменные? Правильно ли они объявлены? В каком месте объявлены?И т.д. То есть в целом проверяется стиль кода. Если в компании есть стандарт кода, то проверяется следование этому стандарту.

  • Покрытие тестами - Есть ли тесты для модифицированного или нового кода?

Это некий гигиенический минимум для code review. Но этого крайне мало, чтобы получить качественный код!

Есть еще минимум три (четыре) аспекта, на что ревьюер (ревьюеры при групповом ревью) должен обратить внимание:

  • Дизайн

  • Читаемость, гибкость и модифицируемость

  • Функциональность

На этапе ревью дизайна надо получить ответы на следующие вопросы.

  • Как новый код вписывается в общую архитектуру? Придерживался ли разработчик общего архитектурного стиля или написал что-то, что сильно выбивается?

  • Следует ли код дизайн парадигмам таким как SOLID, DDD, BDD, TDD или другим, которые приняты и используются в команде, компании при разработке продукта?

  • Какие дизайн паттерны используются? Насколько используемый дизайн паттерн в целом применим при решении задачи?

  • Находится ли новый код в логически правильном месте? Подходит ли он по контексту и смыслу той части кода, где он находится?

  • Мог ли новый код повторно использовать что-то из существующего кода? Предоставляет ли новый код что-то, что мы можем повторно использовать в существующем коде? Дублирует ли новый код существующий? Если да, то следует ли его отрефакторить на более используемый на будущее, или это приемлемо на данном этапе?

  • Не слишком ли код сложный (over-engineered)? Обеспечивает ли он возможность многократного использования, которого сейчас не требуется? Не нарушает ли новый код баланса повторного использования согласно принципам YAGNI?

С дизайном разобрались. Переходим к проверке читаемости, гибкости и модифицируемости кода.

  • Отражают ли имена (полей, переменных, параметров, методов и классов) то, что они представляют, под что они заводились?

  • Понимаю ли я, что делает код, прочитав его?

  • Понимаю ли я, что делают тесты?

  • Охватывают ли тесты достаточный набор кейсов? Учтены ли они все пути, альтернативные и исключительные случаи? Есть ли кейсы, которые не были рассмотрены?

  • Понятны ли сообщения об ошибках при обработке исключений? И обрабатываются ли исключения?

  • Есть ли документация или покрытие тестами для сложного и неодназначного кода?

Проверяя функциональную часть, ревьюер должен заострить внимание на следующих моментах.

  • Действительно ли код делает то, что должен был делать? Если есть автотесты для проверки правильности кода, действительно ли тесты проверяют соответствие кода согласованным требованиям?

  • Похоже ли, что код содержит небольшие ошибки, например, использование неправильной переменной для проверки или случайное использование и вместо или?

Рассмотрев код с этой точки зрения мы в итоге получим хороший код, готовый переходить к следующей стадии - тестированию функционала. Но есть еще пара моментов, на которые следует обратить внимание при проведении code review.

  • Есть ли потенциальные проблемы с безопасностью в коде? Создает ли код новые дыры в безопасности системы?

  • Есть ли нормативные требования, которые необходимо соблюдать? Не нарушил ли новый код нормативные требования, которые были приняты ранее? (К примеру, при прохождении сертификаций по безопасности)

  • Вносит ли новый код проблемы с производительностью в области, которые не охвачены автоматическими тестами по производительности? Есть ли в коде, например ненужные вызовы базы данных или удаленной сервисов? Можно ли избежать этих вызовов?

  • Нужна ли новая публичная документация по реализованной части? Нужно ли обновить существующую?

  • Проверялись ли сообщения, предназначенные для пользователей, на правильность, корректность? Являются ли эти сообщения user-friendly?

  • Есть ли очевидные ошибки, из-за которых код не будет работать в продакшене? Есть ли код, который случайно указывает на тестовую базу данных, или есть жестко запрограммированная заглушка, которую следует заменить на настоящую службу? и т.д.

Code review (инфографика)
Code review (инфографика)

Сode review - это процесс, который должен быть вшит в процесс разработки. Если есть возможность, то лучше делать так называемое групповое code review. Привлекать надо всю команду - от джунов до лидов (джуны будут познавать новый мир, лиды смотреть на код используя чек-лист code review). Без этого процесса построить качественный продукт очень сложно.




Комментарии (12):

  1. chemtech
    /#23784815 / +10

    Спасибо за пост.

    Code review - это процесс, в котором другие разработчики программного обеспечения проверяют код конкретного инженера с целью проверить его соответствие стандартам, выявить ошибки, выявить несоответствия в стиле кодирования и .... и в целом характеристикам качественного кода.

    Вот это не задача Code review. Это задача инструментов, которые проверяют код в автоматическом режите. Например: pre-commit, pre-receive, SonarQube, tslint, prettier и так далее.

    Несмотря на все вспоминаемые плюсы, у code review есть и недостатки. Первый - это длительность процесса, потому что некоторым людям приходится тратить время на просмотр кода, а другим - на его исправление. Другой недостаток - повышенная стоимость реализации проекта, так как разработчикам приходится оплачивать время.

    Автоматизируйте все что можно. Я написал в этом комментарии как и какими инструментами можно автоматизировать и можно сократить расход времени и денег.

    Покрытие тестами - Есть ли тесты для модифицированного или нового кода?

    Для этого есть инструменты. Ищутся так: Test Gap Analysis и Minimize Your Regression, например SonarQube.

    Есть ли потенциальные проблемы с безопасностью в коде? Создает ли код новые дыры в безопасности системы?

    Для этого есть инструменты SAST, DAST, semgrep и другие.

    Есть ли код, который случайно указывает на тестовую базу данных, 

    Тестировать бд нужно в Database Lab.

    Я посту поставил плюс, так как code review действительно нужно.

    • nin-jin
      /#23785749

      Вот это не задача Code review.

      Типичная ситуация: ищут проблемы не там, где они могут быть, а там, где их легко найти. Ну поставите вы пробел с другой стороны - что от этого изменится? Да ничего.

      • amarkevich
        /#23785889 / +1

        шум. хотелось бы ревьювить код, а не изменения форматирования (могут попадать в стиль, поэтому автоматически не отсеить)

        • nin-jin
          /#23786139 / -1

          Так не меняйте форматирование без надобности.

  2. ChiTechOff
    /#23785137

    Спасибо за комментарий и отличный список инструментов. Совершенно согласен, что автоматизировать нужно по максимуму.

  3. andreyiq
    /#23786119

    Получился какой-то code review в идеальном мире. Интересно как часто на все это действительно проверяют? Это сколько же нужно тратить время на ревью, чтобы полностью вникнуть в задачу и насколько хорошо код решает эту проблему, с учётом того что зачастую есть куча легаси в который лучше не лезть и поэтому приходиться городить костыли.

    Как написали выше, зачем проверять форматирование? Если это все хорошо решается автоматически

    Проверка на покрытия тестами? Вы 100% кода покрываете тестами? Зачем? Довольно большая часть кода этого не требует

    • ChiTechOff
      /#23786933

      Спасибо за комментарий. Смотрите, такой вопрос. А откуда появился такой легаси, в который даже страшно заходить? Как он разрабатывался и принимался? Проводятся ли процедуры рефакторинга и реструктуризации кода? Был ли ранее процесс code review и как его проводили?

      Возможно, раньше его не проводили. Всякое бывает. Разработка MVP, потом все начинает обрастать новым кодом и...добрый вечер... имеем не очень качественную кодовую базу. Как быть тут? Как команде вникать в задачу и понять тот код, который разработан? Тут в дело вступают процессы в команде. Как работают сеньоры, как они работают с джунами, какой процесс адаптации и обучения, какие еще практики применяются? Все это можно поэтапно вводить в команду. К примуру, практики экстремального программирования - совместное владение кодом, групповые ревью, парное программирование и т.д.

      Насчет форматирования... Это то, на что надо обратить внимание. Как вы это будете делать - это ваш выбор. Форматирование из IDE с принятыми настройками или какие нибудь автоматические инструменты. Не важно. Главное делать это и отлавливать. Код будет читаемым и его легко дальше передавать.

      100% покрытие тестами - это БАЦ (Большая амбициозная цель). Главное, чтобы тесты покрывали максимально возможные кейсы, которые рождаются из требований и критериев приемки. Можно запустить процесс, который в долгую приблизит ваш продукт(ы) к БАЦ.

      • andreyiq
        /#23787017

        Все видимо сильно зависит от компании. В кровавом энтерпрайз часто дикая конкуренция, нужно быть на шаг впереди, при этом ресурсы сильно ограничены. При этом программисты часто начитаются про всякие совершенные архитектуры, паттерны и что все нужно покрывать тестами и вообще код должен быть такой, что его хоть сейчас выложи на гитхаб и все скажут какой он прекрасный. И этим болеют не только джуны, но и синьоры. Нужно чётко понимать, что бизнес не продаем идеальный код, пока вы его будете вылизывать, конкуренты реализуют эту фичу работающую хоть как-то, но в два раза быстрее. Главная задача это баланс между говнищем которое не возможно поддерживать и супер идеальным кодом который уже не продашь. Мне в этом плане очень понравилась методология: Разработка через страдание. Смысл примерно следующее: сначала сделай чтобы было, потом чтобы было красиво, а потом чтобы было быстро.

        Про тесты и покрытие 100% это вообще адское зло, процентов 80% кода вообще не нуждается в тестах и даже вредно и такой код становится дорого поддерживать.

  4. Zored
    /#23786921 / +1

    Попробуйте применить ваш список правил к своему посту. Например, написать слитно "вдолгую" и "неоценим". И поставить запятую после "я не понимаю"! А после можете осознать, что все эти ошибки не влияют на суть вашего поста. И замечания вроде моих не привносят в процесс ничего кроме раздражения. Нет? Тогда давайте перепишем весь пост и разобъём его на абзацы немного иначе...

    Важно не только на что за замечания вы делаете, но и как и зачем вы их высказываете. И если не понимать этого, то можно потерять время и даже сотрудников.

    • ChiTechOff
      /#23786925 / +1

      Я вам скажу за ваше ревью спасибо. Да, опечатка не влияет на смысл поста. Но, согласитесь, куда приятнее читать пост без ошибок.Тут очень важна культура в команде, компании и под каким соусом подается код ревью. Для разработчика сode review - это не инструмент порицания, а инструмент оттачивания своего мастерства (обучения).

      • nin-jin
        /#23787665

        При чтении я эти ошибки тоже не заметил. Так что приятнее пост от их исправления не стал. А даже если бы и заметил - мне всё равно плевать. Я тексты ради заложенных в них мыслей читаю. И пока опечатки не ломают смысла - мозг их игнорирует.

  5. panzerfaust
    /#23790865

    Важный нюанс: никогда в команде не взлетит культура ревью, если нет культуры декомпозиции задач. Если взято за правило бахать коммитами по 1000 строк в 50 файлах, то либо ревью по чек-листу будет занимать дни, либо все скатится в "ок, это код, ревью пройдено".