Не так давно мой коллега переводил интересную статью о code review, перевод хабражителям понравился. А сегодня утром запутанный граф кроссылок вывел eyeofhell на еще более крутую статью. Вашему вниманию предлагается перевод краткой, но емкой инструкции о том, как делать review чужого кода и пережить review собственного. В отличие от упомянутой выше статьи, эта больше фокусируется на практических аспектах code review и содержит множество полезных рекомендаций как и что делать, чтобы не было мучительно больно. Хинт: чтобы почитать оригинал, кликните на имени автора в плашке под переводом.Для всех участников code review
- Примите как данность, что многие архитектурные решения — это вопрос личного вкуса разработчика. Старайтесь обсуждать только сильные/слабые стороны и как можно быстрее принять решение что делать с кодом.
- Не требуйте, а задавайте вопросы. Например: «Что ты думаешь по поводу использования имени идентификатора :user_id?». Примечание переводчика: этот совет основан на наблюдениях, что человеческий мозг склонен выдавать сильную эмоциональную реакцию на критику и требования, в то время как приглашение к обсуждению часто проходит мимо эмоциональной составляющей и позволяет спокойно общаться о коде.
- Не стесняйтесь уточнять непонятные моменты. Например: «Не могу понять, что тут происходит. Можешь чуть подробнее объяснить?».
- Старайтесь избегать разделение кода на свой/чужой. С подозрением относитесь к словам «мой», «не мой», «твой».
- Обсуждайте программу, а не программиста. Старайтесь избегать перехода на личности и не используйте выражения вроде «тупое решение» или «идиотский код». Проводите code review так, как будет все члены команды — умные, интеллигентные люди с благими намерениями :)
- Старайтесь явным образом выражать свои мысли. Люди не всегда способы понять ваши намерения online.
- Старайтесь быть скромнее, например: «не уверен что это будет работать, давай проверим?».
- Не гиперболизируйте и по возможности избегайте таких слов как «всегда», «никогда», «ничего» итд.
- Сарказм тоже не очень хорошая штука.
- Старайтесь быть собой, как бы заезжено не звучала эта фраза. Если emoji, анимирнованные gif'ы и юмор это «не ваше» — то не стоит себя насиловать, стараясь быть похожим на других членов команды.
- Если в обсуждении встречается слишком много заметок «я не понял» и «есть другой путь решения этой задачи», то хорошей идеей будет обсудить этот код лично, а затем записать краткий follow up в рамках code review.
Если ваш код подвергся code review
- Хорошим тоном считается поблагодарить ревьювера за проделанную работу, например: «спасибо что нашли эту интересную ситуацию, мы поменяем логику завершения работы».
- Старайтесь не принимать результаты code review близко к сердцу. Обсуждают код, а не вас.
- В спорных ситуациях старайтесь объяснить, с какой целью был написан этот код. Ответ на вопрос «зачем?» упрощает коммуникации.
- Хорошей идеей будет вынести объемные исправления в отдельные задачи, а не складывать все в одно место.
- Сделайте ссылку на code review из соответствующей задачи, например: «готово, можно ревьювить: github.com/organization/project/pull/1».
- Не объединяйте коммиты (squash), пока ревью не закончено. У ревьюверов должна быть возможность посмотреть на индивидуальные коммиты, которые исправляют отдельные вопросы по коду.
- Попробуйте понять точку зрения ревьювера.
- Хорошим тоном считается ответить на каждый комментарий.
- Не нужно делать merge, пока не пройдены все тесты continuous integration.
- Merge рекомендуется делать только если вы уверены в коде и его влиянии на проект.
Если вы хотите подвергнуть code review чужой код
В первую очередь постарайтесь разобраться зачем был написан этот код. Это багфикс? Новая фича? Рефакторинг? Кровавое месиво из всего вышеперечисленного? Затем:
- Старайтесь явным образом указывать в каких замечаниях вы полностью уверены, а в каких — не очень.
- Подсказывайте способы упростить код без потери функциональности.
- Если обсуждение кода уходит в философские или академические дебри, то такое обсуждение хорошо вынести в оффлайн. Например, на традиционные пятничные посиделки. В вашей команде же есть пятничные посиделки?
- Предлагайте альтернативные решения, но исходите из предположения что автор уже их попробовал. Например: «что думаешь по поводу кастомного валидатора?»
- Попытайтесь понять точку зрения автора кода.
- В конце code review пометьте pull request комментарием «можно мерджить».
Замечания по оформлению кода
Если вы видите, что код явным образом нарушает принятый в команде стиль кодирования — нелишним будет указать на это во время code review. Если окажется, что кто-то в команде не согласен с таким стилем, то обсуждение рекомендуется выносить в отдельный тикет.
Комментарии (18)
aml
07.12.2015 10:49+1А я бы и по поводу архитектурного дизайна докапывался. Это как раз то, что «отливается в граните» навеки. Неудачное решение лучше переделать сразу, чем дать ему укорениться в проекте.
Автор кода должен был сначала все продумать и обсудить архитектуру с коллегами, провести дизайн-ревью. На этом этапе все поменять было бы элементарно. И только потом садиться писать код.
batmandarkside
07.12.2015 10:54+1Примите как данность, что многие архитектурные решения — это вопрос личного вкуса разработчика. Старайтесь обсуждать только сильные/слабые стороны и как можно быстрее принять решение что делать с кодом.
aml
07.12.2015 11:24-3Может быть мы о разных вещах говорим. Я имею в виду не вкусовщину, а именно ошибки проектирования архитектуры. Например, «смешивание модели данных с их представлением затруднит в будущем написание мобильного приложения». Можно сразу автора «обрадовать» и попросить переделать, а можно дать ему закоммитить это безобразие, а потом кому-то привалит «счастье» распутывать всё.
eyeofhell
07.12.2015 11:12+3ИМХО, это зависит от того, кому этот код дальше поддерживать и развивать. Если автору, то в большинстве случаев «родная» архитектура для него будет ближе и понятнее, чем «чужая» или плод коллективного разума. Конечно, есть вырожденные случаи когда все плохо. Но в таких случаях тоже можно не про архитектуру написать, а про то что именно плохо. Например: «мы ожидаем до 10кк пользователей, это решение не будет работать, если пользователей больше 10к, см. потребление памяти». И пусть сам думает что с этим делать :)
aml
07.12.2015 11:26Разумеется замечания должны быть конкретными, показывающими, что сломается, испортится, будет неудобно, слишком сложно и т.д… Если нет резонных аргументов, то на нет и суда нет.
aml
07.12.2015 10:53+8И ещё дополнение. Если ревьюеру что-то не понятно в коде, автор должен не лично ревьюеру это объяснить, в комментах к ревью и т.д., а либо упростить код и сделать его более понятным, либо привести текстовые объяснения прямо в коде в виде комментариев языка программирования. Ибо если ревьюер чего-то не понял, то и следующие N человек, кому надо будет разобраться, тоже не поймут.
Scf
07.12.2015 14:25Говорят, в Google такой подход. Там ревьюверы часто отклоняют пулл реквесты с формулировкой «я ничего не понял, переделать»
aml
07.12.2015 14:42+5Комментарий в таком духе — это, в первую очередь, неуважение к автору кода. Я такого ни разу не встречал. Обычно все комменты очень логичные и содержательные. Если что-то конкретное непонятно, то спрашивается, что именно. Например: «почему ты здесь делаешь X, если в других аналогичных местах везде делается Y?» И автор, если он действительно делал X намеренно, может оставить комментарий в коде: «Здесь X, потому что Z». Тогда следующий читатель кода не будет голову ломать, почему. Как-то так.
knitevision1
07.12.2015 15:39+2Ну а если код непонятен настолько, что ревьюер даже ничего комментировать не хочет, это мало того, что неуважение к ревьюеру и отвержение надобности код ревью, так еще и проблемы в будущем для проекта.
Неуважение надо отличать от кнута. Не знаю как остальным, мне, например, очень не нравится, когда меня кто-то «хвалит». Гораздо приятнее, когда получаешь пинка хорошенького и заставляешь себя переделывать что-то в лучшей форме. Это на пользу каждому.
Например на codereview.stackexchange.com часто вижу такие вопросы, заминусованные сообществом, потому что ни черта не ясно вообще. Это не неуважение, это нежелание тратить свое время на разбор кода либо слабого инженера, либо инженера, который не удосужился позаботиться о читаемости и понимании кода.
К тому же, обычно ревьюверы — куда более опытне инженеры/архитекторы, и я вполне представляю себя на месте вышеуказанных гуглеров, которые не могут понять, зачем тратить время на ревью плохого/непонятного кода.
matiouchkine
07.12.2015 16:39-2Это же типичный вопрос про курицу и яйцо, только здесь на него есть однозначный ответ.
Мне код на PR не с альфа-центавры доставляют, автор — вон он сидит, в пяти метрах. И если вместо того, чтобы позаботиться о будущем проекта в целом и здоровья рассудка команды в частности, он вывалил какую-то кашу из своего подсознания и тут же нажал кнопку «готово для ревью» — он проявил неуважение первым. Поэтому если я проскроллил один экран и перестал понимать, о чем это вообще, я отправляю задачу на доработку именно с такой пометкой, только еще короче: «Unreadable.»
«Уважение» — это такой зверь, который никак не зависит от того, что кто-то в какой-то момент может написать плохой код. И уж подавно уважение — не синоним толерантности. Я вообще очень люблю повторять «fuck your ego».
madkite
07.12.2015 20:59+2Unreadable — понятие относительное. Если потакать такому поведению — ревьювер может облениться и даже не напрягаться понять. И если какой-то ревьювер ничего не понял, это совсем не значит, что код плохой. Читая исходники ядра FreeBSD я тоже пока мало что понимаю, но это совсем не означает, что код плохой. Если кто-то не понимает, например, алгоритм Ахо-Корасика, то это совсем не означает, что этот алгоритм плохой.
evgenyk
07.12.2015 23:17+3Fuck нельзя говорить никому и никогда. Ну, может только если ногу сломал и никто не слышит.
Ivan22
Еще из моей практики — очень важно оформление кода. Если ревью — неотъемлемая часть процесса, то нужен и стандарт оформления кода, иначе ревьювер просто на эмоциональном уровне уже может быть «отрицательно заряжен» так сказать.
glagoleva
Угу, eyeofhell даже статьей на эту тему разродился некоторое время назад :)