Code review по-человечески (часть 1)

https://mtlynch.io/human-code-reviews-1/
  • Перевод
  • Tutorial
В последнее время я читал статьи о лучших практиках code review и заметил, что эти статьи фокусируются на поиске багов, практически игнорируя другие компоненты ревью. Конструктивное и профессиональное обсуждение обнаруженных проблем? Неважно! Просто найди все баги, а дальше само сложится.

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

Моя революционная книга обучит вас проверенным техникам по выявлению максимального количества недостатков в своём партнёре. Книга не затрагивает следующие области:

• Обсуждение проблем с сочувствием и пониманием.
• Помощь партнёру в устранении недостатков.

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

Как вам нравится такая книжка? Предполагаю, что она вам не очень по душе.

Так почему мы проводим code review таким образом?

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

Я собираюсь сделать смелое предположение, что вам хотелось бы улучшить code review в настоящем, где вы работаете с людьми, а не с роботами. Даже более смелое предположение, что хорошие отношения с коллегами — это главная цель, а не просто переменная для ускорения исправления ошибок (минимизации параметра cost-per-defect). Как бы изменились ваши методы ревью с учётом этих обстоятельств?

В этой статье обсудим техники, которые предполагают, что code review — не только технический, но и социальный процесс.

Что такое code review?


Термин “code review” может означать разные действия, от простого чтения какого-то кода из-за спины разработчика до совещания на 20 человек, где вы разбираете код строчка за строчкой. Я здесь имею в виду формальную и письменную процедуру, но не отягощённую рядом совещаний для инспекции кода.


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

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

Ревью начинается, когда автор отправляет список изменений рецензенту. Оно происходит в несколько раундов. Каждый раунд — это один полный цикл приёма-передачи между автором и рецензентом: автор присылает изменения, рецензент отвечает отзывом на эти изменения. Каждая рецензия кода состоит из одного или нескольких раундов.

Ревью завершается, когда рецензент одобряет изменения. Обычно этому сопутствует отправка сообщения LGTM, сокращённой фразы “looks good to me”.

Почему это так трудно?


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

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

Филип Гринспан, сооснователь ArsDigita, из книги «Основатели за работой».

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

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

Техники


  1. Отдайте компьютерам скучную часть работы
  2. Оформите аргументы по стилю в виде руководства по стилю
  3. Начинайте ревью немедленно
  4. Начните с высокого уровня, и спускайтесь ниже
  5. Щедро используйте примеры кода
  6. Никогда не говорите «ты»
  7. Оформляйте отзывы как запросы, а не команды
  8. Обосновывайте принципами, а не мнениями

Отдайте компьютерам скучную часть работы


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

Очевидный пример — ошибки с пробелами в отступах. Сравните, сколько усилий требует выявление такой ошибки от человека, по сравнению с применением автоматического инструмента форматирования:

Что требуется при ручном редактировании Что требуется при использовании автоматического инструмента форматирования
  1. Найти лишние пробелы и неправильные отступы.
  2. Написать замечание с призывом правильно форматировать код.
  3. Перечитать своё замечание и убедиться, что оно сформулировано ясно, без намёков на обвинения.
  4. Автору нужно прочитать замечание.
  5. Автор исправляет отступы в коде.
  6. Рецензент проверяет, что автор всё корректно исправил.
.
.
.
.
.
.
.
Ничего!

Правая часть пустая, потому что автор использовал редактор кода, который автоматически форматирует пробелы каждый раз при нажатии кнопки «Сохранить». Ну худой конец, когда автор отправляет свой код на проверку, система непрерывной интеграции сообщает о неправильных пробелах. Автор исправляет проблему ещё до того, как рецензент её заметил.

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

Задача Автоматизированное решение
Проверить билды Система непрерывной интеграции, такая как Travis или CircleCI.
Проверить прохождение автоматических тестов Система непрерывной интеграции, такая как Travis или CircleCI.
Проверить, что отступы и пробелы соответствуют корпоративному стилю. Инструмент форматирования кода, такой как ClangFormat (для C/C++) или gofmt (для Go).
Идентификация неиспользуемых модулей или неиспользуемых переменных Статические анализаторы кода, такие как pyflakes (линтер для Python) или JSLint (линтер для JavaScript).

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

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

Нужно всем вместе поработать, чтобы внедрить автоматические проверки такого рода в рабочий процесс code review (например, хуки перед коммитами в Git или вебхуки в GitHub). Если процесс ревью требует от автора запускать такие проверки вручную, то вы лишаетесь большей части преимуществ. Автор неизбежно иногда будет забывать о проверке, из-за чего вам придётся возиться с с простыми ошибками, которые могла исправить автоматическая проверка.

Оформите аргументы по стилю в виде руководства по стилю


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



Хорошее руководство по стилю определяет не только внешние элементы оформления вроде соглашения о присвоении имён или правила отступов, но и как использовать функции данного языка программирования. Например, JavaScript и Perl набиты функциональностью — там есть много вариантов реализации одной и той же логики. Руководство по стилю определяет Один Правильный Способ программирования, так что вы больше не увидите, что одна половина вашей команды использует один набор функций языка, а другая половина — совершенно другой набор функций.

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

Вариант 1: адаптация существующего руководства по стилю
Если поискать в интернете, то можно найти опубликованные руководства по стилю — и позаимствовать их для собственного использования. Самые известные — руководства по стилю от Google, но можно найти и другие, если эти не подходят. Адаптируя существующий документ, вы получаете все выгоды от наличия руководства по стилю, не тратя времени на написание такого документа с нуля.

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

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

Я предпочитаю держать наше руководство в формате Markdown в системе контроля версия (например, на GitHub Pages). Так все изменения проходят через стандартную процедуру рецензирования — кто-то должен явно одобрить изменения, а каждый в команде может поднять любую проблему. Вики и Google Docs тоже подходят.

Вариант 3: Гибридный подход
Сочетая варианты 1 и 2, вы можете адаптировать существующее руководство и одновременно вести локальный документ для расширения или перезаписи базы. Хороший пример — руководство Chromium C++. Там в качестве базы взяли руководство по C++ от Google, но сделали собственные изменения и дополнения.

Начинайте ревью немедленно


Расценивайте code reviews как задачу с высоким приоритетом. Когда вы изучаете код или пишете отзыв, не торопитесь, но начинайте делать это немедленно — в идеале, в течение нескольких минут.



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

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

Представьте, что ваш коллега реализовал новую функцию, которая требует изменения 1000 строчек кода. Если он знает, что вы можете сделать ревью списка изменений на 200 строчек примерно за два часа, то разобьёт эту функцию на фрагменты примерно по 200 строчек и закончит ревью всей функции за один-два дня. Но если любые код ревью занимают у вас целый день, независимо от размера, то утверждение функции займёт неделю. Ваш коллега не хочет ждать целую неделю, так что он скорее направит вам более крупные списки, по 500-600 строк каждый. Их труднее рассматрвиать, да и отзывы станут беднее, потому что тяжелее сохранить контекст для изменения на 600 строк, чем на 200.

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

Начните с высокого уровня, и спускайтесь ниже


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

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

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

Щедро используйте примеры кода


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

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



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

Предположим, ваш автор не очень хорошо знаком с функцией списковых включений в Python. Он присылает на рецензию код с такими строчками:

urls = []
for path in paths:
  url = 'https://'
  url += domain
  url += path
  urls.append(url)

Ответ в стиле «Можно это упростить с помощью списковых включений?» вызовет раздражение, потому что ему нужно потратить 20 минут, изучая функцию, которую никогда раньше не использовал.

Автор будет гораздо счастливее получить такую заметку:

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

urls = ['https://' + domain + path for path in paths]

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

Применяйте эту технику только для ясных, бесспорных улучшений. В вышеприведённом примере спискового включения немногие разработчики будут спорить с сокращением количества строк кода на 83%. И наоборот, если вы напишете пространный пример для демонстрации изменения, которое «лучше» на ваш личный вкус (например, изменения стиля), то такой пример кода создаст впечатление, что вы настырно проталкиваете свои предпочтения, а не проявляете щедрость.

Ограничтесь двумя-тремя примерами кода на каждый раунд. Если начнёте писать код для всего авторского списка изменений, то это создаёт впечатление, как будто вы не считаете самого автора способным написать этот код.

Никогда не говорите «ты»


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

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

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

Например, вот безобидный комментарий:

Ты допустил ошибку в слове «благополучно».

Автор может интерпретировать такое замечание двумя соврешенно разными способами:

  • Интерпретация 1: Эй, приятель! Ты неправильно написал слово «благополучно». Но я всё равно считаю, что ты умный! Вероятно, это просто опечатка.
  • Интерпретация 2: Ты допустил ошибку в слове «благополучно», придурок.

Сравните это с замечанием, в котором отсутствует обращение «ты»:

sucessfully -> successfully

Вторая заметка — это просто исправление, а не оценка автора.

К счастью, можно легко переписать свои комментарии, избегая слова «ты».

Вариант 1: Замените «ты» на «мы»
Ты можешь дать этой переменной более наглядное имя, вроде seconds_remaining?
превращается в:
Мы можем дать этой переменной более наглядное имя, вроде seconds_remaining?

«Мы» усиливает коллективную ответственность всей команды за код. Автор может перейти в другую компанию, как и вы, но в том или ином виде останется команда, которая отвечает за этот код. Может показаться глупым говорить «мы», когда речь идёт об изменении, которое автор явно должен сделать единолично, но лучше выглядеть глупым, чем обвинителем.



Вариант 2: Удалите из предложения субъект
Другой вариант избежать личного обращения — использовать сокращение, которое исключает из предложения субъект:

Надо подумать о переименовании переменной для более наглядного имени, вроде seconds_remaining.

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

Эта переменная должна быть переименована для более наглядного имени, вроде seconds_remaining.

Ещё один вариант — перефразировать предложение в виде вопроса, который начинается со слов «Что насчёт...» или «Как насчёт...»:

Что насчёт переименовать эту переменную для более наглядного имени, вроде seconds_remaining.

Оформляйте отзывы как запросы, а не команды


Код ревью требует большего такта и осторожности, чем обычное общение, потому что здесь выше риск, что обсуждение скатится в личный спор. Казалось бы, рецензенты должны проявлять бóльшую вежливость и учтивость в код ревью по сравнению с личным общением. Но я обнаружил страннейшим образом прямо противоположную ситуацию. Многие люди никогда не скажут коллеге, «Дай мне этот степлер, а потом принеси газировки». Но я видел множество случаев, когда рецензенты оформляют отзывы в таком командном стиле, вроде «Перенеси этот класс в отдельный файл».

Но и не будьте раздражающе вежливым в своих комментариях. Оформляйте их как запросы или предложения, а не как команды.

Сравните одно и то же замечание, оформленное двумя разными способами:

Замечание, оформленное как команда Замечание, оформленное как запрос
Перенеси класс Foo в отдельный файл. Мы можем перенести класс Foo в отдельный файл?

Людям нравится контролировать свою работу. Такой запрос даёт автору чувство самостоятельности.

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

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

Отзыв, оформленный как команда (агрессия) Отзыв, оформленный как запрос (сотрудничество)
Рецензент: Перенеси класс Foo в отдельный файл.
Author: Я не хочу это делать, потому что тогда он будет слишком далеко от класса Bar. Клиенты почти всегда используют их вместе.
Рецензент: Мы можем перенести класс Foo в отдельный файл?
Автор: Мы можем это сделать, но тогда он будет слишком далеко от класса Bar, а клиенты обычно используют эти два класса вместе. Что думаешь?

Видите, насколько более цивилизованным стало общение, когда вы сконструировали воображаемый диалог в доказательство своего тезиса оформили замечание в виде запроса, а не команды?

Обосновывайте принципами, а не мнениями


Когда составляете замечание для автора, то объясните и предлагаемое изменение, и его причину. Вместо фразы «Нам следует разделить этот класс на два» лучше сказать «Сейчас этот класс отвечает одновременно и за скачивание файла, и за его парсинг. Следует разделить его на класс для скачивания и на класс для парсинга согласно принципу единой ответственности».

Если обосновывать замечания принципами, то дискуссия принимает конструктивную форму. Когда вы цитируете конкретную причину, вроде «Нужно сделать эту функцию приватной, чтобы минимизировать открытый интерфейс класса», то автор не может просто ответить «Нет, я предпочитаю сделать по-своему». А если он так ответит, то это будет выглядеть глупо, потому что вы показали, как изменение позволяет достичь цели, а он просто заявил о своём предпочтении какому-то способу.

Разработка программного обеспечения — это одновременно искусство и наука. Не всегда можно точно сформулировать, что именно неправильно в данном фрагменте кода с точки зрения устоявшихся принципов. Иногда код просто некрасивый или излишне усложнённый, и сложно точно сформулировать, почему. В таких случаях объясните как можете, но сохраняйте объективность. Если сказать «Мне кажется, это сложно понять», то это хотя бы объективное утверждение, в сравнении с фразой «Это запутанный код», что является оценочным суждением, с которым не каждый может согласиться.

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

Часть 2: скоро


Через пару недель я опубликую вторую часть статьи. Будьте на связи, там рассмотрим дополнительные советы, в том числе:

  • Работа со слишком большими код ревью.
  • Распознавание возможности дать высокую оценку.
  • Соблюдение рамок ревью.
  • Действия в безвыходных ситуациях.

UPD. Опубликована вторая часть статьи.
Поделиться публикацией
Похожие публикации
AdBlock похитил этот баннер, но баннеры не зубы — отрастут

Подробнее
Реклама
Комментарии 34
  • +2
    Code Review делается не для поиска багов. Да, во время ревью можно найти баги, но цель не в этом. Цель ревью в том, чтобы найти проблемные места. Те места, что могут работать не очень оптимально либо привести к проблемам в будущем. Так же, возможно, увидеть «велосипед», который нужно заменить на готовое решение. В средних и больших компаниях часто не все знают, что встреченная ими проблема уже была решена в соседнем отделе или группе.
    А для поиска багов есть отладка и тестирование.
    • +4
      А еще для шаринга знаний. Проще итеративно понять небольшие изменения, чем целиком вникать в логику незнакомого кода.
      • 0
        Code Review делается не для поиска багов.

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

        P.S. Результаты одного годного исследования эффективности разных способов поиска ошибок. Ревью — не худший способ.
        image
        • +1

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

          • 0

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

            • –1
              «использовать дорогих специалистов для поиска багов, видится мне не очень хорошей идей»
              ой сколько уже на эту тему сказано ))) Тестер это далеко не «недорогой специалист». А уж если все это через аутсорс, то и дороже разработчика. Да, при ревью нет прямой цели найти баги (может я ща откровение выдам, но при тестировании тоже нет такой прямой цели), но уж если видишь в чужом коде потенциальную ошибку, то надо про это как то сообщить и разобраться.
              • 0
                Тестер обычно в коде вообще не найдет ничего. Находят как раз топовые спецы, которые на порядок луче автора кода
            • 0

              А можете поделиться ссылкой на исследование? Интересно было бы узнать подробнее, как получены выводы и как их трактовать.

          • +3
            Котик и собачка тут классные…
            Хорошо, когда умеешь рисовать и даже скучные чеклисты/стайлгайды/списки советов смотрятся гораздо веселее и лучше усваиваются народом, когда они разбавлены забавными комиксами.
            • +1
              Кажется, что работа программиста превращается в работу психолога… Думаешь уже не о самом программировании, а о чувствах других людей)

              Я не говорю, что это плохо, это наоборот хорошо и по другому наверное никак. Просто как-то странно. В каких ещё технических профессиях такое встречается?
              • 0
                Только эмоционально туповатым людям становится тяжело, особенно когда в их сторону подобные усилия не прикладываются.
                • 0
                  Да о чем ты только подумал? Причем тут вообще программист и психолог? Это описание тонкостей нормального человеческого общения!

                  ну или же…

                  Действительно, любой род деятельности который подразумевает общение между людми требует соблюдения определенных правил, если мы конечно хотим чтобы общение было вежливым, эффективным и взаимовыгодным, а уж программисты там или электроники это действительно не важно.
                  • +1
                    У тебя ошибка в слове «людми»!
                    Ой, простите…
                    Мне кажется, что в слове «людьми» должен стоять мягкий знак. Нарушаются правила русского языка.
                    (применение на практике)
                    • 0
                      У проверяемого может появится чувство, что проверяющий сам дилетант и может предложить ему перекреститься.

                      Можно проще, сказать, что тут опечатка.
                  • +1

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

                    • 0
                      А в каких профессиях такое не встречается? Разве что на уровне рабочего на конвейере, где есть технологическая карта и всё расписано до шага.
                  • 0
                    Правильно написано про «отдайте компьютерам скучную часть работы», но забыли упомянуть что существуют еще и инструменты для проведения самого code review которые могут улучшить качество и ускорить процесс ревью.
                    • 0
                      Сконцентрируйтесь на проблемах вроде перепроектирования интерфейса класса или разбиения сложных функций. Подождите исправления этих проблем, прежде чем переходить к низкоуровневым вопросам, таким как именование переменных или понятность комментариев в коде.


                      Логично. Однако как быть, если новый сотрудник использует имена: k1, k2, ...,k23, soxranitsnimokekrana, rasparsitviragenie, scanirovatmatritzy и т.д.? Читать код с подобными именами будут затруднительно.
                      • +2

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

                        • 0
                          К сожалению, при найме невозможно выявить все особенности. Нужно разбираться в причинах. Если это была своеобразная «штука юмора», то можно надеяться, что больше она не повторится после серьезного предупреждения. Гораздо хуже, если у кадра проблемы с английским. В оправдание можно услышать, что взял код из сетки и не стал исправлять имена, пока код не утвердили. Но для примера я взял крайний случай, бывают менее яркие, но все равно неудобные для чтения. Отдельная проблема для наших фирм — язык комментариев. Многие резонно настаивают на английском: не надо каждый раз переключать язык, не возникает путаницы с переводом терминов. Но многие возражают: в программе м.б. специфика зоны .ru, ревью пишут на русском и да знания английского м.б. достаточное, чтобы читать инструкции и спецификации, но этого мало, чтобы свободно выражать свои мысли на этом языке.
                        • 0

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

                        • 0

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


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


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

                          • 0
                            Я заметил, что с возрастом становлюсь консервативен и нетерпим к чужим идеям при обсуждении. Когда у меня есть какой-то план или подход к вопросу, любые поправки и предложения вызывают отторжение и возмущение даже. Нужно некоторое время, чтобы обдумать встречное предложение и ответить без эмоций.
                            Обычно предложения при проверке оказываются хороши, и описанная тенденция расстраивает. Гибкость мышления уходит, ничего не сделаешь.
                          • +2
                            Спасибо за отличную статью! Разрешите предложить ещё одну крутую вещь, которую можно включить во вторую часть, хотя она больше относится к прохождению CR, а не проведению: раннее code-review. Например, спроектировали интерфейс — не надо сразу бросаться его реализовывать, покажите на code-review.
                            • 0
                              И еще вдогонку
                              Хотелось бы еще поинтересоваться опытом коллег по части немедленного проведения CR. С одной стороны всё правильно — блокируется работа коллеги, задача зависает перед отправкой на прод и т.д. Но с другой стороны переключение контекста может быть весьма дорогим и неприятным для разработчика, да и какая-нибудь рабочая встреча в самый неподходящий момент случится.

                              У себя в команде (8 разработчиков) мы ввели несколько правил:
                              1. ревьювить может любой разработчик, и аппрувы всех равны, независимо от опыта и длительности пребывания в команде;
                              2. для мерджа в мастер необходимо минимум два аппрува;
                              3. ни один комментарий не должен остаться без ответа: коммита с исправлением или достигнутого вместе решения что можно не править

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

                              Кто как находит баланс в этой проблеме?
                              • 0
                                Привет, у нас в команде 5 разработчиков и мы на ретроспективе выработали правило ревьюить задачи асапом (+ у нас есть правило не мержить с мастером, пока не получишь аппрув от всей команды). На второй ретроспективе мы обсудили наше правило и единогласно решили его оставить. Почему? Как итог атмосфера в команде стала много лучше, люди стали оформлять коммиты более качественно, ты начинаешь быть благодарным что так много людей уделяют внимание твоему коду, что делаешь все необходимое чтобы им было понятней. Также вовлеченность в продукт возросла, люди в большем контексте засчет постоянного ревью и ввиду того что наши задачи пересекаются мы стали быстрее вникать в чужой код
                              • 0
                                Как по мне, code review только тормозят процесс разработки. У каждого разработчика свой стиль, свои предпочтения, свое видение, и каждое ревью это столкновение двух миров где побеждает обычно более авторитетный или вышестоящий разработчик. Если несколько человек изменяют/ревьювят один и тот же код — они в конечном итоге переделывают его под себя, под свое видение один после другого. Не редко в практике бывают такие случае:

                                Разработчик 1 написал код / сделал дизайн
                                Разработчик 2 во время разработки смежного функционала, заметил код, WTF? Я бы сделал по другому, меняет код.
                                Разработчик 3 во время ревью добавил что-то от себя
                                Разработчик 1 через некоторое время возвращается к своему коду, WTF?

                                Нужно попытаться построить процесс так чтобы минимизировать их количество. Для этого разбить проект на слабосвязанные компоненты, модули, назначить ответственных за каждый модуль и пусть они комитят туда без каких-либо ревью — если компонента работает хорошо, не ломается, то разве не все-равно как она написана? Другое дело если сторонний человек вынужден сделать там какие-то изменения, то тогда ответственный за компоненту человек должен эти изменения ревьювить.
                                • 0
                                  Согласен, что есть много проблем с code review. И это может сильно тормозить процесс разработки. Но если компонента работает хорошо, то это не значит, что будет работать хорошо в следующей версии программы. Если разраб этой компоненты перейдет на другую работу, то другой м.б. вынужден сделать там какие-то изменения.
                                  • +1
                                    если компонента работает хорошо, не ломается, то разве не все-равно как она написана?

                                    Не всё равно. Через пару лет этот человек уволиться (или его переедет самосвал), а код останется. И оставшимся нужно будет его поддерживать: добавлять новый функционал, исправлять старые ошибки, допиливать под новые платформы. Если код нечитабельный, то уйдёт уйма времени на его чтение и понимание.
                                    В пределах команды (а желательно в пределах кампании) видение о том как правильно писать код должно быть одинаковым у всех разработчиков. Код написанный одним должен легко, как интересная книга, читаться другим. Code review в основном как раз для этого и нужен. Если вы видите непонятные имена, длинные функции, не влезающие в экран, запутанные алгоритмы, там где можно было бы обойтись простыми, отсутствие комментариев, там где они должны быть и т.д. — напишите об этом автору. Просматривайте чужой код так, будто вам завтра поручат исправлять в нём ошибки.
                                    Для синхронизации видения есть Coding Style Guide. Есть такой документ на всю кампанию и часто бывают дополнения к нему используемые внутри команды. CSG вырабатывается в результате обсуждения и голосованием выбирается тот вариант, который устраивает большинство. Вам придётся следовать этому документу, даже если он вас не устраивает.
                                  • 0
                                    Прочитал статью и почти в каждом предложении узнал коллегу, который проводит ревью моего кода. Подскажите, как незаметно подкинуть ему ссылочку?
                                    • 0
                                      Мы можем исправить грамматическую ошибку в слове «соврешенно»? =)
                                      • 0
                                        Вся эта толерантность и терпимость к ошибкам других, запрос с поклоном вместо четкой команды произрастает из внутренней атмосферы коллектива.
                                        Как самый верхний уровень компании ведет дела так всё происходит и ниже.
                                        • 0
                                          А уточните, пожалуйста кого вы считаете «самый верхний уровень компании»? Тот кто вам денежку платит или тот кто читает ваш код? Мне, с моим опытом, не попадался начальник которого интересовал бы мой «внутренний мир», начальника интересует что б ты делал работу качественно и вовремя, и что б у заказчика не возникало вопросов по поводу твоей работы. А вот тот кто читает твой код и тратит на тебя свое время, заслуживает уважение и взаимопонимания.
                                          • 0
                                            Снизу вверх скорее да, сверху вниз скорее нет. Рецензент должен быть выше по квалификации чем рецензируемый, поэтому все ужимки «не обидеть замечанием» неуместны.

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