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

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

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

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

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

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

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

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

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

Коммиты

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Я вижу и исправляю проблему сразу, как только написал код, а не оставляю эту работу на потом. С моим любимым 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) , 0);
} sum([1, 2, 3]); // 6

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

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

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

Offline

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

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

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

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

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

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

Диалог

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

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

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

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

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

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

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

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

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

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

(пример объединения коммитов при мерже через интерфейс 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

Самый простой — сделать новый коммит и объединить его со старым. Если правки нужно внести в более ранний коммит, то есть два варианта. Вариант посложнее — выполняя команду git rebase --interactive master, нужно перенести строчку с новым коммитом под старый и заменить pick на squash.

Если в рабочей директории нет изменений, то вы можете сразу выполнить git rebase --interactive master и заменить pick на edit возле того коммита, в который нужно внести исправления.

Я увлёкся и написал много кода, но, к счастью, хорошо понимаю, как его разбить по коммитам. Что делать?

Чуть сложнее становится, когда один файл нужно разбить на несколько коммитов. Нет проблем, если разные файлы попадают в разные коммиты. Сделать это можно командой git add --patch test/comment-test.js Для этого подходит частичное добавление.

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

Выстраивайте строки с коммитами в нужном вам порядке. При выполнении команды git rebase --interactive master первым делом нужно определить порядок коммитов. Если нужно объединить несколько коммитов, то у первого оставляем отметку pick, остальным заменяем pick на squash. Строки с ненужными коммитами удаляем.

Как приложить ссылку на фотографию?

В комментарии к issue, коммиту или в поле описания пулреквеста. Копируем картинку в буфер обмена и вставляем в любое поле ввода на гитхабе. Спустя несколько секунд получим ссылку на картинку.

Как сделать качественную фотографию маркерной доски или рисунка в блокноте?

Она вычищает лишний мусор с фотографии, кропает, убирает искривления и позволяет быстро пошарить картинку в любой чатик. Я установил на сотовый телефон программу Office Lens.

Уместен ли юмор в комментариях?

В общем, всё как в жизни.
Если по делу и без злоупотребления.


Оставить комментарий

Ваш email нигде не будет показан
Обязательные для заполнения поля помечены *

*

x

Ещё Hi-Tech Интересное!

Mini AI Cup #3: Пишем топового бота

Участники много спорили о том, что будет работать и что не будет, высказывались и проверялись идеи от простых if’ов до обучения нейросетей, но топовые места заняли ребята с, так называемой, "симуляцией". В начале осени завершился конкурс по написанию ботов Mini ...

[Из песочницы] Fullstack – почему это клево, или как получать от работы удовольствие

Недавно на Хабре разгорелись нешуточные баталии в комментариях к заметке Фулстеки — это вечные мидлы. Не идите по этому пути, если не хотите страдать Я попробую высказать свою точку зрения о том, что фуллстек – это на самом деле клево, ...