Привет! Меня зовут Артем Валевич, я тимлид в AGIMA. Одна из важнейших моих обязанностей — код-ревью, то есть проверка кода на качество, надежность и соответствие требованиям проекта. Этот процесс может ощутимо улучшить продукт, а может превратить жизнь всей команды в ад. Ключ к этому процессу — в умении не перегибать палку. Давайте посмотрим, как может выглядеть токсичный и нетоксичной фидбек, а заодно на то, как можно оптимизировать сам процесс ревью.
Останавливаться на том, что такое код-ревью и зачем оно нужно, как-то не хочется. На Хабре много подобных статей, вы их без труда найдете. Вместо этого сосредоточимся на вопросах обратной связи и концептуального подхода к проверке кода. Сто раз спорили с коллегами о том, насколько дотошным нужно быть в этом деле. Стоит ли придираться к каждой мелочи? Или лучше смотреть исключительно с позиции «работает — и ладно»?
Тема дискуссионная — и предлагаю вам делиться мнение в комментариях. Но лично я определил для себя несколько перегибов, которых нужно избегать. Таким образом я пытаюсь нащупать тонкую грань между «мой тимлид — монстр» и «мой тимлид — мастер». И к сожалению, найти ее с первого раза бывает сложно. Все мы помним случаи, когда получали некорректную обратную связь — и не понимали, что с ней делать дальше.
Мои «нет» в код-ревью: перегибы, которых лучше избегать
Фокус на мелочах. Если уделять много внимания опечаткам и код-стайлу, то времени потратишь много, а реальных результатов получишь мало. Главное — это всё-таки архитектура, алгоритмы и производительность, а косметика только на втором плане. Зачем портить настроение себе и команде, если можно ничего никому не портить.
Поверхностный анализ. Забивать на проверку кода — тоже плохая идея. И дело даже не в багах, их отловит тестировщик. Дело в безопасности всей системы. Если какой-нибудь хакер найдет уязвимость, которую QA не нашел, — жди беды. Ну и техдолг никто не отменял: если не делать сразу хорошо, однажды придется переделывать.
Токсичность. Когда я провожу код-ревью, я всё-таки ищу потенциальные проблемы с кодом, а не рассказываю коллегам, как сделал бы эту работу сам. То есть фокус моего внимание — не на поиске недостатков в их работе, а на самом коде. Мои личные предпочтения и вкусы относительно кода остаются при мне.
И обратную связь я тоже даю очень вдумчиво, подбираю слова, а иногда и интонации. И нет, я не считаю, что это пустая трата времени. Давать фидбек нужно так, чтобы у разработчика не возникало каждый раз желания попрощаться со мной раз и навсегда.
Подключаем здравый смысл: четкий алгоритм проверки и перевариваемые мердж-реквесты
Существует несколько простых правил, которым я следую, когда провожу ревью. Во-первых, всегда держу в голове, зачем этот код вообще нужен — а как иначе? Во-вторых, анализирую, насколько код вообще вписывается в проект и правильно ли спроектирована архитектура. И в-третьих, проверяю расположение: в правильном ли модуле, в нужной ли папке, в том ли классе описан метод, корректно ли он назван.
Кроме того, мы всегда подбираем разумные мердж-реквесты (MR). Лучше, если это одна фича или одно исправление. Если в MR намешали и рефакторинг, и новую фичу, и исправление багов, то ревью превращается в пытку. Согласитесь, что ревьюить 8к новых строк кода довольно сложно. К тому же на этой уйдет куча времени.
Поэтому:
разбиваем большую задачу на несколько небольших;
один MR — одна логическая задача;
чем меньше изменений в MR, тем быстрее и качественнее будет его проверка.
Упрощаем себе работу: автоматизируем проверку и внедряем кросс-ревью
Выше я говорил, что во время код-ревью не стоит концентрироваться на код-стайле и опечатках, но в целом хороший тон — когда стиль един, а опечаток нет. Добиться этого поможет автоматизация. Да и в целом современные инструменты помогают сильно упростить проверку кода:
статический анализ кода (SonarQube, Psalm) — выявление ошибок, проблем производительности и безопасности;
CI/CD-системы (GitHub Actions, GitLab CI/CD, Jenkins) — автоматизированные проверки перед интеграцией;
линтеры (ESLint, Stylelint) — автоматическая проверка код-стайла.
Еще один способ, который используется нечасто — кросс-ревью. Это такой процесс, когда код проверяет не только тимлид, но и другие разработчики из команды. И у этого подхода огромное количество жирных плюсов:
команда учится и профессионально растет;
оценка становится объективнее;
обработка мердж-реквестов ускоряется.
Но тут нужно помнить, что просто так взять и внедрить практику кросс-ревью не получится. Предварительно придется подготовить команду к этому: участники должны понимать, зачем они это делают и на что вообще нужно обращать внимание.
Даем обратную связь деликатно, вкрадчиво, но в то же время прозрачно
Существует мнение, что главное в коммуникации быть понятым. Я лично встречал людей, для которых сообщение в телеге или комментарий в коде — это просто сухая передача нужной информации. «Переделай», «Мне не нравится», «Получилось не так, как надо» — такие сообщение при этом подходе не звучат грубо. А что такого? Я просто констатировал факт: мне же реально не нравится.
Но я думаю, что надо подходить к делу тоньше и мудрее. Потому что коммуникация, особенно письменная — это не только набор слов, каждое из которых равно себе. Это еще и настроение, и эмоциональная окраска. В живом общении смысл фразы меняется в зависимости от интонации, мимики, контекста. Даже слова «я бы тебя уволил» за длинным столом в кабинете босса прозвучат одним образом, а в курилке под смех коллег совершенно другим.
Поэтому стоит всегда помнить: в общении мы используем не только слова, но и десяток других инструментов. И надо их применять осторожно.
Нет | Почему нет | Да |
Этот кусок можно отправить на помойку. Очень плохо | Эмоции не помогают решать задачи, а только мешают. К тому же неудачный подбор слов может обидеть человека | У этого кода проблемы с производительностью и логикой. Следует пересмотреть подход и сделать... |
Тут что-то не так | Обратная связь должна быть четкой и конструктивной. «Что-то» не может быть не так. Проблему всегда нужно обозначать словами | Кажется данный фрагмент кода может нарушить бизнес-логику в... |
А этот код точно нужен? | Вопрос — не помогает разобраться в сути комментария. Лучше формулировать свои пожелания четко и по существу | Проверьте, пожалуйста, нужен ли этот код. Если нет — давайте удалим |
Уверен, примеров для первого столбца у каждого наберется миллион. Да и сами мы все не без греха. Кажется, что писать длиннющий комментарий — какая-то лишняя морока. И так понятно, что я имею в виду! Но лично я себя одергиваю: пусть мой комментарий и так понятен — но лучше я перестараюсь, чем обижу разработчика. Он всё-таки свою работу сделал, теперь моя очередь.
Критерии качественного код-ревью
Собственно, вот и всё. Правил не так уж и много. Главная мысль: не превращаться в строгую учительницу с красной ручкой, которая входит в раж и ставит жирные двойки даже за описки и неаккуратный почерк. Но всё же при этом следить за важными моментами, чтобы не копить технический долг и в целом держать код в чистоте и аккуратности.
Мы для себя определили такие стандарты и применяем их на каждом проекте:
соответствие CodeStyle;
наличие тестов;
выполнение архитектурных требований;
тщательная документация и комментарии.
Плюс автоматизируем всё, что можем автоматизировать, и даем максимально развернутую и дружелюбную обратную связь. Других секретов нет. Но возможно, они есть у вас. Поэтому рассказывайте в комментариях, как проводите код-ревью, с чем согласны, с чем не согласны. В общем, давайте обсудим.
А еще у нас есть телеграм-канал для тимлидов, в котором мы подобные темы периодически поднимаем. Приходите читать.