Code review Терминатор. Ревью, за которое вам скажут спасибо


Рыжик помогает мне ревьюить код. А когда ему что-то не нравится — тоже настоящий Терминатор

«Code review Терминатор», — однажды назвал меня коллега после особо продуктивного ревью. С одной стороны, это тешило ЧСВ и было приятно. С другой — коллега действительно научился чему-то новому, и это позволило писать ему более качественный код. Так что win-win.

После смены работы сменились и коллеги. Но и на новом месте тоже начали благодарить за ревью. Решил разобраться, почему, и разложил по полочкам. Получилось 11 рекомендаций.

1. Относитесь к ревью, как к одной из главных активностей


Оставить парочку комментариев вида «А тут не хватает проверки на null» недостаточно. Надо:

  • Разобраться, что нужно было сделать
  • Понять, как это было сделано
  • Поразмыслить о других возможных подходах. Есть ли лучше (более читаемые, поддерживаемые, быстрые)?
  • Если текущий подход оптимальный, убедиться, что он реализован корректно. Иначе — предложить другой и объяснить, почему он может быть лучше.

2. Компенсируйте отсутствие невербальных сигналов


Одно дело — сидеть с коллегой рядом и что-то обсуждать, смотря в один экран. Другое дело — ревьюить код на гитхабе или битбакете. Легко неправильно понять намерения коллеги, когда общение происходит текстом. Возьмём фразу «В этой строке баг». Да, понятно, что фраза сообщает, что в статье баг. Но она могла быть сказана с разными эмоциями: злостью, удивлением, радостью о том, что баг отловлен и не попадёт на продакшн. А может, и с безразличием.

Ваш коллега может неправильно понять ваши намерения — а это ведёт к обидам, напряжению в команде и в целом фигово.

Сделайте себе жизнь проще: смягчите фразу, преобразуйте её в вопрос или, быть может, смайлик добавьте.



3. Выделите время


Чтобы убедиться, что код работает правильно и хорошо, вам нужно полностью его понимать. А на это нужно время; убедитесь, что выделяете его достаточно. Помните, что на ревью частей приложения, которые вы не знаете, понадобится ещё больше времени.

Вообще, это обратная сторона хороших ревью: это времязатратно. Если у вас нет времени, чтобы сделать его хорошо, попробуйте отложить или назначить кого-то другого (звучит как совет прокрастинатора, но ситуации бывают разные). Если это не вариант, а сделать ревью надо — помните, что вы жертвуете качеством. Хоть и необходимый, но компромисс. Если превратите его в привычку, то получите больше технического долга в перспективе.

4. Не делайте допущений; спрашивайте


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



5. Отлавливайте ситуации, когда нужно обратиться напрямую


Обычно ревью проводят асинхронно. Так можно и в код погрузиться, и коллегу меньше отвлекать. Но в некоторых ситуациях лучше напрямую обратиться (подойдя или позвонив):

  • Когда сроки поджимают. Быстрый фидбек ускоряет решения. Но аккуратно: при дедлайнах и так все на взводе, и отвлечения будут раздражать и сбивать концентрацию.
  • Грубые ошибки. Публичное их обсуждение может кого-то очень сильно смутить и расстроить. Лучше лично поговорить и разъяснить проблему. Может быть, это просто недосмотр, а может, пробел в знаниях — который теперь заполнен.
  • Выбран полностью неправильный подход. Говорить человеку о том, что нужно выбросить его работу, нужно аккуратно. Лучше использовать язык тела и интонацию, чтобы донести уточнения без обид.

Ну и хорошей идеей будет добавить потом в ревью-систему сводку разговора.

6. Прочитайте сначала тикет


Часть требований может быть не покрыта во вроде бы правильном пулл реквесте. Чтобы избежать такого — просто прочитайте сначала постановку задачи. Заодно это поможет в понимании того, что в целом в изменениях происходит.



7. Обоснуйте ваши предложения


Некоторые ошибки (опечатки, например), не нуждаются в объяснениях. Но если вы предлагаете альтернативное архитектурное решение или другое наименование — поясните преимущества и недостатки обоих вариантов. То, что кажется очевидным вам, может быть совершенно не очевидным другим людям.

Кроме того, есть пословица: «Дайте человеку рыбу, и вы накормите его на день. Научите его рыбачить, и вы накормите его на всю жизнь». Когда вы научили коллегу новому подходу, он сможет использовать его в будущем и писать код лучше. Заодно ещё и бонусом улучшится качество проекта.

8. Хвалите хорошие решения


Ревью не обязано быть перечислением ошибок. Если вы увидели хорошее место, интересное решение, полезный метод — скажите про это. Краткий комментарий «Классное решение» может улучшить человеку настроение на весь день. Да, и не хвалите плохие пулл реквесты: это натянуто и рушит смысл идеи.



9. Будьте вежливыми


Подсказка: фраза вида "Can we please get rid of the brain-damaged stupid networking comment syntax style?" не является вежливой (простите за английский тут, но это прямая речь Линуса Торвальдса и её было бы стрёмно переводить; он там красочно настаивает не использовать определённый вид комментариев, для вида вежливости добавляя «please»).



10. Помогайте


Если в процессе ревью вы проанализировали несколько файлов и в результате нашли альтернативное решение — скиньте коллеге ссылки на эти файлы. Можно даже с номерами строк, так ещё удобнее. Это не займёт много ваших усилий, а вот коллеге может сэкономить порядком времени.

11. Предлагайте, не указывайте


Когда вы на ревью предлагаете другой подход — не говорите в приказном порядке коллеге использовать ваш вариант. Аргументируйте и позвольте коллеге решить. Скорее всего, он примет ваш совет. А если нет, в чём может быть причина?

  • Оба подхода примерно одинаковы. Если нет объективных причин для выбора нового подхода, то нет причин тратить время и применять его. Disclaimer: единообразие кода — объективная причина.
  • Есть объективная причина, по которой ваш подход лучше. Но почему-то автор кода её не понимает. Пересмотрите вашу аргументацию, объясните ещё раз — а заодно проверьте, не ошибаетесь ли вы сами.
  • Личные конфликты. Это скользкий лёд и действовать тут нужно аккуратно. И это уже выходит за рамки темы ревью.



На этом всё. Суммируя:

Делайте мир вокруг вас чуточку лучше. Делайте хорошие ревью.



UPD. Эта статья — вольный перевод моего же оригинала на английском. Сконвертировал из «перевода» в «статью», чтобы не сбивать с толку читателей.
Ads
AdBlock has stolen the banner, but banners are not teeth — they will be back

More

Comments 15

    +2
    Классный текст. Жаль, что это не авторский материал.
      +13
      Авторский. Это перевод моей же статьи, но изначально опубликованной на английском. Хотел поделиться размышлениями и с хабрасообществом.
      Спасибо за тёплый фидбек!
        0
        Супер
      +3
      Не умаляя ценности статьи, от себя добавлю — советы по поводу корректного общения, например использование уточняющих вопросов вместо явных указаний на ошибки или похвалы удачных мест, они же напрямую с реальной жизнью коррелируют. Т.е. не только при проведении code review стоит вести себя уважительно и культурно по отношению к коллегам, это помогает поддерживать здорововые дружеские отношения в коллективе.
        +1
        Напомнило: habr.com/ru/post/340550
          +1
          Спасибо за статью. Мне кажеться, что многие разработчики недооценивают важность рекомендация №5. Я бы еще рекомендовал быть готовым выслушать идеи автора кода перед тем как разносить в пух и прах его код. Очень часто бывает, что автор предусмотрел случаи, про которые ревьювер не подумал.

          Can we please get rid of...

          для вида вежливости добавляя «please»


          Я всегда воспринимал эту фразу как «Господи, ну давайте уже избавимся от ...». Т.е. «please» в данном случае скорее выражает раздражение, чем вежливую просьбу.
            +1
            Советы немного странные — ну т.е. допустим 2. — вам просто зададут ответный вопрос — а какой класс ты посоветуешь? Далее тупо поправят на это и любая любая ошибка будет на ваш счет
            или «7. Обоснуйте ваши предложения» —
            Когда вы научили коллегу новому подходу

            вы как бы автоматически ставите себя выше коллеги. это будет работать если это действительно так и он сам с этим согласен, но если руководство подразумевает что вы на одной ступени, то такие «учения» сразу ставят вопрос — «кому следующему поднимать зарплату» не в его пользу
            Совет 11 вообще непонятен — ну т.е. вы что-то предлагаете, коллеге это влом делать, возникает вопрос — мы коммитим код в основную ветку или нет? за кем финальное слово? типовой ответ будет — «ну да, я знаю что неоптимально, но сделал так ибо..»
            или еще хуже — он с вами начнет обсуждать как лучше, какие варианты, в итоге 2 человека будут заниматься ерундой вместо решения задач проекта. По идее проектный менеджер, если у проекта ограниченный бюджет должен такое пресекать на корню.
              +1
              Думаю ваши замечания справедливы для большинства коллективов. Но это не здоровая ситуация. Хотелось бы избежать работы в таком коллективе, где люди вместо того чтобы учиться друг у друга будут переживать о том, что менеджер, скорее всего, читает их ревью и грезит сокращением премии.
            • UFO just landed and posted this here
                +1
                Я сильно привык писать комментарии на английском, и сначала внутренне не согласился, но поразмышлял — и да, это может быть оправдано во многих случаях. Общение на родном языке эффективнее, а на английский переходят для того, чтобы не было проблем при превращении команды в интернациональную.
                Но если комментарии в коде, внутренние вики (confluence, jira) и документация имеют долгосрочную ценность, то комментарии при ревью обычно сиюминутны и к ним (по крайней мере, в моей практике) почти не нужно обращаться в будущем. Конечно, бывают важные обсуждения и решения при ревью, но я предпочитаю сводку таких дискуссий переносить в таск-трекер или комментарий в коде.
                +1
                В статье есть важная фраза:
                Обычно ревью проводят асинхронно.

                и это действительно стало практически промышленным стандартом (причины — распределенная команда, разные часовые пояса и прочее). Как и классическая ситуация с огромным количеством изменений в долгоживущей (в которой разработка велась больше 2-х дней) feature ветке, которую ревьювят в пять раундов (либо смотрят поверхностно и не дают дельных замечаний в 2 раунда) — это может длиться неделю с перерывами из-за прокрастинации обоих участников (которая понятна — вся энергия итак потрачена на чтение кода для решения текущей задачи и переключаться на чужие или свои недельные простыни кода уже тяжело) и неважных споров по мелочам.

                Для меня было глотком свежего воздуха попробовать синхронное ревью, которое выглядит следующим образом:
                ш.1 коллега сбрасывает ссылку на merge request (в нем есть ссылка на задачу)
                ш.2 я с задеркжой 0-5-30 (если обед, созвон и проч) минут смотрю код и оставляю замечания — могу часть в личные сообщения, часть в сам merge request. Коллега к новой задаче НЕ приступает (не увеличивает незавершенное производство, нацелен на решение и продвижение текущей задачи)
                ш.3 (опционально) — мы созваниваемся с коллегой и проходимся по коду устно. Если есть возможность смержить код (несмотря на замечания) — он мержится (код закрыт feature toggle либо KeystoneInterface — подробнее у Martin Fowler). В любом случае если есть замечания — приступаем к следующем этапу
                ш.4 (опционально) коллега правит замечания, я в это время возвращаюсь к своей задаче (я НЕ увеличиваю количество незавершенное работы — так как я уже работал над текущей задаче перед ревью). Но я могу и подождать коллегу и не переключаться
                ш.5 возвращаемся к ш.2

                Важные условия:
                — размер merge request, у нас это ограниение заложено в рамках pipeline — <= 150 строк нового кода
                — continious integration. У нас строгий пайплайн и мы часто мержимся
                — коллега готов работать в таком режиме. То есть периодически переключаться на то чтобы посмотреть мелкие изменения (оказывается, переключаться чаще на небольшие изменения проще, чем редко на очень больше — также в этой технике не находится места прокрастинации). Обычно МР в этом случае проходит максимум в два раунда и ветка не живет больше 1-2 дней (ci/trunk based development)
                — в команде также практикуется парное программирование и парная спецификация задач (ревьювер и автор merge request находятся глубоко в одном контексте)

                В нашей команде 2 программиста, но говорят twitter.com/jezhumble/status/1260973883697401856 это масштабируется и на большие команды, тем более если можно дробить команды на более мелкие

                Конечно, синхронное ревью (как и парное программирование) звучит очень контринтуитивно в наше время прославления асинхронного общения. Но если есть возможность попробовать и у вас приятные и профессиональные коллеги — почему нет?
                  0
                  Когда все merge request'ы маленькие (а с помощью feature toggles этого можно достичь), то такой подход действительно интересен. Хотя частые переключения ведут к более частой смене контекста, и такое жонглирование может привести к снижению концентрации в течение рабочего дня и повышению усталости в конце его. Впрочем, это индивидуально и необязательно.

                  Про парное программирование — как-то взаимодействовал с командой из ThoughtWorks, широко использующей практики XP. У них, кстати, Martin Fowler работает. Сам работал в парном режиме с ними; видел, как они друг с другом работает. И сделал такой вывод: если у людей высокая квалификация и они умеют применять парное программирование правильно, то это прям эффективная техника. Иначе это может свестись к тому, что навигатор тупо сидит в телефоне, а драйвер всё молча делает сам, и тогда только вред от такого применения техники.

                  А долгое время жизни pull request'ов — это боль, да. Особенно, если потом ещё есть дополнительный шаг в виде manual QA check, который становится боттлнеком, если тестировщиков не хватает.
                    0
                    Все так, даже добавить нечего к вашему замечанию. Третий коллега с таким режимом работы не справился (точнее не смог адаптироваться) — но он и с обычным режимом не очень справлялся в других командах
                  0
                  Хороший ревью сейчас убережёт от технического долга в дальнейшем — это вы классно подметили
                    +1
                    Много дельных советов по поводу проведения код ревью, можно найти в этой публикации от гугл. Они вроде бы не открывают америку и многие советы перекликаются с теми, что даны в статье или комментариях, но некоторые моменты могут быть интересными.
                    Конечно там есть некоторые крайности, на мой взгляд, по поводу сроков, но видимо это от специфики команды зависит.

                    Only users with full accounts can post comments. Log in, please.