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

I often hear people say that pull requests are necessary because without them you can't do code reviews - that's rubbish. Pre-integration code review is just one way to do code reviews, and for many teams it isn't the best choice.

Martin Fowler, Pull Requests

Pick the style of review that matches your context. Invent a new one if necessary. Don’t just copy the open source pull request model just cause. Think. Experiment. Share.

Kent Beck, Thinking About Code Review

Короче, давайте с источниками и мемчиками пройдемся по теме ревью и пулл-реквестов.

Про саму практику код-ревью

Очевидно, код-ревью работает. Мы видим кучу исследований, где показано, что код-ревью приводит к значительному снижению числа багов. Например, такое:

Most forms of testing average only about 30 to 35 percent in defect removal efficiency levels and seldom top 50 percent. Formal design and code inspections, on the other hand, often top 85 percent in defect removal efficiency and average about 65 percent.

Впрочем, если вы знаете, как работают исследования, то понимаете, что нередко все очень сильно зависит от формата исследования: выборки, условий, контроля, трактовки, позиции автора, фазы луны и погоды на улице. Поэтому, при желании, вы легко найдете исследование, которое б��дет полностью опровергать эти выводы. Например, такое:

Contrary to the often stated primary goal of code reviews, they often do not find functionality defects that should block a code submission. Only about 15% of comments provided by reviewers indicate a possible defect, much less a blocking defect. Rather, it is feedback related to the long-term code maintainability that comprises a much larger portion of comments provided by reviewers; at least 50% of all.

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

И это все, конечно, хорошо, но чаще всего код-ревью используют не столько для повышения качества. Эти задачи обычно возлагают на процессы тестирования. В основном, код-ревью используется для решения проблемы доверия. Сейчас поясню.

Например, в опенсурсе контрибьютором может быть кто угодно. И это, конечно, не очень безопасная ситуация, поэтому, разумеется, перед мержем изменения должны быть проверены ответственным лицом. Да и в корпоративной разработке опытные разработчики смотрят код новых членов команды и менее опытных специалистов, чтобы убедиться, что те не наломали дров. И, объективности ради, должен признать, что это тоже своего рода контроль качества.

Код-ревью под микроскопом

И вот теперь давайте разберём ревью по полочкам.

1. Код-ревью ≠ пулл-реквесты

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

В основном, слышим мы её от экстремальщиков. Но ведь и это не абы кто, это глыбы нашей индустрии, те самые атланты, на чьих плечах стоят Design Patterns, Agile, DDD, CI, CD, Clean Code, TDD, BDD и прочие аббревиатуры и идеи, смысл которых так часто ускользает от простого программиста. Я уже привел выше цитаты Кента Бека, Дейва Фарли, Мартина Фаулера. Но и это далеко не полный список. И не одни лишь экстремальщики всерьёз критикуют пулл-реквесты.

Но почему именно экстремальщики? Если коротко, потому что у этих есть парное программирование и непрерывная интеграция, а вот пулл-реквесты в их картину мира ложатся плохо. Если чуть раскрыть тему, то причина кроется в том, что pull request - это типичный shift-right, что для экстремальщиков - страшный грех, хуже убийства и кода без тестов. И вот этот вопрос мы обсудим поподробнее чуть ниже.

Собственно предметная критика пулл-реквестов

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

Какие у пулл-реквестов есть недостатки? Начнем с явных, которые все знают и не любят: долгое ожидание ревью, бессмысленные стилистические замечания и, часто, номинальное ревью при отсутствии реального. Существуют, например, исследования, показывающие, что чем больше файлов �� пулл-реквесте, тем меньше у него комментариев. Это означает, что чем больше фича, тем меньше шансов, что она получит все хорошее от код-ревью, и тем больше шансов, что она получит все плохое.

Code review usefulness is negatively correlated with the size of a code review. That is, the more files there are in a single review, the lower the overall rate of useful feedback. The decrease however only starts to be noticeable for reviews with 20 or more changed files.

Code Reviews Do Not Find Bugs. How the Current Code Review Best Practice Slows Us Down

The number of review comments decreases for successive files in the pull request; by around 16% per file.

Impact of number of files on number of review comments

И, вдобавок, есть неявный недостаток, который не очень хорошо заметен, но очень важен: pull request - это тот самый shift right, о котором я писал выше. Изменения часами и, порой, днями, настаиваются в фича-ветке, прежде чем пойдут дальше.

The median time from a review being requested to receiving all necessary sign-offs is about 24 hours, with many lasting days if not weeks [5]. A long time in review causes process stalls and affects anyone who might be waiting to take a dependency on the new code. In addition, the longer the review time, the harder is for the author to switch back to the change and incorporate the feedback of the reviewers without potentially introducing new defects.

Code Reviews Do Not Find Bugs. How the Current Code Review Best Practice Slows Us Down

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

Есть ли иные виды ревью, которые бы не обладали такими проблемами? Да, асинхронное ревью и парное программирование.

2. Проблема доверия может решаться не только через код-ревью

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

А как тогда? Аджилисты утверждают, что не стоит нанимать людей, которым вы не доверяете. Но жизнь, конечно, чуть сложнее. Чтобы построить атмосферу доверия, надо сделать очень много всего правильно: нанять правильных людей, хорошо их обучить, погружать постепенно, наделить их правом принимать решения, выстраивать отношения, где вы понимаете их мотивы, ценности и цели, а они - ваши. Короче, это вообще нетривиальная задача.

И, если вы плохо справляетесь с пипл-менеджментом, то у вас тут будут проблемы. Если хорошо справляетесь, тоже будут. Но, несмотря на все трудности, именно этим путём я и рекомендую идти тимлидам. Команда, в которой есть доверие - это команда, в которой есть ответственность, самостоятельность, инициатива и уверенность. И это не просто красивые слова, это именно то, что отличает того самого программиста ясно-солнышко, на которого молится вся команда, от остальных её членов. Если вы смогли выстроить доверие в коллективе, то в вашей команде каждый будет программистом ясно-солнышко. И вот такая культура в команде - это некст-левел, который значит куда больше, чем любая техническая практика.

3. Для контроля качества есть тестирование

Ревью, конечно, не убережёт вас от всех багов. Значит ли это, что задачу контроля качества надо переложить только на тесты? Дискуссионный вопрос.

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

В общем, ревью - это хороший усилитель качества. За дополнительную плату. И плата тут немаленькая: и парное программирование, и асинхронное ревью, и пулл-реквесты имеют свою весьма приличную стоимость.

Modern code review process is expensive. Developers spend on average six hours a week reviewing changes of others [6]. Not only is it a significant effort in terms of time spent but also it forces the reviewer to switch context away from their current work.

Code Reviews Do Not Find Bugs. How the Current Code Review Best Practice Slows Us Down

4. Качественное ревью требует времени

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

Да, это непросто. Да, не для каждой команды подойдет. Но, если вам удастся это внедрить, вы вряд ли пожалеете.

Меня зовут Саша Раковский. Работаю техлидом в расчетном центре одного из крупнейших банков РФ, где ежедневно проводятся миллионы платежей, а ошибка может стоить банку очень дорого. Законченный фанат экстремального программирования, а значит и DDD, TDD, и вот этого всего. Штуки редкие, крутые, так мало кто умеет, для этого я здесь - делюсь опытом. Если стало интересно, добро пожаловать в мой блог.