Что нужно и чего не нужно делать во время Code Review (код ревью)

Code Review могут быть спорными. Недавно я впервые выступала на конференции с темой токсичного поведения в культуре просмотра кода. Я готовилась получить большое количество критики, но в итоге тему приняли добротой и поддержкой.

Behaviors to avoid in code reviews:
— stating opinion as fact
— avalanche of comments
— asking devs to fix problems they didn’t cause
— judgemental questions
— sarcasm@sandyaaaas #AlterConf pic.twitter.com/LX1AeG4JOk

— Rachel Thomas (@math_rachel) December 10, 2017

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

Я перечислила несколько распространенных шаблонов поведения, которые случаются во время Code Review, а также несколько рекомендаций о том, как команды разработчиков могут создать более благоприятную атмосферу, если не будут делать токсичность нормой. Все описанное происходило со мной или я была этому свидетельницей. Я и сама вела так себя в прошлом.

Что нужно и чего не нужно делать во время Code Review

1. Представление мнения в качестве факта

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

Вместо того, чтобы говорить “В этом компоненте не должно быть состояний” предоставьте контекст этих рекомендаций:

“Так как в этом компоненте нет методов или состояний жизненного цикла, его можно сделать функциональным компонентом без состояний. Это улучшит производительность и читабельность. *Вот* документация.”

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

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

Это особенно важно, когда у вас более высокая должность или авторитет в команде. Если вы будете так себя вести, у разработчиков не останется другого выбора, и им придется подстраиваться под ваши требования.

2. Лавина комментариев

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

Объединение комментариев позволит вам донести свое сообщение и не ошеломить человека. Бесполезно и угнетает:

Более полезно:

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

3. Просить инженеров решить проблемы, которые возникли не из-за них

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

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

Правила, которые я выработал по результатам тысяч code review

4. Задавать осуждающие вопросы

Не задавайте вопросов вроде: “Почему ты просто не сделаешь это здесь?”. Такие вопросы предполагают, что это очевидное решение. Это также заставляет разработчиков оправдываться.

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

“Ты можешь сделать это, и это будет полезно поэтому.”

5. Сарказм

Сарказм — это неправильное поведение, когда вы даете обратную связь. Саркастические комментарии ничего не объясняют. Вместо этого детально опишите проблему и напишите рекомендации, но оставьте шутки в стороне.

Бесполезно: “Ты вообще тестировал(а) этот код?”

Полезно: “Происходит сбой при вводе отрицательного числа. Можешь, пожалуйста, разобраться с проблемой?”

Вот ещё один пример комментария, который несмешной и бесполезный:

Я не говорю, что мы подлые. Мы просто беспощадные. Ты заметишь, что я оставил комментарий “Бип!” на импорте каждого файла, к которому ты притронулся. Я имел в виду, что твои импорты нарушают нашу стандартную процедуру, но это было слишком долго писать в каждом файле.

Комментарий “Бип!” бесполезен. Это просто педантичный юмор, который не помогает человеку, который писал код.

6. Использование эмодзи, чтобы указать на проблемы

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

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

7. Ответ не на все комментарии

Люди, которые писали код, могут тоже вести себя некорректно. Если вы отправляете код без ответа на все комментарии, люди могут задуматься, зачем они вообще тратили время на помощь вам, а вы покажете, что некоторые мнения важнее других.

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

8. Игнорирование токсичного поведения производительных людей

Токсичное поведение не нужно игнорировать только потому, что разработчик отлично работает. Хотя он или она может делать фантастическую работу, важно помнить, что их поведение делает работу других людей сложнее.

О работе с людьми, ведущими себя токсично:

Всем остальным работать с этим человеком будет сложно и неинтересно. Они будут избегать взаимодействия с ними, даже если это негативно повлияет на их способность выполнять задания. Коммуникация в вашей организации нарушится. Если все станет плохо, ваша команда начнет искать возможности в другом месте. Пока вы будете сталкиваться с утечкой людей и неудачными проектами, эти разработчики продолжат работать так, как будто все в порядке. — Джозеф Гефрох.

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

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

Чистый код – красивый код

Полезные вещи в обзоре кода

Это рекомендации, которые можно применить к любому человеку в дружелюбной среде, даже если здесь они даны в контексте просмотра кода.

1. Используйте вопросы или рекомендации, чтобы вести диалог

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

“Можно поместить эти трансляции в файл констант, как думаешь? Их слишком много, имеет смысл создать для них отдельный файл.”

или предоставьте рекомендацию:

“У тебя в этом файле много запросов на трансляцию функции X. Имеет смысл создать отдельный файл под константы функции X.”

2. Не указывайте, а работайте вместе

Когда вы занимаетесь парным программированием, вы должны задавать вопросы, обсуждать и давать ссылки на ресурсы.

“Когда вы хотите работать с другим человеком, вы должны быть полностью вовлечены, а не просто появляться периодически”, — указания для пользователей Recurse Center.

3. Отвечайте на каждый комментарий

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

Например:

“ — Что ты думаешь о создании хелпера для этого вызова API?

— Эта строка не входит в мой набор изменений. Я пока отправлю этот код, но я создам отдельную issue на GitHub для вызова API и отправлю это в бэклог группы.”

4. Иногда обсуждение нужно перенести в оффлайн

После десятков противоречивых комментариев и предложенных решений должно быть понятно, что онлайн-коммуникация стала непродуктивной для решения проблемы. Устройте встречу, чтобы обсудить вопрос вживую, так вы сможете прийти к решению быстрее.

5. Используйте возможности, чтобы научить чему-то, и не хвастайтесь

Перед тем, как участвовать в просмотре кода, спросите себя — ваш комментарий помогает другому разработчику учиться или вы просто любите придираться?

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

6. Не выказывайте удивление

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

Григорий Петров: Код проекта: что хотел сказать разработчик

7. Автоматизируйте все возможное

Просмотр проблем, которые могут уловить линтеры, перехватчики в git или автоматизированные тесты, бесполезен. Люди не всегда видят эти проблемы, и поэтому существуют эти инструменты автоматизации.

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

8. Отказывайтесь от нормализации токсичного поведения

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

Григорий Петров: Как и зачем читать чужой код

9. Менеджеры, нанимайте внимательно, слушайте свою команду и применяйте меры

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

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

10. Установите стандарт с самого начала существования команды

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

11. Поймите, что вы можете быть частью проблемы

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

Одна последняя вещь…

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

Я знаю, что обратная связь важна, и я не объявляю ей войну. Я не прошу команды жертвовать качеством своего кода. Здоровая культура Code Review и высокое качество кода не исключают друг друга. Я надеюсь, что люди предпримут шаги, чтобы предоставлять конструктивную обратную связь и создать более благоприятную среду, где разработчики смогут учиться, расти и совершать ошибки.