Как выжить на Code Review?

Тебя попросили провести первый code review и ты не знаешь, как это сделать? На что стоит обращать внимание при просмотре чужого кода? В какой форме следует писать комментарии? Или, может быть, твой код будет проверен другими и ты не знаешь, чего ожидать и как реагировать? На эти темы мы поговорили с Евгением Алуевым, разработчиком ПО.

— Что такое code review?

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

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

— Как code review происходит на практике?

— Эта практика может принимать разные формы в разных организациях и командах. Это может быть неформальная просьба к коллеге за соседним столом, чтобы тот посмотрел через плечо на код на экране и сказал, что он о нем думает. С другой стороны, это также может быть полностью формализованный процесс, который использует специальный инструмент в системе контроля версий (например, "merge request" на GitLab) и требует от человека, просматривающего коммиты, разблокировать возможность слияния.

— Зачем делать code review?

— Стоит ли внедрять практику code review в том или ином проекте, следует решать индивидуально, учитывая характер проекта и знание преимуществ и недостатков этой практики. Среди преимуществ первое, что приходит на ум, — это улучшение качества кода. Дополнительная пара глаз на каждый новый или измененный фрагмент кода — это возможность оценить его с другой точки зрения, найти и сообщить о любых ошибках до того, как они попадут в репозиторий. Это могут быть логические ошибки или неправильно обработанные «exceptions» (например, когда входные данные пусты), утечки памяти, проблемы с производительностью или даже уязвимости безопасности.

Однако есть и другие, менее очевидные преимущества code review. Внедрение этой практики позволяет как минимум двум людям видеть каждую деталь и более-менее знать код. Это приводит к желаемому распространению знаний в каждой команде вместо того, чтобы запирать каждого инженера в «бункере» с задачей, которой он занимается. Благодаря этому, кому-то еще при необходимости в будущем будет проще вернуться к работе с данным кодом.

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

— Мотивирует ли наличие code review?

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

— Много достоинств. А как же недостатки?

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

— Есть ли противники code review? Какие аргументы они выдвигают?

— Аргумент, часто выдвигаемый противниками включения code review в процесс, заключается в том, что ошибки в коде лучше находить с помощью хорошо написанных и регулярно выполняемых тестов или ручного тестирования программы отделом QA, а также других автоматических инструментов, таких как статический анализ кода. Это, конечно, правда, так же, как верно и то, что человек, читающий код, не будет «проверять» в своей голове столько возможных случаев, сколько хорошие тесты.

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

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

— Как наличие code review влияет на время разработки?

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

— Что советуешь? Делать или воздержаться?

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

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

— Чем нужно руководствоваться при проведении code review?

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

В крупных организациях также существуют собственные правила, дополняющие или изменяющие стандарты языка. Например, язык требует делать отступ в 2 символа при переносе части строки и 4 символа для каждого внутреннего блока. А в компании может по умолчанию использоваться стандарт другого языка, где порядок этих величин определен иным образом. Также возможно наличие установленных правил не только на уровне организации, но и на уровне проекта или команды. Обычно это письменный документ “coding standard”, который предписывает определенный способ именования переменных (camelCase, PascalCase или snake_case) или обработки ошибок в коде (выбрасываем ли мы исключения или возвращаем числовой код ошибки) и прочего.

Процесс code review позволяет определить соответствие таким правилам.

Ибо конкретный работник может быть ленив, неквалифицирован или действовать по какому-то одному ему известному умыслу. В результате получаем странный код, который трудно читать и изменять. Есть даже такое понятие, как “code smells”.

— Какие качества нужны человеку для code review?

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

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

В этой выдуманной истории проекту трудно развиваться и быть “up-to-date’ с технологией и тенденциями. И никому не хотелось бы оказаться в таком проекте. Особенно когда мы просматриваем чужой код, мы не хотим быть «Биг Беном». Поэтому стоит начать с азов и подумать, какой психологический подход подойдет при проведении проверки кода.

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

На практике это означает лишь следующее: не привязываться к своему коду и не принимать комментарии по этому поводу на свой счет. Помни, твой код — это не ты!

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

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

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

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

— Кто должен делать code review?

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

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

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

— Как делать code review?

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

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

Единственное отличие здесь в том, что при code review у нас есть четкое разделение на те функции или классы, которые являются новыми (добавлены автором ревью кода) и теми, которые остались неизменными в коде. Мы можем даже не увидеть последних в ПО, которое показывает только сами изменения, но иногда нам нужно дотянуться до них, чтобы иметь полное представление обо всей архитектуре.

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

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

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

— Это сложно?

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

— Сколько сотрудников должны делать процесс ревью?

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

— Существует ли какая-то иерархия среди ревьюеров?

— Она скорее возможна только на уровне ответственности, которая выражается в количестве плюсиков или пунктов, которые может поставить ревьюер. Например, для успешного добавления кода в репозиторий каждый merge request должен набрать 2 пункта. Малоопытный ревьюер может поставить только один пункт, а опытный — сразу два. Создатель риквеста тоже может поставить пункт, но он не будет засчитан.

А иногда ревьюеры ставят флажок LGTM (Looks good to me), а самый опытный работник может сделать финальный аппрув. Предполагается, что первые сделали поверхностную (предварительную) проверку.

— Какие комментарии делает ревьюер?

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

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

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

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

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

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

— Бывает ли увлеченность улучшениями чрезмерной?

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

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

— Как софт скиллы помогают в работе?

— Прежде всего, при написании комментариев к чужому коду самое главное — смирение. Мы никогда не должны быть уверены на 100%, но всегда допускаем, что можем ошибаться. Поэтому хорошо начать с высказывания: «По моему мнению…», «Я думаю, что…» и т. д. В конечном итоге мы выражаем здесь свое мнение — особенно если что-то субъективно («по моему мнению, лучше было бы поместить эту функцию в класс»).

Во-вторых, утверждение «потому что (…)» — это место для обоснования. Стоит подкрепить свое мнение некоторыми аргументами. Если мы видим ошибку, давайте напишем пример входных данных или значений переменных, для которых результат будет неверным. Если, по нашему мнению, вызов функции не соответствует спецификации данной библиотеки, давайте вставим адрес документации, объясняющей ее правильное использование. В случае языка C или C++ и его стандартной библиотеки это может быть, например, адрес конкретной страницы сайта cppreference.com. Если данный код кажется несовместимым с общепринятыми передовыми практиками программирования, возможно, стоит даже вставить адрес примера веб-сайта, найденного в Интернете, чтобы подтвердить свой тезис о том, что данная языковая конструкция не рекомендуется.

В-третьих, хороший комментарий содержит конкретное предложение по изменению: «Не лучше ли написать так: (...)?» Стоит быть позитивным и не только указывать на проблему, но и предлагать возможное решение или решения. Вместо просто: «Имена переменных i, j нечитабельны», добавим еще: «Может быть, лучше называть их group_index, element_index?». Некоторые системы, например Gerrit, позволяют оставлять комментарий как suggestion. И если автор кода с ним согласен, то он его применяет к своему коду одним кликом.

Наконец, предложение «Не лучше ли написать так: (...)?» неслучайно принимает форму вопроса. Эту форму стоит использовать, поскольку она не только звучит более вежливо, но и поощряет ответы и дальнейшее обсуждение. Если окажется, что мы ошиблись, что-то не учли, код правильный или по каким-то другим причинам его автор не согласен с нами и не желает его менять, то для него будет естественным ответить на такой вопрос следующим образом: «Нет, потому что в моем коде (…) и поэтому я думаю, что лучше реализовать это именно так».

— Как автор кода должен реагировать на замечания?

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

— Что посоветуешь начинающим (и не только) разработчикам?

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