[Перевод] Что я узнал после более чем 1000 code review
За последние 3 года я рассмотрел более 1000 pull (merge) request«ов. За это время я многому научился — в основном тому, как не проверять код, как сделать процесс менее болезненным, что делает код хорошего качества и так далее.
Pull request должен делать только одну вещь
Это самая важная вещь, на которую стоит обратить внимание.
Делая code review, вы должны держать в голове много вещей. «Что за этим стоит?», «Как это согласуется с остальной частью кода?» и «Будет ли это хорошо работать?» Вот лишь некоторые из вопросов, на которые нужно ответить. Таким образом, когда у вас есть pull request, который пытается решить одну проблему, на некоторые из этих вопросов легче ответить.
Другим важным аспектом является размер pull request«а. Большие запросы требуют экспоненциально больше времени для рассмотрения. И когда я узнаю, что мне нужно потратить более 15 минут на запрос, вам придется подождать до пары часов.
Более крупные запросы также содержат больше ошибок, поэтому время на утверждение также значительно возрастет. Это означает, что у вас может быть код, ожидающий подтверждения в течение нескольких дней. И если ваша компания гибкая, это увеличивает шансы на конфликты, которые очень болезненны.
Лучше всего разбить pull request«ы на осмысленные части — один запрос должен решить только одну вещь.
Так что следите за частыми ловушками, такими как переименования, обобщения кода и тому подобное. Хотя он невиновен и сделан с добрыми намерениями, он убирает внимание с важных частей — повышать качество кода и уменьшать количество ошибок. Просто сделайте свою гениальную идею в другом запросе.
Автоматизируйте как можно больше проверок
Продолжая мысль о том, что рецензент должен держать в голове много вещей, включая проверку форматирования кода, наличие соответствующей документации, проходимые тесты и так далее. Я помню время, когда мне приходилось принимать все это во внимание — это отвлекало и отнимало много времени.
Хорошо, что есть довольно простое решение — интегрируйте все проверки в CI. Тогда у вас будут запускаться все проверки всякий раз, когда кто-то отправляет pull request, и не разрешает слияние пока все проверки не пройдут успешно. Вам как рецензенту больше никогда не придется беспокоиться о форматировании.
Автоматические тесты помогают убедиться, что автор ничего не сломал, и что тесты все еще проходят. В зависимости от вашего подхода к тестам, это, пожалуй, самая часть в работе вашей CI.
Автоматическое форматирование кода приводит к тому, что все споры вокруг идеальной длины строки или места добавления новой строки исчезают. Просто решите командой какой набор правил форматирования вам подходит и передайте его автоформатору. Это избавит вас от многих неприятностей. Если у вас возникли проблемы с согласованием формата, посмотрите, как это делают Google, Facebook или другие крупные компании.
Автоматический линтер также очень важен для таких языков, как Python, где форматирование кода влияет на логику кода. Большинство форматеров кода для Python форматируют только тот код, который, как они знают, точно не повлияет на логику. Добавив линтер, вы решите все скрытые проблемы.
Также заслуживают упоминания проверки типов и документации, но самое главное — автоматизировать проверки, которые имеют смысл для вас и вашей команды. Если вы думаете, что что-то поможет, обсудите это и попробуйте в течение недели или двух.
Сядьте с автором, чтобы просмотреть код
Это ускоряет процесс рецензирования, поскольку вы можете быстрее перебирать код с автором и делиться своей точкой зрения. Автор также может лучше объяснить причину своего подхода, например, если он уже что-то пробовал и почему это не сработало.
Это также может быть отличной возможностью больше общаться с людьми, что очень важно. Когда ваши коллеги-разработчики увидят, что вы инвестировали в их личный и деловой рост, они это оценят. Для вас и автора это также прекрасная возможность попрактиковаться в общении, что крайне важно для хорошо функционирующих команд.
Будьте осторожны, чтобы не переусердствовать, хотя. Некоторые pull request«ы слишком малы и слишком просты, чтобы вести какой-либо эффектный разговор. В этом случае встреча с автором может быть пустой тратой времени.
Будьте внимательны при написании отзывов
Если вы более опытны, чем автор кода, вы должны принять во внимание, что то, как вы говорите, имеет большое значение. Хорошо написанная критика может помочь разработчику стать лучше в будущем, но она также может разрушить чьи-то мечты.
Я обнаружил, что лучше всего задавать открытые вопросы — это не агрессивно и даже побуждает разработчика мыслить критически. Это займет больше времени, чем просто рассказать кому-нибудь решение? Да, краткосрочно. Но в долгосрочной перспективе вы помогаете им расти, и они с меньшей вероятностью повторят свои ошибки.
Итак, в следующий раз, когда кто-то откроет файл внутри цикла for, а не перед ним, вместо того, чтобы просто указать на это, спросите: «Как бы вы уменьшили сложность здесь?». Это будет много значить.
Добавьте флаг «Я запускал этот код локально»
Больше всего меня беспокоит обнаружение ошибки в небольшом запросе, из-за которого функциональность вообще не работает. Это означает, что разработчик даже не запустил код — вероятно, думая, что в этом нет необходимости, поскольку изменения настолько малы.
После того, как это случилось пару раз, я добавил флаг под названием «Я запускал этот код локально», что полностью решило проблему. Я прекратил просмотр кода, который не запускался локально, и люди не лгали об этом. Не было обид, потому что мы все знали, что это нужно делать.
Бонус: это наш шаблон, который должен заполнить каждый разработчик при создании pull request«a:
Описание Merge Request«a
Что нового?Реализовано…
Что изменилось?Изменено…
ЧЕК-ЛИСТ
[] Я запускал этот код локально
[] Я написал необходимые тесты
[] Я покрыл свой код тайп хинтами
[] Я обновил CHANGELOG
Трелло карта
trello.com/cДолжен знать о
Есть что-нибудь еще, что должно быть известно?
Любые замечания по развертыванию?
Любая дополнительная документация?
Узнайте подробности, как получить востребованную профессию с нуля или Level Up по навыкам и зарплате, пройдя платные онлайн-курсы SkillFactory:
