Code Review & Response

aka «The Hard Part»

Коллега написала код, опубликовала и описала его. Тебе пришло уведомление, что она назначила на тебя code review. Что делать дальше?

Серия записей про code review:

1. Зачем?
2. Создание Pull Request’а
3. Мысли про Pull Request Assignee
4. Code Review & Response

Слушать

У каждого изменения в pull request’е есть цель. Кроме относительно очевидных внешних «исправить багу», «добавить фичу»¹ и «актуализировать код/зависимости», это могут быть внутренние, например:

• служить промежуточным шагом в исправлении бага/добавлении фичи («эта функция экспортируется потому, что раньше, в неопубликованном коде, она использовалась в другом модуле»);
• выровнять существующий код с ментальной моделью в голове автора pull request’а («оно было непонятно, я переписала, чтобы стало понятнее»);
• повысить удобство разработки («я умею делать grep, а это файл раньше этому сильно сопротивлялся»)

Ревьювер может не соглашаться с ценностью каждой отдельной цели или способа её достижения, но для хорошего code review нужно их знать и, при необходимости, выравнивать их с целями остального проекта

Поэтому перед тем, как беситься из-за «говнокода» (что бы это ни значило), задумайся над целью, которую преследовала автор в этих строках. С внешними целями тебе помогут текст задачи или фоновые обсуждения. Внутренние же зачастую можно понять разве что по контексту в истории коммитов или описании pull request’а

Если же контекста автор оставила немного, то можно помочь себе с пониманием цели фильтрацией изменённых файлов, просмотром её прошлых изменений, локальным поднятием ветки pull request’а, попыткой переписать «странное решение» с учётом своих «ну, это же так не делается»…

Ну, или в краааааайнем случае, можно спросить её напрямую

Говорить

Каждый комментарий ревьювера, как и каждое изменение в pull request’е, служит для чего-то. Например, с помощью комментариев ревьювер может…

Узнавать

«Жанр» комментариев, наиболее приближённый к главному назначению code review, но редко используемый, это «Почему это написано именно так?». Даже когда все участники дискуссии знают причину, но она нигде не упомянута явно, необходимость сформулировать ответ поможет заметить избыточную сложность

Если они думают, что знают причину, то явная формулировка точки зрения одной из участниц code review может выявить случаи, когда эти знания являются всего лишь предположениями. А предположения зачастую ведут к багам, так что иногда лучше уточнить что-то «очевидное». Если же это «очевидное» уже было где-то расписано, то ничего ужасного в том, чтобы получить ссылку на комментарий² с ответом или на коммит с объяснением

К сожалению, этот жанр не только самый (на мой взгляд) важный, но и самый опасный. Прочитав «почему это написано именно так?» в плохом настроении, я могу накрутить себя мыслями «да как они сомневаются в моём труде?!» и/или «похоже, тут так нельзя, поэтому, вместо ответа на вопрос, сейчас же всё перепишу». Поэтому очень важно дать понять автору pull request’а, что в твоём вопросе нет никакого скрытого смысла. В тексте интонация передаётся крайне плохо. То есть, чтобы комментарий читался текстом настолько же положительно, как если бы он был услышан вслух, эту нехватку интонации нужно компенсировать. Например, перефразировав вопрос, выделив ударение курсивом, явно написав «прости, я тут ни на что не намекаю», или добавив капельку эмоций эмоджами

Корректировать

Намного более популярный жанр комментариев к pull request’ам — это «тут неправильно». Многие считают, что эти комментарии — самое важное в code review. Да, дополнительные пары глаз помогут выявить проблемы пораньше, но, возвращаясь к началу этой записи, исключительно важно понимать цель изменений перед тем, как их критиковать. Без этого понимания, обе стороны code review будут портить друг другу настроение — либо вредными советами, либо поверхностными правками на «отстань»

И даже если же ты знаешь, какую цель преследовала автор в этом изменении, то…³

Совсем необязательно просить исправить каждую строчку, в которой видишь проблему. Во-первых, проблемы может не быть и тогда «почему?» сработает лучше. Во-вторых, личный опыт запоминается лучше чужого, так что если проблема некритичная и/или быстро проявится и/или легко спрячется от пользователей, то десять минут паники научат многому. Например, семь лет назад, я так научился тому, что новые фичи стоит прятать под feature toggle и что кэш надо инвалидировать

Когда же проблема действительно стоящая комментария, то все подобные комментарии лучше оставить за раз, чтобы не затягивать время до закрытия pull request’а ожиданием фиксов/комментариев. Иными словами, «комментарий-комментарий-фикс-фикс» лучше «комментарий-фикс-комментарий-фикс»

Кроме указания на проблему, ревьювер может предложить её решение. Github позволяет удобно это делать прямо в веб-морде

Если имеешь хорошие отношения с автором pull request’а и вы не сомневаетесь в способностях друг друга, то можно склонировать себе ветку и предложить улучшения в виде коммитов. Куда пушать эти исправления — в новую ветку (с открытием pull request’а к pull request’у ⁴) или в ту же самую — зависит от того, как к этому относится автор оригинальной ветки. Это, как и «позволить ошибиться», крайне рискованный путь, но он существует

Возвращаясь к потери интонаций в тексте… Если человек может неправильно понять безобидное «почему?», то «я написал код для твоего pull request’а» или ярко красное «@username requested changes» точно рано или поздно будут приняты на личный счёт. Так что его надо намеренно смягчать или хотя бы напомнить всем, что критика кода, а не человека

Но даже «@username requested changes» лучше тишины

Форматировать

Что не надо комментировать, так это точки-с-запятыми, отступы и прочее форматирование. Если форматирование кода действительно важно, то вместо написания комментария «мы ставим пробелы внутри фигурных скобок», потрать силы на простое автоматическое исправление форматирования⁵ и на запуск линтера в CI-окружении⁶

Без этого, ревьюеру надо постараться, чтобы удержать фокус на смысле pull request’а и не переключиться на комментирование поверхностных и зачастую бесполезных для конечных пользователей «отсортируй импорты по алфавиту»

Иметь в виду

Комментарии ревьюверов могут быть адресованы не исключительно автору pull request’а, но и другим коллегам

Например, чтобы привлечь внимание к сложности интеграции с API другой части системы. Или к большему-чем-хотелось-бы количеству мануальных правок там, где рефакторинг/типы/автоматизация упростили бы и сократили бы будущие pull request’ы. Или к схожести нового кода на код в другом проекте и возможному code share

Сразу реализовывать эти комментарии, в том же pull request’е, скорее всего, излишне увеличит его scope, но, после пары таких напоминаний, будет легче о них вспомнить когда возникнет вопрос планирования следующего спринта

Поощрать

Если после открытия pull request’а получать только описанные ранее нейтральные и относительно негативные комментарии, то естественно люди будут избегать code review — «Я старалась, работала, обходила проблемы, только чтобы перед самым финишем слушать, что тут странно, тут неправильно, тут плохо…»

Поэтому, в противовес всему этому, полезно хотя бы изредка хвалить особенно хорошие правки, будь то изящное решение проблемы, хорошая идея (которую ревьювер может применить в своём коде), или банальное «спасибо, давно болит, но никак руки не доходили»

Без комментариев, code review — это всего лишь вахтёрство. С комментариями только из серии «тут направильно, исправь» — вредный gatekeeping. Надеюсь, эта запись продемонстрировала, что с правильными комментариями, pull request становится намного полезнее всем его участникам

Спасибо Богдану Бузу и Александру Веринову за помощь в написании этой записи

P.S. Этот блог давно можно читать с помощью RSS, но им пользуются только олдфаги, вроде меня. Для более молодых духом читателей, с недавних пор появился телеграм-канал