Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
Привет! Меня зовут Сергей Загурский, я работаю в Joom в команде инфраструктуры. В своей практике ревьюера кода я регулярно сталкиваюсь с тем, что автор не понимает, что ревьюер не является волшебным чёрным ящиком, в который можно закинуть любые изменения и получить по ним обратную связь. Ревьюер, как и автор, будучи человеком, обладает рядом слабостей. И автор должен (если, конечно, он заинтересован в качественном ревью), помочь ревьюеру настолько, насколько это возможно.
Хочу рассказать, как автор кода может упростить работу ревьюеру и увеличить тем самым как качество ревью, так и производительность ревьюера. Эта статья вполне может использоваться в качестве в вашей внутрикорпоративной документации как руководство для подготовки изменений к ревью. Она, собственно, и была скомпилирована из такого руководства.
Зачем мы делаем ревью кода
Вы наверняка и без меня это знаете. Поэтому я не буду углубляться в подробности и растекаться мыслью по древу. Расскажу о самых основах.
Ревью кода замедляет разработку. Это главный аргумент, который вы услышите от противников ревью. И это правда, но не вся. Ревью действительно замедлит вашу разработку. Изменения действительно будут дольше проходить весь путь от клавиатуры до продакшена. Однако, замедляя разработку «здесь и сейчас», ревью позволяет сделать кривую сложности изменения системы более пологой. Я не претендую на научность, но проиллюстрировал это на графике выше.
Он показывает концепцию: качественный процесс ревью уменьшает темп роста сложности. И это — главная ценность ревью кода. Это та ценность, которая понятна бизнесу, т. к. речь идёт о скорости разработки, которая напрямую уменьшает стоимость разработки и увеличивает конкурентоспособность. Но если вдруг планируется, что ваш проект не должен надолго пережить точку пересечения графиков, то ревью можно и не вводить.
На примитивном уровне ревью кода также является моделью того, как с написанным кодом будет идти взаимодействие в дальнейшем. Если у ревьюера не возникает никаких проблем с пониманием, значит и при чтении или отладке кода в будущем также сложностей не будет. И наоборот, если ревьюер не может ничего понять, то это write only код. Его развитие и отладка будут стоить дороже.
На более высоком уровне, ревью хранит в целостности архитектуру вашей системы. Ревью сопротивляется неоправданному внесению каких-то противоречащих системе элементов (также известных как костыли). Ревью способствует единообразию подходов к решению задач, что также уменьшает сложность проекта.
Дальше мы сосредоточимся на самой фундаментальной составляющей процесса ревью — на понимании кода.
Как ревьюер делает ревью
Чтобы процесс ревью являлся таковым, ревьюеру требуется, не больше и не меньше, понять суть изменений. Это основа процесса, без которой ревью не имеет никакого смысла. Для понимания требуется время. И это не то количество времени, которое можно потратить в перерыве, «за кофейком». Для понимания требуется время, сопоставимое с тем временем, которое было затрачено на написание кода. Иногда даже больше. Ревью является полноценной работой, наряду с написанием кода. В чём-то даже сложнее.
Ревьюер часто бывает не настолько погружен в контекст, как автор. Это тоже усложняет понимание сути изменений. Ревьюер может первый раз в жизни видеть код, в котором производятся изменения. Ревьюер может ничего не знать о задаче, которую решает автор. Ревьюер может не понимать, зачем всё это делается. Своевременное предоставление ревьюеру информации о контексте экономит время ревьюера и улучшает качество ревью.
Если речь идёт о каких-то нетривиальных изменениях, то в голове ревьюера происходит тот же процесс анализа, что и в голове автора. И если в ходе этого анализа могут возникать разные решения, не имеющие друг перед другом очевидных преимуществ, то ревьюеру придётся потратить время, чтобы понять, почему автор остановился на определённом решении. Автор, в итоге, мог принять единственно верное решение, но ревьюер, в условиях отсутствия обоснования от автора, потратит своё время на это обоснование.
Бывает такое, что автор вместе с основными изменениями делает что-то, не связанное с решением своей задачи. Когда ревьюер сталкивается с таким, то он тратит дополнительное время, чтобы понять, как эти изменения связаны с решением основной задачи.
Ещё одним большим пожирателем времени ревьюера является написание замечаний. Да, есть замечания, которые относятся к каким-то, условно, косметическим аспектам. Однако даже их бывает полезно снабдить сопровождающей информацией вроде ссылки на нужную часть документа по стилю кода. И это всё требует времени.
Также нужно помнить, что ревьюер, будучи человеком, склонен откладывать начало ревью на потом, если видит ревью повышенной сложности. Вежливый ревьюер может постесняться потребовать разбить большой Pull Request на несколько и будет платить за это своим временем и качеством ревью.
Как помочь ревьюеру провести качественное ревью
Главная задача автора, который болеет за качество процесса ревью, состоит в том, чтобы максимально упростить и ускорить работу ревьюера. Автор, который без дополнительной обработки вываливает на ревьюера результат своей работы, проявляет крайнюю степень неуважения к работе ревьюера.
Перед тем, как превратить свои изменения в Pull Request, следует разбить их на логические куски, если в этом есть необходимость. Комфортный объём ревью заканчивается примерно на 500 строках кода изменений. Допустимый — примерно на 1000 строках. Всё, что за пределами 1000 строк, должно быть разбито на более мелкие Pull Request’ы.
Если вы, как и я, не разбиваете свои изменения на куски комфортного размера заранее, то это придётся проделать перед отправкой ваших изменений на ревью. Отмазка, что на это нужно потратить время, не катит. Принимая решение об отправке на ревью 1000+ строк кода, вы, фактически, оцениваете своё время дороже времени ревьюера. По нашим правилам у ревьюера всегда есть право потребовать разбить изменения на более мелкие куски. Мы всегда просим коллег относиться с пониманием, если он этим воспользуется. С опытом становится проще строить свою работу так, чтобы у вас не появлялись гигантские Pull Request’ы, в которых «ничего нельзя отделить».
Отдельно стоит упомянуть изменения, внесённые в код с помощью авторефакторинга или sed’а. Объём таких изменений может быть очень большим. Наличие автоматических изменений вместе с осмысленными изменениями усложняет ревью. Всегда отделяйте авторефакторинги в отдельные Pull Request’ы, если их объём сопоставим с объёмом нового кода, написанного вами.
Следующая часть подготовки состоит в информировании ревьюера о сути изменений. Как я уже писал выше, ревьюеру нужно знать: что сделано, зачем сделано, как сделано, и почему именно так сделано. Часть информации у ревьюера уже может быть. Часть — есть в коде. Всё, что не очевидно из кода, должно быть прописано в описании изменений.
Если задача подразумевала какой-то нетривиальный анализ, то автор должен быть готов, что этот же анализ будет проводить и ревьюер. И ревьюер может прийти к другому выводу. Часто бывает так, что автор в процессе решения задачи проходил через этап «более простого» решения и отверг его в процессе по какой-то причине. Для экономии времени ревьюера стоит включить обоснование тех или иных решений в описание изменений. Это обоснование останется в проекте, позволив более уверенно менять решение, если соображения, которыми руководствовался автор при принятии решения, утратили актуальность.
Случаются ситуации, когда у ревьюера может быть своё сильное мнение о том, как должна быть решена задача. В этом случае автору следует заранее обсудить это с ревьюером, чтобы исключить крайне некомфортную для всех сторон ситуацию, когда ревьюер потребует переделать значительную часть изменений. Возможно даже придётся встретиться и поговорить с ревьюером, чтобы обосновать своё решение и убедить ревьюера или принять его решение.
Итак, автор подготовил изменения к ревью. Пора отправлять ревьюеру? Нет! Тут наступает ещё один важный этап, саморевью. Под саморевью я понимаю именно ревью, осуществлённое самим автором. Не просто беглый визуальный контроль. Нужно постараться провести полноценное ревью. Ответственно проведённое саморевью сэкономит уйму времени ревьюеру. Наверняка в коде есть какие-то остатки отладочных конструкций, закомментированного кода, TODO-комментариев, бессмысленных изменений форматирования, не пригодившегося кода и прочего подобного. Взгляд глазами ревьюера также поможет оценить сложность ревью и даст возможность добавить дополнительной информации к ревью, которая была упущена при подготовке. Пренебрежение автором саморевью я считаю проявлением крайнего неуважения к ревьюеру.
В целом, я считаю допустимым потратить порядка 10% от времени, затраченного на написание кода, на подготовку к ревью. Это время автора, которое мы обменяем на экономию времени ревьюера, и на улучшение качества ревью. Следует помнить, что ревьюеру на качественное ревью легко может потребоваться и 20%, и 50% от времени, затраченного автором на написание кода.
Вот только теперь автор может с чистой совестью отправить изменения ревьюеру.
Дальше начинается жизненный цикл Pull Request’а. Ревьюер должен либо одобрить его, либо попросить внести изменения. Чтобы упростить работу ревьюера, автору стоит добавить комментарий к каждому запрошенному изменению. Это вполне может быть краткое «OK» или «Исправил», если ничего другого не требуется. Убедитесь, что вы понимаете, что просит ревьюер и что вам понятна его аргументация. Не стоит безоговорочно принимать любые запросы на внесение изменений, возводя тем самым ревьюера в околобожественный ранг. Ревью — это обоюдный процесс. Если ревьюеру что-то не понятно, то он спрашивает об этом автора, и наоборот. В случае, если автор не очень опытен, ревьюеру следует приложить особенные усилия к описанию запросов на изменения, чтобы воспользоваться возможностью поделиться с автором своим опытом. Бывают и спорные моменты, когда аргументация недостаточно сильная, чтобы склонить обе стороны к единой точке зрения. С учётом того, что автор находится в более уязвимой позиции, считаю, что при прочих равных преимущество должно оставаться за автором.
Не вносите изменений в свой Pull Request, не относящихся к тому, что попросил вас ревьюер. Это крайне сбивает с толку. Также следует воздержаться от rebase и подобных действий.
Получили одобрение от ревьюера? Отлично, ещё одно качественно написанное и оформленное изменение было добавлено в проект!
