Практики хорошего code review, или что такое code review за 15 минут. Доклад Никиты Соболева на DUMP в Казани

В 2019 году на DUMP в Казани выступал Никита Соболев – технический директор компании wemake.services. И Никита на протяжении почти 40 минут пытался вскипятить мозги слушателей секции Backend, рассуждая о code review. Сегодня хотим привести расшифровку этого «взрывного» доклада, чтобы если уж мозги бурлили, то у всех сразу.

А вот, кстати, и сам Никита Соболев во время своего выступления.

Для удобства вот ссылка на презентацию.
Доклад начался с постановки того, о чем же он будет. И внезапно прозвучала фраза: «Я не буду рассказывать о code review. Доклад будет про хардкорные технические инструменты и методы автоматизации». Главный тезис – если вы делаете code review, то вы уже проиграли. Интригующе звучит? :) Давайте вместе с Никитой разбираться, что же за этим стоит.

За основу были взяты понятия, которые предложил американский писатель Карлос Кастанеда, делание и неделание. Что же это значит? Делание – это привычка, внутри которой мы существуем. Эдакое «хомячье колесо». А неделание – взгляд на те же самые вещи, но под другим углом. Никита ловко наложил эти понятия на «просмотр кода». И получилось, что мы имеем «делание code review» и, соответственно, «неделание code review». Вот как раз последнему и посвящен доклад.

Делание становится рутиной, чем-то сложным, где нужно применить свои коммуникативные навыки, быть эмпатичным. И встает вопрос: а нафига я буду смотреть какой-то код? Неделание же – это гармония. Ты смотришь на код и восхищаешься им. Ты понимаешь, что вот этот вот код станет частью продукта и принесет новые деньги. И конечная цель, к которой подводит нас Никита всеми этими речами в том, чтобы делать code review за 15 минут. И даже есть живой пример.

Но для начала нужно разобраться, почему же все так плохо с code review?

Когда ты узнал, что есть code review, то начинаешь пытаться решить все проблемы с его помощью. А проблем на самом деле очень много, как и целей. Какие же цели ставят перед code review?

• многие люди контролируют выполнение какой-то задачи при помощи code review;
• контроль качества кода;
• чтобы новички не повторяли грабли, на которые уже сто раз наступали;
• многие делают code review для архитектурного review;
• многие через code review обучают людей.

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

Обучение людей с помощью code review.

Обычно новым людям дают какую-то неважную задачу, а это плохая идея. Она тянет за собой изоляцию (посиди вон там, сам поразбирайся) и отсутствие контроля (задача же неважная, поэтому всем пофиг).

Решение: маленькие задачи. Вы можете дать новичку сразу что-то важное, но при этом небольшое. Это ускоряет обратную связь, увеличивает вероятность найти косяк в коде. Ведь чем массивнее код, тем сложнее поиск ошибок – говорит статистика. Под «маленькими» подразумеваются задачи от 15 минут до 2х, максимум 4х часов. Соответственно, это все предполагает наличие онбординга и хорошей документации. За примером хорошего онбординга (без нудного code review по неделям) идем на Open-Source. Что у них есть? Чек-лист:

• Contributing.md – документ, в котором описаны самые верхнеуровневые шаги;
• Developer Docs – документы с описанием всех api-методов и вообще всего;
• Architecture Decision Records – история решений, принятых по поводу архитектуры. Очень помогает «въезжать» в процесс новичкам;
• Wiki с ответами на популярные вопросы;
• База данных задач и pull requests;
• И главное – понятные для людей тесты.

Если ваш онбординг построен как-то не так, то вам есть куда расти, потому что на Open-Source люди стремятся вот к этому.

Для вдохновения Никита предлагает ряд хороших мест:

1. Gatsby.js – одна из лучших и самых быстрорастущих по количеству контрибьюторов систем, которая описала вообще все;
2. Dev.to – сайт со статьями, документацией, видосами и т.д. У них тоже все прекрасно;
3. Wemake-python-styleguide – творение, к которому сам Никита относится, и по его словам «большое количество новых контрибьюторов и довольно мало ошибок».

Перейдем к обсуждению следующей проблемы — review архитектуры. Обычно оно корявое. Какие с ним могут быть проблемы?

• Долго. Когда мы говорим о том, что у нас есть review архитектуры, то это значит, что человек должен пойти и подумать, написать, все перерефакторить и через месяц прислать нам pull request. И как мы будем его ревьюить? Естественно, никак.
  
• Дорого переделывать.
• Участвует один человек.

Решение: проектировать и прототипировать design до review. Design до review – это когда вы делаете не саму архитектуру, а ее скелет. Текстом, или фиксируете в каком-нибудь файлике. Человек по скелету сможет сказать нравится\не нравится. Это намного быстрее и эффективнее, чем code review в данном смысле.

И, конечно, автоматизация! Никита рассказал о том, как можно автоматизировать архитектурные проверки.

В каждом приложении есть слои (тут, кстати, Никита проводил аналогию с офигенным бургером). И чтобы проверять их – есть все возможности. Далее примеры пойдут на языке python.

• Есть такая штука – называется importlinter. Она умеет проверять вашу архитектуру на основе ваших импортов. Когда вы делаете импорт, значит, эти два модуля связаны между собой. Эта связанность и проверяется. Как это делается? Указываете название вашего пакета, после этого вы определяете тип контракта. Все абсолютно декларативно, без кода. Это все текстовый файл. У нас есть такое то имя, вот такой-то тип контракта, который называется layers. И мы будем проверять пакет django_project. У него есть слои: urls, views, forms, models, logic. Соответственно, logic может импортировать – ничего. Models могут импортировать logics. Forms могут импортировать models и logic, и так далее.

• Мы можем проверять независимость наших модулей. Часто нам не просто важно определить, что пакеты лежат по разным слоям, но что они лежат на одном уровне. Делается все то же самое, что и в случае со связанностью.

• Проверка абстракций, которые не должны «перетекать». Например, у нас есть логика. И логика чистая, которая не знает ни про какие джанги, фласки или любые другие ваши пакеты. Она просто работает с какими-то абстрактными классами, функциями и т.д. Тогда выборочно запретите импортировать.

• Для более сложных случаев, когда вот здесь можно, а здесь – нельзя, а сверху еще требуются дополнительные проверки – напишите свой контракт. Есть api, вы пишите свой контракт – готово!

Таким образом можно ревьюить архитектуру автоматически.

Следующая проблема – «неповторение грабель».

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

В понимании нашего чудесного докладчика, хорошая документация – это когда вот так.

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

В ситуациях когда что-то сломалось, можно написать регрессионный тест. И выходит вот такая «документашка».

И теперь другая автоматизация! Часто люди делают ошибки не только в коде. Для решения этой проблемы есть замечательный проект – Danger. С ним схема будет выглядеть вот так:

На 2020 год danger-правило можно написать на следующих языках: JS, Swift, Ruby, Kotlin и Python. Никита в своем докладе привел примеры на JS.

«Валим CI» проверки:

• Pull request не в мастере, а надо в мастер.
• Pull request не мерджится из-за конфликтов.

«Предупреждающие» проверки:

• Pull request без описания.
• Забыли указать номер issue в теле.

Так это выглядит в итоге. Здесь исключено человеческое взаимодействие, все автоматизировано.

И еще одна фишечка – утилита bellybutton. Она есть для всех языков (проверено Никитой). Допустим, у вас есть некая deprecated_fn(), которую не надо вызывать. Она старая и странная, и существует просто потому, что так надо. Вы говорите человеку, чтоб не трогал ее, но он все равно трогает. Когда-нибудь надоест это повторять, и тогда вы берете декларативный файл формата yaml и пишете:

Декларативный файл можно поместить в репозиторий, описать там все правила для текущего проекта и больше человеку по code review не говорить об этом вообще никогда.

Контроль качества – разбираем еще одну проблему. Для человека, который пишет статические анализаторы, слышать фразу о том, что человек может контролировать качество – анекдот.

Почему все так? Во-первых, люди не умеют писать код. Загвоздка в том, что человек должен писать понятно и для другого человека, и для компьютера, а последние два говорят на разных языках. Отсюда и большая сложность. Во-вторых, эта сложность «быть понятным для всех и сразу» постепенно накапливается. В-третьих, скрытость метрик. Чтобы проверить качество кода, нужно сначала вывести метрики куда-то на экран, посмотреть все это дело, а затем еще долго исправлять. Именно поэтому проблема качества кода – это очень и очень серьезная проблема, и к ней нужно подходить с самого начала проекта. Решение: самый жесткий линтер из доступных. Чаще придется писать свой: под разные языки + существующий может быть недостаточно строг. Как говорит Никита: «чем больше вы ненавидите линтер, тем он лучше».

Проблема баланса кода и архитектуры. Тут могут возникать такие ситуации:

• При слишком сложной архитектуре приложения перестаешь понимать, что происходит и куда писать наш код;
• Архитектуры много, а кода – 20 строчек;
• Наоборот, кода много – отсюда сложность работы с ним.

Решение: Architecture on Demand. Оно позволит писать ровно столько архитектуры, сколько нужно. Простая архитектура и простой код под текущую ситуацию. Подробнее Никита этот способ разобрал здесь.

И last but not least – проблема контроля выполнения. Обычно говорят, что контроль выполнения – это когда code review, прогон всех тестов, запуск на компьютере, но нет. Перед нами встают проблемы:

• Я не умею запускать код в мозгу;
• Я не знаю, что нужно клиенту;
• Не все нюансы понятны из кода.

Решение: BDD (эффективный способ общения и с заказчиком, и с программистами, и с кем угодно) и Review Apps. Последнее нужно для «посмотреть глазками», потому что мало просто прогнать тест. При таком подходе вы сможете сделать ревью уже задеплоенного приложения. Например, использовать ZEIT для GitLab. Так появляется возможность получить на каждый pull request версию приложения.

Только тогда, когда вы прошли все тесты, клиент посмотрел и сказал, что это то, что нужно, вы сами посмотрели, что это работает, – можно сказать, что задача выполнена. В итоге получается, что это все не про code review. Код еще даже никто не смотрел.

А теперь подведем итоги. Были разобраны все обозначенные проблемы. Как можно увидеть, ни одна из них к code review отношения не имеет. Для каждой из проблем есть свое целевое решение. Зачем тогда нужно code review? Code review нужно для контроля корректности автоматики. Автоматики у нас полно. Она, естественно, сбоит. И нам нужно глазами проверять, что человек при ней сделал неправильно.

Инструменты, о которых мы говорили:

1. маленькие задачи (от 15 мин до 2ч., max – 4ч.);
2. review apps – чтоб контролировать исполнение;
3. суровый статический анализ, чтоб проверять качество кода и вообще все, что с ним связано;
4. строгие архитектурные правила, которые будут декларативно описаны, – чтобы четко понимать, где и что мы делаем;
5. глаза человека как самый последний рубеж. То есть пока не пройдет вся автоматика, на review код даже не попадает.

А нужно ли тогда код вообще смотреть, спросите вы? Ответ – конечно, ведь нужен контроль некоторых нюансов:

• корректность имен;
• проверка гипотез;
• проверка общей адекватности.

Вот это основные задачи code review. Все остальное вполне может решить автоматизация. При таком подходе «делание code review» превращается в «неделание», причем за 15 минут. И последние несколько слов от Никиты: «созерцайте ваш код, смотрите на то, как можно сделать лучше вашу кодовую базу и делайте это».

Заревьюить доклад своими глазками можно тут :)

А вы что думаете по поводу философского взгляда на code review?

P.S. Как организаторы DUMP`а хотим выразить благодарность Никите Соболеву за классный доклад :) Надеемся увидеться с ним, и с нашими участниками на DUMP 2020 в Екатеринбурге 20 ноября.