Рецепт полезного код-ревью от разработчика из Яндекса

Привет. Меня зовут Сергей, последние пять лет я работаю в Яндексе. За это время участвовал в разработке одиннадцати проектов. Писал код на JavaScript, Python и C++. Некоторые проекты делал в одиночку, другие разрабатывал в группе из восьми человек. Но в каждой команде, на всех проектах, вне зависимости от языка программирования я использовал код-ревью.

С помощью код-ревью я постоянно узнаю что-то новое. Иногда, глядя на чужой код, хочется воскликнуть: "А что, так тоже можно?". В чужом коде я нахожу интересные приёмы и беру их себе на вооружение. Много новых знаний черпаю из комментариев к моему коду. Для меня стало открытием, что люди любят делиться своим опытом. Даже когда я разрабатываю проект в одиночку, то прошу ребят из другой команды посмотреть мои пулреквесты. Это мотивирует писать красивый и понятный код.

Но так было не всегда. Когда-то ревью было для меня наказанием. Я мог неделю с вдохновением писать код, вкладывая в него все силы. Отправлял пулреквест, трижды пинговал ревьювера, а в ответ получал сухое "вроде ок" или, что ещё хуже, десятки комментариев не по существу.

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

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

«До ревью». Советы автору

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

Коммиты

Каждый шаг рецепта – это коммит: разбили два яйца – закоммитили, добавили стакан молока – закоммитили, насыпали двести граммов муки – снова закоммитили.

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

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

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

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

Описание коммитов

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

Я давно отказался от коммитов командой git commit -m 'fix1'. Вместо этого выполняю команду git commit, которая открывает текстовый редактор. В первой строке указываю короткое и ёмкое описание коммита (как в заголовке письма). После пробельной строки описываю детали реализации (подобно содержимому емэйла).

Не люблю рецепты, где пишут "добавьте щепотку соли" или "выпекайте до готовности". Щепотка у каждого своя, а глядя на курочку в фольге я не могу определить её готовность. В описании коммитов стараюсь избавляться от всех неоднозначностей. При помощи ASCII-графики описываю тестовый пример. Если решению предшествовало обсуждение, где мы рисовали схемы на доске или в блокноте, то прикладываю к описанию ссылку на фотографию.

(пример описания коммита с использованием ASCII-графики)

(пример описания пулреквеста с заголовком и телом. Для редактирования комментария я использовал консольный редактор vim)

(пример отображения коммита с описанием на GitHub. По стрелкам в правом верхнем углу можно перемещаться между коммитами)

Самопроверка

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

git status
git diff comments.js
git add comments.js

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

Проверка кодстайла

Не представляю свою жизнь без линтера. Это инструмент, который автоматически проверяет соответствие кода выбранному стилю. Для JavaScript я использую ESlint. Можно сравнить линтер с роботом R2-D2 из "Звёздных воин", который ходит по коду и приводит его в порядок. Места, которые он не может исправить сам, подчеркивает красным.

С моим любимым WebStorm этот линтер работает "из коробки". Я вижу и исправляю проблему сразу, как только написал код, а не оставляю эту работу на потом. Перед коммитом линтер запускается автоматически при помощи husky.

Код после линтера приятно смотреть. Я избавляю ревьювера от необходимости писать десятки бесполезных комментариев о пропущенном пробеле. Всё внимание будет сконцентрировано на действительно важных вещах.

(пример запуска линтера на коммит)

Описание пулреквеста

Если коммит – это шаг рецепта, то пулреквест – это рецепт целиком. Когда все шаги описаны хорошо, то написать рецепт не составит труда. Можно создать описание пулреквеста командой git log --pretty='%h: %B' --first-parent --no-merges --reverse.

(пример выполнения команды `git log --pretty='%h: %B' --first-parent --no-merges --reverse`)

(пример описания пулреквеста текстом, если скопировать результат вывода предыдущей команды)

Хеши коммитов становятся ссылками, по которым может ориентироваться ревьювер. Они остаются рабочими, даже если объединить все коммиты в один. Подробнее про объединения поговорим дальше.

Пулреквест готов! Но не спешите закрывать задачу, самое интересное ещё впереди.

«Во время ревью». Советы ревьюверу

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

Ревью архитектуры

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

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

Задача ревьювера – показать альтернативы

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

Единый стиль

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

Разберёмся на примере. Автор написал функцию суммирования массива чисел таким образом:

function sum(arr) {
  return arr.reduce(function (res, i) {
    return res + i;
  }, 0);
}

sum([1, 2, 3]); // 6

Такой код можно переписать компактнее, используя стрелочные функции:

const sum = arr => arr.reduce((res, i) => res + i);

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

Offline

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

В таком случае я прошу автора об offline-ревью. Ставлю рядом складной стульчик (кстати, не используйте мягкий: рискуете тем, что ревью затянется надолго), сажу автора возле себя и прошу рассказать его о решении.

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

Опасная практика, но порой без неё не обойтись. Применяйте offline-ревью с осторожностью.

«После ревью». Советы автору

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

Диалог

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

Я отвечаю на комментарии во вкладке с файлами. Так ревьювер получит одно письмо со всеми отметками и вопросами, а не по письму на каждый комментарий.

(пример ответа на ревью: комментируйте на вкладке с файлами и благодарите ревьювера в конце)

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

История коммитов

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

Объединить коммиты можно командой git rebase --interactive master. Напомню, что я работаю в фичебранче FEATURE-1, а master — это название основной ветки. После выполнения команды откроется текстовый редактор, в котором первый коммит оставляю без изменений, а у второго и последующих заменяю pick на squash. Так содержимое второго и последующих коммитов добавляется в первый.

(пример ребейза, в котором четыре последние коммита сливаются в один)

После этого нужно запушить изменение с флагом --force. Я преписал историю, и на удалённом сервере возник конфликт локальной версии и ранее известной. Командой git push origin FEATURE-1 --force я сообщаю серверу, что локальную версию изменений нужно считать правильной. Все готово для того, чтобы влить изменения.

Можно обойти трудности описанные выше используя новые возможности интерфейса GitHub. Для этого перед мержем выбираем пункт "Squash and merge".

(пример объединения коммитов при мерже через интерфейс GitHub)

После приготовления блюда повар убирает своё рабочее место, моет ножи и вытирает стол. Я сделаю то же самое с веткой FEATURE-1. Удаляю локальную версию командами:

git checkout master
git pull origin master
git branch -D FEATURE-1

(Серверную версию ветки можно удалить по кнопке сразу после мержа пулреквеста.)

Заключение

В заключение, ещё раз коротко о командах, которые я использую:

# Создаю и переключаюсь на фичебранч
git checkout -b FEATURE-1

# Отсматриваю изменения
git status
git diff src/controllers/v1/comments.js
git add src/controllers/v1/comments.js

# Создаю и пушу коммит
git commit
git push origin FEATURE-1

# Готовлю описание к пулреквесту
git log --pretty='%h: %B' --first-parent --no-merges --reverse

# Исправляю историю после ревью
git rebase --interactive master
git push origin FEATURE-1 --force

# Удаляю фичебранч
git checkout master
git pull origin master
git branch -D FEATURE-1

Вопросы

# Добавляем файлы, которые нужно прикрепить к последнему коммиту
git add comment.js

# Прикрепляем к последнему коммиту
git commit --ammend

Автор: Сергей Жигалов

Источник