В начале своей карьеры я как-то работал над одним заказом, создавая платформу сентимент-анализа для социальных сетей. В то время Twitter ещё был Twitter’ом. Наша команда состояла из семи человек, среди которых я был джуниором. Мы были молоды и полны энтузиазма. Наш девиз можно было описать как: «Мы гибкие, быстрые и всё ломаем!». Да, мы действительно гордились своей скоростью. Код-ревью? Я вас умоляю. Мы считали эту практику бюрократическим пережитком корпоративного мира.
И что вы думаете? Через несколько месяцев наша база кода стала подобна минному полю. Причём баги нас волновали меньше всего, хотя их была уйма. Реальная проблема заключалась в том, что никто не мог понять код, написанный другими. У нас во многих местах дублировалась логика, и в модулях использовались разные стили кода. Всё было очень печально.
Тогда до нас дошло! Нужно взять всё под контроль. Код-ревью реально помогают сохранять код читаемым, обслуживаемым и масштабируемым.
Итак, в двух словах: если вы не проводите код-ревью, или делаете их «для галочки», то обрекаете себя на боль, пусть не сразу, но в конечном итоге однозначно. Это можно сравнить с возведением дома на фундаменте из песка. Какое-то время он, может, и простоит, но явно недолго. А в мире стартапов второго шанса у вас может уже не быть.
Думаю, теперь мы все согласны, что код-ревью – это хорошо. В сети уже есть сотни статей, которые раз за разом подтверждают, что выполнение код-ревью приносит больше ценности, чем требует времени. Многие статьи также рассказывают, как лучше их выполнять. И да, я решил написать собственную, почему нет? У меня есть своё видение многих вещей, и код-ревью – это одна из таких вещей, которую все делают по-своему, и для которой нет единого «верного» способа.
Так что предлагаю вам прочесть и мою статью, принять в ней то, с чем вы согласны, и проигнорировать остальное. В конечном итоге ваш процесс код-ревью наверняка будет отличаться от того, которому следую я, и это нормально.
Прежде чем начать, предлагаю вспомнить, почему код-ревью столь необходимы с позиции разработчика:
- Две пары глаз видят лучше одной. Мы, как разработчики, склонны переоценивать качество собственного кода, поэтому вторая пара глаз позволяет поумерить наш пыл и сделать код объективно лучше. Чем больше ревьюеров, тем более объективно качественным становится код. Хотя для качественного ревью обычно достаточно одного человека.
- Ревьюер – это первый, кто читает наш код, и он же может оценить его на пригодность к обслуживанию. Вы наверняка слышали фразу: «Пишите код так, будто забудете о нём на пять лет и потом вернётесь, чтобы обслужить». Если у ревьюера возникает сложность с пониманием происходящего во всех хитросплетениях функций, значит код необслуживаемый и требует рефакторинга.
- Все мы люди и не всегда исправно следуем руководствам. Код-ревью помогают нам оставаться в тонусе и препятствуют отправке кода, который: а) не соответствует руководствам компании и б) имеет нежелательные побочные эффекты. Под побочными эффектами я имею ввиду, например исключения нулевых указателей, аварийное завершение и состояния гонки. Существует множество «типичных» случаев, которые определяются тем, какое ПО вы пишете. Некоторые из них мы разберём ниже.
Теперь представим, что вы автор новой функциональности или исправления бага, которое требует ревью.
Одна из самых серьёзных проблем из мне известных заключается в том, что люди слишком привязываются к своему коду. Они рассматривают его почти как часть себя. В итоге, если код плохой, значит, они тоже плохие. Но это не так. Во-первых, вы не идеальны, и вы не являетесь кодом, который пишете. Код – это коллаборативный документ, который будет дорабатываться сотнями инженеров, и с вашей помощью в том числе, он постепенно станет лучше. Через пять лет вы уже не вспомните, что писали сегодня. Так зачем испытывать столь сильную эмоциональную привязанность к тому, чему суждено претерпеть ещё множество изменений? Чёрт возьми, я едва могу вспомнить, что писал два года назад, хотя это всего лишь я.
Вторая серьёзная проблема – это ментальная модель «Мы против Них». Это самое худшее. Предположим, что вы находитесь по разные стороны баррикад. Тогда получается, что есть два лагеря: разработчики и ревьюеры. Нет же. И те и другие являются инженерами, создающими продукт. В конечном итоге вы тоже станете ревьюером, следящим за тем, чтобы код двигал ПО вперёд, а не назад.
Перед началом ревью
Есть не менее двух моментов, которые вам с командой нужно утвердить, прежде чем начинать ревью:
- Определить, что значит «Готово».
- Установить стандарты оформления кода.
▍ Определение «Готово»
Вам явно не нужно, чтобы кто-то отправлял на ревью код, не доведённый до нужного уровня. И если умножить это в сто раз, то вы получите полуготовое ПО. Вам нужно конкретное, «чёрным по белому», определение, которое будет понятно для всех членов команды, и с которым они будут согласны.
Подумайте об этом. Если «Готово» означает, что код написан, но не протестирован, то вы обрекаете себя на серьёзные проблемы. Если «Готово» означает, что он был протестирован, но не задокументирован, то вы просто перекладываете работу на следующего несчастного, которому придётся разбираться со всем ходом вашей мысли. Запомните, «Готово» должно означать, что код написан, протестирован, задокументирован и готов к развёртыванию, которое не приведёт к падению всего кластера серверов. Вроде легко, не так ли? Опят же, ваши критерии могут быть иными, поэтому следует сесть и обсудить все условия с командой.
▍ Стандарты оформления кода
Здесь тоже самое – стандарты оформления кода важны не только для самого инженера, который его пишет, но и для всех разработчиков, которые будут его читать.
Ваши стандарты должны охватывать все аспекты начиная с отступов и именования переменных, заканчивая обработкой ошибок и логированием. Цель в том, чтобы любой разработчик из любой команды, глядя на код, мог сразу узнать его паттерны, правила именования и т.д. Код должен восприниматься знакомым, особенно в процессе ревью.
Реализуйте правильно хотя бы этих два условия, и вы заложите фундамент для эффективного и более комфортного процесса код-ревью.
Стандартизируйте всё
Заложив основу для код-ревью, можно сосредоточиться на стандартизации – чек-листах, инструментах и CI (Continuous Integration, непрерывная интеграция).
Начните с чистого чек-листа. Вот несколько неплохих примеров. Как правило, ваш чек-лист должен включать всё самое главное: функциональность кода, читаемость, соответствие стандартам оформления, тесты безопасности и прочее. Например, нужно проверять, делает ли код то, что должен, не содержит ли очевидных логических ошибок, и не изобретает ли колесо.
Из репозитория GitHub выше
Вы можете задаться вопросом, зачем вам чек-лист? Возможно, он прост, удобен, и его легко запомнить? Вовсе нет. Даже лётчики после тысяч полётов используют чек-листы во время взлёта и посадки. Каким-бы экспертом вы ни являлись, грамотные чек-листы могут улучшить итоговые результаты и снизить риск внесения плохого кода в продакшен.
После публикации чек-листа нужно настроить инструменты. Они станут вашими лучшими друзьями, несущими всю основную нагрузку – линтеры, статические анализаторы кода и сканеры уязвимостей. Например, есть огромное множество хуков pre-commit, которые следят, чтобы отправляемый код был надёжным:
Хук pre-commit помогает убедиться в качестве кода до его отправки в репозиторий
Эта тема довольно спорная, и здесь есть несколько мнений. Некоторым нравятся эти хуки, другие считают, что проверка должна происходить в процессе CI, а третьи предпочитают плагины IDE. Здесь выбирать вам. Лично я миксую все эти подходы – например, у меня есть несколько хуков для линтинга кода Python, также есть прикреплённые к CI инструменты для проверки типов, а ещё в JetBrains IDE настроено авто-форматирование. Поэтому, если что-то идёт не так, я либо не могу внести код, либо получаю уведомление по почте от CI, либо IDE сообщает мне, что я идиот.
Типичные моменты
Ниже я опишу, на чём фокусируюсь в процессе код-ревью. Опять же, ознакомьтесь с этой информацией и просто отметьте для себя то, что вам подходит. Уверен, в этом вопросе нет чего-то правильного или ошибочного при условии, что код, который вы отправляете в основную ветку, соответствует вашим субъективным стандартам качества.
Итак, первым делом я обращаю внимание на размер пул-реквеста. Чем он больше, тем сложнее его анализировать и выше вероятность, что я устану. Большие пул-реквесты перегружают. А когда человек перегружен, он начинает упускать детали. Важные детали. Поэтому если я вижу объёмный пул-реквест, то обычно оставляю комментарий, что его нужно разбить на более мелкие.
Далее я проверяю код на наличие граничных операторов. У них есть как плюсы, так и минусы. Некоторые говорят, что функция должна иметь одну точку выхода, но я не согласен. Я воспринимаю граничные операторы как то, что сохраняет функцию понятной. И я ненавижу вложенные условия if. Когда я вижу граничный оператор, то думаю: «Это условие неверно, значит, функция выходит рано, вызывает исключение или возвращает результат». Таким образом, я уже сократил ментальную нагрузку в отношении следующих строк, так как в этом случае они меня не интересуют. Всё просто.
Граничные операторы. Источник
Мёртвый или закомментированный код. Я бегло просматриваю код в поиске чего-то, что не соответствует заявленной функциональности. Каждая строка должна служить какой-то цели. Если причины для её присутствия нет, то зачем впихивать эту строку в продакшен?
Я проверяю имена переменных и параметры. Я ценю явные параметры и изначальную ясность их действий. Не люблю разбираться, что делает код, читая функцию. Я хочу видеть это сразу. Есть огромная разница между именованием входного параметра как data и sumEmployees. Это совсем разный уровень информации.
Я также проверяю, не выполнил ли разработчик преждевременную оптимизацию и не внёс ли лишних технических сложностей. Будучи сам разработчиком, я понимаю, что мы любим пошаманить с кодом, и порой тяготеем к сложным формам вместо того, чтобы сделать всё просто.
Оверинжиниринг – это, пожалуй, один из самых распространённых грехов среди тех, что мне встречались (наряду с неудачным именованием переменных). Это та ситуация, когда разработчики создают лабиринт из интерфейсов, фабрик и абстрактных классов для задач, которые можно было решить одной функцией. Их просили собрать тостер, а они построили фабрику тостеров. Обычно в таких случаях я задаю вопрос: «А эта сложность реально необходима? Что подтолкнуло тебя создать это таким образом?» Ревьюер не всегда прав, поэтому стоит задуматься, вдруг для этой сложности есть причина. Так что я обычно просто спрашиваю.
Если вы проверяете пул-реквест для огромной базы кода, то нужно учитывать, что основная часть служебной функциональности в ней наверняка уже реализована. Иногда люди не осознают, что заново изобретают колесо. Но наличие в системе нескольких модулей, делающих одинаковые вещи, вряд ли будет удачной идеей. Поэтому я также проверяю, не пошёл ли разработчик случайно таким путём, когда мог просто повторно использовать уже существующие модули. Как правило, это требует лишь небольшого надзора, зато позволяет сохранить код в ухоженном виде.
Ещё бывает, что программист увлекается разработкой порученной ему функциональности настолько, что начинает рефакторить части кода, которые трогать не должен. Думаю, что рефакторинг следует делать отдельным пул-реквестом, и для его проверки нужно также уведомлять авторов корректируемого кода. Это поможет избежать ситуаций, когда несколько человек работают над разными функциями, опирающимися на один модуль, и один из них, занявшись рефакторингом этого модуля, ломает что-то у другого. Лучше без таких сюрпризов.
И хотя я согласен, что грамотно написанный код должен говорить сам за себя, иногда не обойтись без комментариев. Когда мы смотрим на код, нам должно быть понятно, что он делает. Если же мы читаем комментарии, должно становиться ясно, почему он это делает. Поэтому я проверяю, поясняют ли комментарии внутреннюю суть функции. В комментариях вы объясняете сложность структуры бизнеса и решения, которые не сразу очевидны. Это помогает будущим мейнтейнерам (под будущим мы подразумеваем момент, спустя год и более) понять причины принятия вами тех или иных решений в коде. Вы же не хотите, чтобы при чтении собственного кода через год у вас возник вопрос: «Что всё это значит?»
Я также очень не люблю пул-реквесты, где вносятся новые зависимости. Приходится много разбираться, пытаясь понять, насколько эта технология уместна и можно ли её использовать исходя из требований лицензии и влияния на безопасность. Поэтому если я вижу, что автор добавил новую технологию, то обычно задаю много вопросов.
Если были созданы новые конечные точки, я проверяю, отвечают ли они нашим руководствам. Обычно мы следуем стандарту REST, значит, она должна ему соответствовать. Кроме того, я проверяю, достаточно ли защищена эта конечная точка по следующим пунктам:
- Возвращает ли она только релевантные данные, прошедшие необходимую фильтрацию?
- Как обрабатывается сеанс, если конечная точка находится после этапа авторизации?
- Заголовки.
- Состояние гонки/Идемпотентность.
Думаю, всего перечисленного достаточно, чтобы вы примерно поняли, на чём я сосредотачиваю внимание в код-ревью. Естественно, есть и множество других аспектов, которые вы можете проверить. Вот несколько ссылок, где приводятся полезные рекомендации по этой теме:
Эти недочёты весьма распространены, но вполне избегаемы. Со временем вы выработаете привычку их не допускать. Тут всё дело в опыте – чем больше кода вы пишете, чем больше ревью проходите, тем меньше акцентируете внимание на доработке кода, и тем больше это происходит бессознательно. Как я писал выше, очень важно использовать чек-листы. Помните, цель код-ревью – не просто найти ошибки, но также сделать из них выводы, которые повысят не только качество кода, но и ваш навык.
Как правильно сообщать о проблемах
Представьте, что нашли в коде недочёты. Как сделать так, чтобы разработчик не возненавидел вас после того, как вы ему о них сообщите? Я нередко видел, как после обнаружения проблем в процессе ревью мотивация инженеров падала на несколько недель. Но так быть не должно.
Я строго верю в методологию «Гуманных код-ревью». Вот её ключевые принципы:
- Любой код, в том числе ваш, может быть лучше. Если кто-то хочет изменить что-то в вашем коде, то это не нужно воспринимать как личное нападение.
- Речь идёт о коде, а не его авторе.
- Никаких личных чувств. Избегайте мыслей вроде «…потому что мы всегда делали это так», «Я потратил на это уйму времени», «Это не я дурак, а ты», «Мой код лучше» и так далее. Улыбнитесь, простите себя, взбодритесь и двигайтесь вперёд.
- Пусть все ваши комментарии будут позитивными и направлены на улучшение кода. Будьте добры к его автору, но с ним не церемоньтесь.
Прекрасный пример, как делать не следует. Источник «Toxic Code Reviews»
Иногда то, что вызывает у вас недовольство, является просто делом вкуса, а не объективным недочётом кода. Мы все разработчики ПО, и с годами каждый накопил свой багаж мнений. Учитесь отличать претензии на основе своего вкуса от объективных недочётов. Цель – улучшить код, а не победить в споре.
- Неправильно: «Ты пишешь непонятный мне код».
- Неправильно: «Твой код очень плох/непонятен/и т.д.».
- Правильно: «Мне сложно понять твой код».
- Правильно: «По-моему, коду в этом модуле недостаёт ясности».
- Правильно: «Не могу разобраться в сложном моменте, можно как-то его прояснить?»
Используйте формулировки от своего лица. Вместо того чтобы говорить: «Ты пишешь код, как школьник», попробуйте: «Мне сложно понять, что тут происходит». Чувствуете разницу? Первый вариант – личное оскорбление, а второй – конструктивная обратная связь.
Согласно методологии «Гуманной коммуникации» вопросы нужно задавать с позиции равного, а не делать заявления свыше. Вместо: «Сейчас же переименуй эту переменную в sumOfBalances», попробуйте: «Как насчёт назвать эту переменную sumOfBalances?» И дело здесь не просто в вежливости, важно развить диалог, а не застопорить его.
- Неправильно: «Мы делаем так постоянно!»
- Неправильно: «Мне не нравится, как ты назвал переменные».
- Неправильно: «Ты выбрал плохой фреймворк, нужно было использовать X».
- Правильно: «У тебя неплохое решение, но я знаю несколько альтернативных вариантов. Предлагаю обсудить».
- Правильно: «Ты уверен, что переменные названы в соответствии с нашими стандартами оформления кода?»
- Правильно: «У меня есть личное мнение насчёт этого фреймворка, давай обсудим?»
Не забывайте, вы делаете ревью кода, а не личных качеств его автора. Не называйте человека неаккуратным/плохим/отсталым просто потому, что он упустил пару тестов. Указывайте на код. «Думаю, будет лучше, если уделить тестированию этой функции больше внимания. Как считаешь?» Воспринимается гораздо мягче, нежели: «Вечно ты пропускаешь тестовые кейсы!»
Никто не садится дома за компьютер с мыслями: «Сегодня я напишу худший код, какой только видел мир». Разработчики стараются. Имейте это ввиду. Хвалите их, когда не находите проблем, или когда они делают что-то очень грамотно.
Вопрос-ответ
Вот несколько распространённых вопросов, с которыми мне доводилось сталкиваться. Ответы на них помогут вам сориентироваться в этом важном процессе.
1. Что, если код-ревью замедляет ход разработки?
Этот вопрос часто звучит с позиции выгоды для бизнеса. Зачем вообще проводить код-ревью, когда можно просто отправить код в основную ветку? Здесь возникает классический компромисс «Скорость или Качество». Помните, цель код-ревью – не просто сделать код качественным сегодня, но и предотвратить будущие проблемы, которые могут значительно замедлить ваш проект. Если вы будете принимать в основную ветку любой код, то со временем база кода станет такой вонючей, что обслуживать её будет невозможно. Если ревью занимают слишком много времени, подумайте об упрощении этого процесса с помощью чек-листов, автоматизированных инструментов и отчётливых руководств.
2. Обязательно ли делать ревью всего, или что-то можно пропускать?
Несмотря на то что есть соблазн подвергать ревью каждую строку кода, это не всегда практично или необходимо. Установите критерий для того, что именно требует ревью – например, основная функциональность, исправления багов и код, влияющий на критические части приложения. В отношении же менее критических частей можете ориентироваться на здравый смысл и доверять своей команде, например, опечатки или обновления документации не требуют ревью.
3. Можно ли полагаться исключительно на автоматизированные инструменты код-ревью?
Автоматизированные инструменты – это хорошее подспорье, но никак не полноценная замена человеку. Они прекрасно справляются с обнаружением синтаксических ошибок, нарушений стиля кода и очевидных багов. Но эти инструменты не могут проанализировать подоплёки функций, логики, архитектуры и сказать, отвечает ли код конкретным бизнес-требованиям.
4. Как организовывать код-ревью в удалённых или распределённых командах?
Просто делать это асинхронно. Я знаю, некоторые предпочитают, чтобы обе стороны в процессе ревью были онлайн, так как это ускоряет взаимодействие и экономит время, но сам предпочитаю асинхронный диалог. Вы проводите ревью, ставите вопросы и продолжаете работать, пока разработчик не выйдет в сеть и не ответит на вашу обратную связь или не внесёт правки. Для удалённых или распределённых команд ключевыми аспектами являются прозрачная коммуникация и правильные инструменты.
5. Какой максимальный размер пул-реквеста, который следует брать в ревью?
Желательно, чтобы пул-реквесты были подъёмными. Я бы посоветовал придерживаться такого объёма, который можно тщательно проанализировать за 30 – 60 минут. Если он будет слишком большим, эффективность ревью снизится и повысится шанс упустить критические проблемы. Если у вас пул-реквесты постоянно получаются объёмными, постарайтесь разбивать их на более мелкие, акцентированные единицы.
6. Как быть со срочными исправлениями в плане код-ревью?
Срочные исправления зачастую требуют иного подхода. Несмотря на то что их тоже нужно подвергать ревью, этот процесс можно ускорить. В подобных случаях важно проводить ревью после развёртывания исправления, чтобы найти любые упущенные недочёты и сделать выводы из произошедшего инцидента.
Буду признателен, если поделитесь в комментариях своими мнениями. Всех благ!
Автор: Дмитрий Брайт
