Выжимаем максимум из код-ревью и спасаем нервные клетки

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

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

Небольшой дисклеймер: мысли в статье — результат исследования нашей команды мобильной разработки. Текст написан опытными разработчиками iOS-команды Surf, которые знают, о чём говорят.

Перед тем, как вы начали читать:

• да, здесь будут клише, которые все знают. Но мы про них всё равно расскажем;

• да, здесь будут любопытные предложения и мысли;

• да, будут и те, прочитав которые, вы скажете «детский сад!». Но суровая правда жизни показывает, что детского сада в нашей сфере становится всё больше и больше;

• будут даже такие, увидев которые, вы воскликните «Да это же бред! Это не сработает!»

Но просто попробуйте. Мысли из этой статьи проверялись на практике. При этом они подойдут не всем командам. Собранные здесь советы — далеко не самое главное. Запомните эту мысль — мы вернёмся к ней в конце статьи.

Так почему же разработчики не горят желанием проводить код-ревью? Разделим возможные причины на три сегмента:

• внешние и внутренние факторы;

• откровенно плохой код;

• и большая, но весомая, внутренняя причина под названием лень.

Внешние и внутренние факторы

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

Вопрос звучал так: «Что мешает приступить вам к код-ревью?»

А результаты были следующими:

Мы используем термин PR — pull requst, он же merge request. И исходим из того, что любой PR должен пройти код-ревью перед его «мёржем» в рабочую ветку.

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

Большой PR, большое количество изменений

Многие не хотят смотреть на большие изменения — большой diff —  такие PR могут быть сложными для восприятия.

Что делать автору PR, если всё-таки большое количество изменений неизбежно:

• если этот вопрос появился на этапе создания PR — сожалеем, но вы уже опоздали. Следовало разбить задачу на более мелкие и выкладывать каждую в отдельном PR (если это допускается на проекте);

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

• постарайтесь покрыть хотя бы часть кода тестами. Они покажут (если написаны правильно) надежность изменений — и уже меньшее количество ревьюверов будет бояться код-ревью;

• разбейте один PR на логические коммиты, чтобы ревьюер мог просмотреть каждую часть отдельно;

• не добавляйте в задачу сторонние правки — оставайтесь в рамках текущей задачи;

• делайте паузы во время работы, чтобы оценить объем изменений и понять, нужно ли разбивать PR на части.

Что делать ревьюверу? Поговорить с командой, попробовать изменить процесс, сделать его проще и удобнее. Ну, или страдать .

Но и страдать можно эффективно:

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

• бегло просмотрите те файлы, которые волнуют вас меньше всего. Например, файлы, созданные инструментами автоматизации или кодогенерации;

• начните с наиболее «низкоуровневых» компонентов системы, не имеющих зависимостей. Проанализируйте, всё ли вас в них устраивает;

• двигайтесь выше — проверяйте интеграцию данных компонентов друг в друга и в остальные части системы.

Незнание логики или фичи

• эта проблема частично решается качественным описанием PR. Пожалейте ревьюера и дайте подробное объяснение происходящего в PR — тогда код проверят быстрее. Как это сделать — расскажем дальше;

• называйте коммиты со смыслом. Например, «исправлена анимация во view», а не просто “fix”. Это добавит понимания написанному коду;

• добавляйте блок-схемы в описание PR, если они могут лучше объяснить происходящее. Небольшая схема, написанная от руки на клочке бумаги, сфотографированная на телефон и приложенная к описанию PR, может сильно помочь ревьюверу;

• опишите, за что отвечают изменения в конкретных файлах.

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

Более приоритетные задачи

Решение только в общении. Стоит обсудить эту проблему с командой, лидом, менеджером, product owner-ом. Чтобы понять, почему другие задачи приоритетнее, чем код-ревью, и отнимают всё время.

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

Коллеги считают, что их задачи более важны, чем код-ревью? Расскажите им, что ваши задачи не менее важны. И намекните, что все мы люди, и если человек избегает код-ревью, то, возможно, люди будут избегать код-ревью его кода в ответ. Начнутся взаимные обиды, конфликты, что приведёт к затягиванию сроков, а атмосфера в команде уже не будет столь дружелюбной и открытой. Не надо так!

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

Недостаточное описание PR

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

• если внутри команды нет договоренностей о том, что необходимо указывать в описании PR — и эти договоренности не оформлены в виде шаблона для описания PR — бегом делать!

• если ссылка на задачу в описании PR не добавляется автоматически, то бросайте всё и добавляйте скрипт!

• дополните описание видео или скриншотом того, что получилось, или того, что ничего не сломалось;

• описывайте, как проверять задачу. Куда идти, куда нажимать, что вводить, включать ли feature toggle и какой, на каком стенде, и так далее;

• приложите файлы подмен и патчей — не мучайте команду выяснением происходящего. Они быстрее проверят код и не захотят вас убить;

• укажите причину бага —  в самом баге написано только как его воспроизвести.

• укажите все части приложения, которые задеты правками;

• укажите, что не решалось в рамках задачи, чтобы не было вопросов. Например, «был найден баг в процессе работы, но на него заведём отдельную задачу». И добавьте TODO с номером задачи;

• если PR в статусе WIP (work in progress, он же Draft) — добавьте причину постановки такого статуса. И обязательно обозначьте PR, если он подвис и его не надо смотреть;

• добавьте ссылки на ТЗ или дизайн, особенно если их нет в задаче.

Много PR ждут моего апрува

Тут придётся разобраться в причинах сложившейся ситуации. Например:

• слишком раздутый скоуп в спринте — вы бросили все силы на решение задач, а не на проверку PR (тут смотрите пункт «более приоритетные задачи»).

• вы проводите код-ревью несколько раз в неделю или реже. Практика показывает, что на код-ревью стоит выделять хотя бы час в день, даже если горят сроки;

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

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

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

• свежий взгляд на PR помогает найти различные подводные камни, а значит найти и исправить больше недочетов на стадии работы над задачей;

• участвуя в код-ревью, вы знакомитесь с изменениями в проекте и кодовой базой, а значит, вам станет проще ориентироваться в проекте;

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

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

Self-review

Другая причина, по которой команда может отлынивать от процесса код-ревью, — приходится смотреть очень много откровенно сырого кода. Здесь решение может быть только в авторе кода. И очень эффективное воздействие показал подход self-review, в простонародье «посмотри свой код сам, прежде чем его кому-то показывать».

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

Да, в больших компаниях часть из этих советов покрывается требованиями обеспечения практически 100% покрытия тестами. Но давайте честно — далеко не все пишут даже unit-тесты, не говоря уже о snapshot или UI-тестах.

Перед созданием PR вернитесь в описание задачи и ТЗ

• проверьте, что не упустили детали, и всё работает так, как должно. Особенно актуально, если задача насыщенная и вы делали ее этапами. Будет полезно проверить как все работает вместе;

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

Проверьте, одинаково ли хорошо ваш код работает на разных версиях устройств или браузеров с разными разрешениями экранов?

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

Сверьтесь еще раз с дизайном

Отступы, цвета, все варианты вёрстки. Не поленитесь, проверьте, что всё отображается так, как задумано.

А что насчёт отладочного кода?

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

Проверьте код повторно

• оформлен ли он согласно принятым на проекте правилам? Code style никто не отменял, ведь одинаково написанный код позволяет облегчить мозгу восприятие информации и пустить высвободившийся ресурс на что-то более полезное;

• проверьте названия переменных и методов. Соответствуют ли они логике, за которую отвечают? Однозначно ли передают то, что выполняют? Не введут ли в заблуждение стороннего человека?

• подумайте, точно ли вашему коду не требуются комментарии?

• посмотрите, не передаете ли вы избыточную информацию между компонентами системы?

• простота — наш друг! Проверьте, нет ли лишних отрицаний при проверках? Разумно ли вы используете тернарные выражения — или из них растут ветвистые деревья?

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

Помечайте правки на будущее TODO с номером задачи

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

Не надо ругаться (в коде)

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

Обращайте внимание на то, что говорят скрипты

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

Посмотрите на результаты своего труда как пользователь

Ответьте честно: даже если весь написанный код удовлетворяет требованиям ТЗ, было бы вам комфортно пользоваться плодами своего труда? Не будет ли потом стыдно? Вполне вероятно, что обнаруженные неудобства — проблема не вашего кода, а «рядом» с ним. В любом случае, подсветите это ревьюверам и QA, возможно, это сэкономит кому-то время.

Вы бы стали смотреть такой PR?

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

Уважайте CI

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

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

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

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

Что делать с ленью?

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

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

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

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

Попробуйте проводить код-ревью поэтапно

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

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

Проверьте реализацию с точки зрения пользователя

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

Запускайте код на разных симуляторах и устройствах

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

Сравните реализацию с ТЗ

Человеческий фактор никто не отменял, и небольшая деталь (и даже большая) могла затеряться.

Посмотрите на код с точки зрения возможного расширения

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

Если у вас нет времени на тщательное ревью, сосредоточьтесь на определенных аспектах кода

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

Смотрите изменения по коммитам, а не целиком

PR из 3 коммитов: в 1-м верстка новой view, во 2-м добавлены новые параметры в запрос, в 3-м новая view выводится в нужном месте экрана с полученными данными. Безусловно, не у каждого PR будет такая  идеально подходящая структура, но, если звезды всё-таки сошлись, — то подход может оказаться очень полезным.

И немного мыслей напоследок

Все мы пониманием, почему нужно проводить код-ревью и на что обращать внимание в рамках этого процесса. Что надо смотреть на соблюдение архитектурных подходов, на следование принципам программирования, на соблюдение code style проекта, на применение общепринятых на проекте механик. Но очень редко можно встретить человека, способного дать более глубокий ответ на вопрос «что нужно проверять на код-ревью»?

Представьте, что вы решили построить дом, сделали проект, наняли бригаду строителей, нашли технадзор и приступили непосредственно к этапу строительства. Бригада произвела земельные работы, приступила к монтажу блоков ФБС (фундаментный блок сплошной — да-да), закончила эти работы и… оказывается, что будущее строение стало на 10 см шире.

С одной стороны кажется, что 10 см — ерунда. Согласимся, ничего страшного нет, всё исправимо. С другой стороны:

• изменились размеры стен, и теперь необходимо договариваться о том, какую именно часть дома необходимо увеличить и как изменится расстояние от оконных проемов до осей плана;

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

• необходимо внести корректировки в проект кровли, особенно над той самой входной группой;

• и теперь придется совсем капельку переплатить практически за все материалы, ведь бетона в монолитной плите в основании дома стало больше. Объём стен, кстати, тоже увеличился, площадь кровли увеличилась, площадь утепления плит перекрытия увеличилась. И это только на этапе тёплого контура. А впереди вас ждут еще материалы для отделки полов и стен внутри дома, а ещё материалы для внешней отделки фасадов.

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

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

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

Вторая — это то, что должно быть проведено проектирование и сформирован план, с которым и нужно сверяться на код-ревью.

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

Кейсы и лучшие практики в области системной и бизнес-аналитики, новости, вакансии и стажировки Surf — в телеграм-канале Surf Tech.

Присоединяйтесь!