Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
Когда речь заходит об инспекции кода, внимание людей обычно сосредоточено на том, кто её проводит. Но разработчик, который писал код, играет в процессе такую же важную роль, как и проверяющий. Рекомендаций по подготовке кода к инспекции почти не встречается, поэтому авторы часто допускают ошибки просто по незнанию.
В этой статье собраны лучшие практики для участия в инспекции кода в роли автора. По завершению чтения вы начнёте отсылать инспекторам такие безупречные наборы изменений, что они воспылают к вам любовью, не меньше.
Но я не хочу, чтобы инспекторы кода пылали ко мне любовью
А они всё равно будут. Смиритесь. Никто в истории человечества ещё не сетовал на смертном одре, что при жизни его слишком уж любили.
Зачем совершенствовать инспекцию кода?
Освоение эффективных техник инспекции облегчит жизнь проверяющему, команде и, самое-то главное, вам.
- Ускоренный процесс передачи знаний. Если как следует подготовить набор изменений, внимание инспектора будут притягивать именно те области, в которых вам нужно расти, а не скучные стилистические огрехи. А когда вы даёте понять, что цените конструктивную критику, проверяющий начинает вкладывать больше сил в обратную связь.
- Пример для окружающих. Эффективные практики инспекции кода в вашем исполнении зададут планки для окружающих. Хорошие техники обычно перенимаются, а значит, вам будет проще, когда кто-нибудь из коллег вышлет код вам на проверку.
- Минимум конфликтов. Инспекция кода часто вызывает напряжение в команде. Если подходить к делу ответственно и сознательно, споры будут возникать реже.
Золотое правило: цените время проверяющего
Это вроде бы очевидно, но мне часто встречаются авторы, которые относятся к инспекторам как к своим личным специалистам контроля качества. Такие люди палец о палец не ударят, чтобы отловить хоть часть ошибок или сделать набор изменений удобным для проверки.
Ваши коллеги каждый день приходят на работу с конечным запасом внимания. Если какую-то его часть они уделяют вам, то на собственные задачи потратить этот ресурс уже не могут. Сделать так, чтобы их время тратилось с максимальной отдачей – вопрос простой справедливости.
Проверка кода проходит в разы успешнее, когда участники могут друг другу доверять. Ваш инспектор не будет жалеть сил, если знает, что вы серьёзно отнесётесь к его замечаниям. Если же вы воспринимаете проверяющего как препятствие, которое нужно преодолеть, то получите от него значительно меньше ценных рекомендаций.
Техники
1. Сначала просмотрите код сами
Прежде чем отсылать код коллеге, прочитайте его. Не ограничивайтесь вылавливанием ошибок – представьте, что видите этот фрагмент впервые. Что в нём может сбить с толку?
По моему опыту, бывает полезно сделать перерыв между написанием и редактированием кода. Люди часто отсылают свежую порцию кода под конец дня, но именно в этот период наиболее высока вероятность что-то упустить по небрежности. Дождитесь утра, посмотрите на набор изменений свежим взглядом и тогда уж передавайте его коллеге.
Какой идиот это написал?
Синхронизация Cron Jobs с лунным циклом: Я добавил логику с синхронизацией, чтобы наш пайплайн ETL оставался в гармонии с природой
Сымитируйте рабочую среду проверяющего насколько это возможно. Используйте ту же утилиту для сравнения, что и он. Глупые ошибки в таких утилитах отыскиваются лучше, чем в вашем обычном редакторе кода.
Не ожидайте от себя совершенства. Вы неминуемо отправите код с куском, который забыли удалить после устранения бага, или приблудным файлом, который собирались убрать. Это не конец света, но всё-таки такие вещи стоит отслеживать. Подмечайте закономерности в своих ошибках и прикидывайте, какие можно внедрить системы для их предупреждения. Если одинаковые ошибки проскальзывают очень часто, инспектор придёт к выводу, что вы не цените его время.
2. Составьте ясное описание набора изменений
На последней работе в рамках программы наставничества мне периодически устраивали встречи с более опытным программистом. Перед первой встречей он попросил меня захватить документ с описанием дизайна ПО, который я составил. Подавая ему бумаги, я пояснил, что это за проект и как он соотносится с целями моей команды. Мой наставник нахмурился и сказал без обиняков: «Всё, что ты сейчас говорил, должно быть на первой странице документа».
И он был прав. Я писал документ с расчётом на разработчиков из своей команды, но не учёл, что его, возможно, будут читать и люди со стороны. Аудитория не ограничивалась пределами моего отдела – оставались ещё партнёрские команды, наставники, люди, которые принимали решения о повышениях. Документ следовало писать так, чтобы и им всё было ясно. После этого разговора я всегда стараюсь подать свою работу в необходимом для понимания контексте.
Описание набора изменений должно включать в себя всю фоновую информацию, которая может понадобиться читателю. Даже если вы составляете описание прицельно для проверяющего, имейте в виду, что у него может быть меньше сведений о контексте, чем вам кажется. Кроме того, не исключено, что другим коллегам тоже придётся работать с этим кодом в будущем – при просмотре истории изменений им должны быть понятны ваши намерения.
Хорошее описание в общих чертах сообщает, для чего служит набор изменений и почему вы решили действовать именно так.
Если вы хотите глубже погрузиться в искусство составления превосходных описаний, прочитайте “How to Write a Git Commit Message” Криса Бимса и “My favourite Git commit” Дэвида Томпсона.
3. Автоматизируйте простые вещи
Если вы полагаетесь на инспектора в таких мелочах, как уплывшая фигурная скобка или проблемы с автоматическими тестами, значит тратите его время впустую.
Проверишь, всё ли там в порядке с синтаксисом? Я бы к компилятору обратился, но жаль тратить его время.
Автоматические тесты должны считаться в команде частью стандартной процедуры. Инспекция начинается только тогда, когда пройдена вся серия автоматических проверок в среде непрерывной интеграции.
Если же ваша команда пребывает в трагическом заблуждении и отказывается от непрерывной интеграции, организуйте автоматизированную проверку своими силами. Внедрите в среду разработки pre-commit хуки, линтеры и форматтеры, чтобы удостовериться, что все соглашения соблюдены и предполагаемое поведение сохраняется от коммита к коммиту.
4. Позаботьтесь о том, чтобы код сам отвечал на вопросы
Что не так на этой картинке?
Мне непонятно назначение функции.
А, это на случай если при вызове передаётся Frombobulator при отсутствии имплементации frombobulate
Мне автор помог разобраться в функции, а как быть следующему читателю? Закопаться в историю изменений и перечитывать все обсуждения? Ещё хуже, когда автор подходит к моему столу, чтобы объяснить всё лично. Во-первых, это мешает мне сосредоточиться, во-вторых, ни у одной живой души больше не будет доступа к озвученной информации.
Когда инспектор выражает замешательство по поводу какого-то момента в коде, ваша задача состоит не в том, чтобы прояснить ситуацию конкретно для того человека. Прояснить ситуацию нужно для всех сразу.
— Алло?
— Когда ты шесть лет назад писал bill.py, почему у тебя t=6?
— Рад, что ты позвонил! Это из-за налога на продажи в 6%.
— Ну разумеется!
— Отличный способ обосновать решение по реализации.
Лучший ответ на любой вопрос – это рефакторинг кода, который его снимает. Что можно переименовать, чтобы стало понятнее, как изменить логику? Комментарии – приемлемое решение, но они однозначно проигрывают на фоне кода, который естественным образом сам себя документирует.
5. Вводите изменения дробно
Разрастание границ – это дурная практика, которую часто можно наблюдать при инспекции кода. Разработчик берётся за устранение бага в логике, но по ходу дела ему на глаза попадается какой-то изъян в интерфейсе. «Ну, раз уж всё равно с этим куском работаю», — думает он, — «заодно и его исправлю». А в итоге начинается путаница. Проверяющему теперь придётся вникать, какие изменения работают на задачу А, а какие – на задачу Б.
Лучшие наборы изменений делают что-то одно. Чем точечнее и проще изменение, тем легче инспектору удерживать в голове контекст. Разделяя не связанные друг с другом изменения, вы также получаете возможность организовать параллельную проверку несколькими коллегами, а значит, код вернётся к вам быстрее.
6. Отделяйте функциональные изменения от нефункциональных
Из ограничения масштабов следует другое правило – функциональные и нефункциональные изменения не должны смешиваться.
Разработчики, у которых мало опыта с инспекцией кода, часто нарушают это правило. Они вносят какое-то изменение с разбивкой строки, и редактор кода тут же меняет формат по всему файлу. Разработчик либо не улавливает, что произошло, либо решает, что так даже лучше, и отправляет код, где одно функциональное изменение погребено под сотнями строк с нефункциональными.
А вы найдёте функциональное изменение?
Подобная неразбериха – как плевок в лицо проверяющему. Изменения, которые касаются только формата, проверять легко. Функциональные изменения – тоже. От одной функциональной разбивки в океане изменений формата можно исчахнуть и свихнуться.
Вдобавок разработчики любят несвоевременно примешивать к основным изменениям рефакторинг. Рефакторинг кода со стороны коллег я всячески одобряю, но не когда они параллельно реализуют новое поведение.
Изменение в поведении, погребённое среди рефакторинга
Если фрагмент кода нуждается и в рефакторинге, и в изменениях поведения, необходимо разбить процесс на два-три набора:
- Добавить тесты для текущего поведения (если их нет)
- Провести рефакторинг кода, ничего не меняя в тестах
- Изменить поведение и соответствующим образом обновить тесты
За счёт того, что тесты на втором этапе остаются нетронутыми, проверяющий может убедиться, что рефакторинг ничего не нарушает в поведении. Соответственно, на третьем этапе ему не придётся разбираться, что здесь рефакторинг, а что работает на изменение поведения – ведь вы отделили одного от другого заранее.
7. Разбивайте слишком крупные наборы изменений
Раздутые наборы изменений – это что-то вроде страшненьких родственников разрастающихся границ. Представьте себе ситуацию: разработчик приходит к выводу, что для внедрения функциональности А ему сначала придётся подкорректировать семантику библиотек Б и В. Если изменений немного, то ещё куда ни шло, но очень часто эти модификации расползаются и на выходе получается огромный список изменений.
Сложность списка изменений возрастает по экспоненте с увеличением числа затронутых строк. Если изменения затрагивают 400 и больше строк, я ищу возможность разбить их на порции, прежде чем запрашивать инспекцию.
Может, вместо того чтобы делать всё разом, удастся сначала поработать с зависимостями, а уже в следующем наборе реализовать новую функциональность? Реально ли сохранить код в здравом состоянии, если внедрить половину функциональности сейчас, а другую половину – на следующем этапе?
Раздумывать, как бы разбить код, чтобы получился рабочий, вразумительный фрагмент, бывает утомительно. Но это позволяет получить более качественную обратную связь и создаёт меньше сложностей для проверяющего.
8. Принимайте критику доброжелательно
Самый верный способ испортить всю проверку – принимать замечания в штыки. Удержаться бывает трудно, ведь многие разработчики гордятся тем, что делают, и воспринимают свою работу как продолжение себя. А если инспектор отпускает бестактные комментарии или переходит на личности, то это ещё сильнее усложняет дело.
В конечном счёте, вы, как автор, вольны решать, как именно вам реагировать на замечания. Относитесь к словам инспектора как к объективному обсуждению кода, которое не имеет никакого отношения к оценке вашей личности. Если вы займёте оборонительную позицию, станет только хуже.
Я стараюсь принимать любые замечания как попытки помочь и принести пользу. Если проверяющий находит в коде глупую ошибку, меня инстинктивно тянет начать оправдываться. Но я удерживаюсь и вместо этого хвалю его наблюдательность.
— Для января и февраля 1900 года это не сработает.
— Точно, хорошо подмечено!
Как ни странно, то, что инспектор обнаруживает в коде неочевидные недочёты – хороший знак. Это говорит о том, что вы хорошо готовите файлы к проверке. В отсутствие небрежного форматирования, неудачных названий и других бросающихся в глаза вещей проверяющий может глубоко изучить логику и дизайн и высказать более ценные наблюдения.
9. Проявляйте терпение, если инспектор допускает ошибку
Периодически случается, что проверяющий просто-напросто не прав. Он может неверно прочесть совершенно нормальный код с таким же успехом, как вы – насажать багов. Многие разработчики в таких случаях начинают яростно защищаться. Их оскорбляет, что кто-то подвергает их работу критике, да ещё и ни на чём не основанной.
Однако даже при том, что проверяющий не прав, вам есть о чём задуматься. Если он что-то неверно прочёл, какова вероятность того, что в будущем и другие люди допустят ту же ошибку? Не придётся ли читателям изучать код вдоль и поперёк, только чтобы убедиться, что бага, который им мерещится, здесь всё-таки нет?
— Здесь переполнение буфера, потому что не проводилась верификация, хватит ли памяти в name, чтобы вместить все символы NewNameLen.
— Это в моём-то коде? Немыслимо! Конструктор вызывает PurchaseHats, а она вызывает CheckWeather, которая выдала бы ошибку, если бы длина буфера не соответствовала. Ты бы сначала прочитал 200 000 строк кода, а потом уже осмеливался допускать возможность, что я где-то ошибся.
Подумайте о том, как провести рефакторинг, или добавьте комментарии, чтобы с первого взгляда можно было понять: здесь всё в порядке. Если недоразумение вышло из-за использования малоизвестных возможностей языка, перепишите код с использованием механизмов, которые не требуют досконального знания.
10. Давайте доходчивую реакцию
Я часто попадаю в такую ситуацию: отправляю человеку замечания, он обновляет код с учётом некоторых из них, но при этом хранит молчание. Наступает период неопределённости. То ли он пропустил остальные замечания, то ли ещё в процессе исправления. Если начать проверять уже сделанное, гипотетически может оказаться, что этот фрагмент кода ещё в работе и я зря теряю время. Если выжидать, можно попасть в патовую ситуацию – будем оба сидеть и ждать друг от друга следующего шага.
Заведите в команде определённый порядок действий, чтобы всегда была ясно, у кого в данный момент «эстафетная палочка». Нельзя допускать, чтобы процесс затормаживался только из-за того, что никто не понимает, что вообще происходит. Сделать это легко – достаточно оставлять на уровне набора изменений комментарии, из которых понятно, когда право действовать передаётся другому участнику.
Исправляю заголовки > Обновляю код в соответствии с замечаниями > Всё готово, посмотри, пожалуйста
На любое замечание, требующее от вас каких-то действий, следует отреагировать, подтверждая, что вы их предприняли. Некоторые инструменты для инспекции кода дают возможность отмечать обработанные комментарии. Если такого варианта нет, используйте набор простых текстовых маркеров вроде «Сделано». Если вы не согласны с замечанием, вежливо объясните, почему решили воздержаться от исправлений.
Некоторые инструменты вроде Reviewable и Gerritt предлагают ставить метки на обработанные замечания
Соразмеряйте свои реакции с усилиями, которые приложил проверяющий. Если он подробно расписал какой-то момент, чтобы вы могли почерпнуть что-то новое для себя, не ограничивайтесь пометкой «Сделано». Отвечайте вдумчиво, чтобы показать, что цените его труд.
11. Умело запрашивайте недостающую информацию
Иногда комментарии инспектора оставляют слишком много возможностей для толкования. Когда тебе пишут что-то в духе «Мне непонятна эта функция», сразу возникает вопрос, что именно человеку непонятно. Слишком длинная? Название неудачное? Нужно лучше задокументировать?
Долгое время я ломал голову, как внести ясность в туманные комментарии и при этом ненароком не вступать в конфронтацию. Первое, что приходило в голову – спросить: «Что в ней непонятного?», но это звучит сварливо.
Как-то раз я умышленно отправил неопределённую реплику такого рода коллеге, и он обезоружил меня своим ответом:
«Что изменить, чтобы стало лучше?»
Эта фраза нравится мне тем, что выражает открытость к критике и отсутствие враждебности. Теперь, когда я получаю туманную обратную связь, то отвечаю какой-нибудь вариацией базового мотива «Что изменить, чтобы стало лучше?».
Другая полезная техника – попытаться угадать, что имел в виду проверяющий, и взять на себя инициативу, исправив код соответствующим образом. Если вы получили замечание в духе «непонятно», просмотрите код повнимательнее. Обычно что-нибудь, что можно улучшить в плане ясности, находится. Правки дают инспектору понять, что вы готовы вносить изменения, пусть даже он представлял себе итоговый результат иначе.
12. Отдавайте преимущество проверяющему
В теннисе, если не совсем ясно попал ли мяч в аут при подаче противника, принято трактовать ситуацию в его пользу. При инспекции кода тоже следует придерживаться этого правила.
Некоторые решения в разработке – дело вкуса. Если проверяющий считает, что вашу функцию в десять строк лучше бы разделить на две пятистрочных, объективная истина не принадлежит ни вам, ни ему. Какой вариант лучше – зависит от предпочтений.
Если инспектор вносит предложение и аргументы в поддержку своей позиции у вас примерно равнозначны, примите его точку зрения. В конце концов, из вас двоих преимуществом свежего взгляда на код обладает именно он.
13. Сокращайте время передачи кода
Несколько месяцев назад один пользователь внёс небольшое изменение в проект с открытым кодом, который я поддерживаю. Я отписался с замечаниями спустя несколько часов, но он уже исчез с концами. Через несколько дней я вернулся и убедился, что ответа по-прежнему нет.
Прошло шесть недель и таинственный разработчик снова возник в проекте с правками. Хоть я и ценю его труд, но большой перерыв между двумя этапами проверки привёл к тому, что я потратил в два раза больше сил на инспекцию. Пришлось перечитывать не только код, но и собственные комментарии, чтобы вспомнить о чём вообще шла речь. Если бы человек появился через пару дней, в дополнительной работе не было бы необходимости.
Интеллектуальные затраты на инспекцию кода при коротких (слева) и долгих (справа) сроках передачи
Ось x — время; ось y — память инспектора; синяя штриховка — восстановление контекста; голубая заливка — проверка кода; серая заливка — ожидание; красная штриховка — дополнительные издержки
Шесть недель – это, конечно, исключительный случай, однако мне часто приходится наблюдать долгие, ничем не оправданные паузы у себя в команде. Кто-нибудь отсылает набор изменений на проверку, получает обратную связь и неделю откладывает правки на потом, потому что отвлёкся на другую задачу.
Помимо того, что потом придётся тратить время на восстановление контекста, полуготовые фрагменты кода порождают сложность. Из-за них становится труднее отслеживать, что уже слито, а что ещё нет. Чем больше разводится частично обработанных списков изменений, тем выше вероятность возникновения конфликтов при слиянии, а уж с ними никто не любит возиться.
Как только вы отправили код на инспекцию, приоритетной задачей становится довести дело до конца. Если процесс буксует по вашей вине, то вы крадёте время у проверяющего и усложняете жизнь всей команде.
В заключение
Готовя следующий список изменений к отправке, подумайте, на какие факторы можете воздействовать и используйте эти возможности, чтобы проверка прошла продуктивно. В процессе участия в инспекции кода отмечайте те вещи, которые регулярно замедляют процесс и впустую тратят ресурсы.
Запомните золотое правило: цените время проверяющего. Если вы дадите ему возможность сосредоточиться на по-настоящему интересных сторонах кода, его комментарии будут дельными. Если же вы вынудите его тратить время на мелочи или распутывать путаницу – тем хуже для вас обоих.
И наконец, хорошо обдумывайте свои ответы. Недопонимания из-за пустяков или бездумные слова могут пустить всё под откос с пугающей быстротой. Когда дело доходит до критики чужой работы, начинают бурлить страсти, так что тщательно избегайте всего, что может навести проверяющего на мысль, что вы на него нападаете или относитесь к нему без должного уважения.
Поздравляю! Если вы дочитали до этого места, то можете уже считать себя экспертом по инспекции кода. Ваши проверяющие, скорее всего, уже воспылали, так что и вы обращайтесь с ними по-человечески.
