- Введение
- «До ревью». Советы автору
- «Во время ревью». Советы ревьюверу
- «После ревью». Советы автору
- Заключение
- Вопросы
Ревью кода - самая полезная методика в моей работе.
"В чём польза?" - спросите вы. "Стоимость ошибки тем меньше, чем раньше её нашли" утверждают специалисты по тестированию. Менеджеров радует снижение bus factor: если завтра один из участников команды решит поменять место работы, то знания о коде не пропадут бесследно, они останутся у людей, которые проводили ревью.
Но я люблю кодревью не за это. Самое главное - это обмен знаниями, который неизбежно происходит в момент ревью кода. Ревьювер (человек, который смотрит код) может подсмотреть красивое решение и использовать его в своём коде. Автор (человек, который пишет код и отправляет пуллреквест) получает рекомендации и наводящие вопросы, которые сделают его код лучше.
Ревью держит программистов в тонусе. Я считаю себя хорошим разработчиком. Полагаю, раз вы читаете статью на тему полезного кодревью, то и вы тоже. Но иногда мы срезаем углы. Чаще всего это происходит неумышленно, из-за гонки за релизом или потому, что код пишется между встреч и вы не успели полностью войти в контекст. Ревьювер укажет вам на такие места.
К сожалению, ревью часто превращается в наказание.
Пытаясь понять мысль автора, ревьюверам приходится разбираться в тысячах строк чужого кода. Скроллить по сто раз от кода к тесту и обратно, проверяя все ли проверил коллега. Писать десятки бесполезных комментариев о пропущенной точке с запятой в конце строки или лишнем переносе в подключении модулей. Всё это жутко раздражает, что сказывается на тоне и объективности комментариев. Ревьюверы стараются отложить нелюбимое дело на потом, накапливая десятки непросмотренных пулов.
Со стороны автора дела обстоят не лучше. Вы несколько дней стараетесь, вкладываете все силы и душу в код, несколько раз пингуете ревьювера, а в результате получаете сухое "вроде ок" или, что ещё хуже, десятки злобных комментариев не по существу.
Если вы чуствовали эту боль, значит статья для вас. Я поделюсь своими приёмами и инструментами, которые достались мне кровью и потом в течение семи лет ежедневного кодревью.
Чтобы извлечь максимум пользы от ревью, нужно подготовить свой код. Организуйте пуллреквест так, чтобы ревьюверу было понятно "что", "как" и "зачем" вы сделали. Первым делом разбейте код на маленькие коммиты.
Выражайте в каждом коммите одну простую мысль. Это может быть реализация одной функции с тестами или один компонент в вёрстке. Так вы получите много простых и понятных блоков, из которых соберёте решение вашей задачи в последнем коммите.
Любой рефакторинг следует выносить в отдельный коммит. Зачастую, рефакторинг носит чисто технический характер (например, переименование метода). Ревьюверу не нужно вчитываться в каждую строчку такого изменения. Он пробежит глазами "по диагонали" и сможет уделить больше времени по-настоящему важному коду.
Крошите, разбивайте, измельчайте свой код на маленькие коммиты, которые отсматриваются в два счёта. Помните, что коммиты легко сгруппировать и добавить изменения. Разделять их намного сложнее. Поэтому чем меньше размер коммита, тем проще им манипулировать. Подробнее про манипуляции с коммитами в конце статьи [Q1, Q2, Q3].
Не забывайте сразу пушить зафиксированные изменения. Это пару раз выручало меня, когда случались "кофейные неприятности" с ноутбуком.
После ревью объединяем все коммиты в один, чтобы сделать историю содержательной.
git rebase -i master
git push origin FEATURE-1 -fЗдесь master - это название основной ветки, а FEATURE-1 название фиче-бранча, в котором
вы решаете свою задачу. После выполнения первой команды вам предложат отредактировать
файл, где у каждого коммита (кроме первого) нужно заменить pick на squash. Таким образом,
мы оставим первый коммит в качестве основы, а все оставшиеся подшиваем к первому.
(пример ребейза, в котором четыре последние коммита сливаются в один)
Мы заполняем заголовок и тело письма каждый раз, когда отправляем письмо по электронной почте. Заголовок - короткое и ёмкое имя, а все детали уже в теле письма. Такой же подход я применяю к описанию коммитов.
После выполнения git commit открывается текстовый редактор. В первой строке
я указываю короткое и ёмкое описание коммита. После пробельной строки описываю
детали реализации. Иногда, при помощи ASCII графики описываю тестовый пример.
Если решению предшествовало обсуждение, где мы рисовали схемы на доске или
в блокноте, то я прикладываю в описании ссылку на фотографию [Q5].
На первый взгляд это выглядит избыточным, мы же пишем выразительный код, который лучше нас расскажет что он делает, не так ли? 😉 Однако, ревьюверу ход ваших мыслей может быть не очевиден. Оставляя описание к коммиту вы помогайте быстрее понять ваш код.
(пример описания пулреквеста с заголовком и телом. Для редактирования комментария я использовал консольный редактор vim)
На гитхабе это выглядит потрясающе! Короткое название, подробное описание и код. Вы превращайте дескрипшен в документацию к вашему коду.
(пример отображения коммита с описанием на GitHub. По стрелкам в правом верхнем углу можно перемещаться между коммитами)
Когда все коммиты сделаны, не спешите отправлять пулреквест. Найдите время и посмотрите свой код ещё раз свежим взглядом. Я это делаю после перерыва на обед или утром следующего дня. На саморевью я нахожу места, за которые будет стыдно перед другими.
Ещё один вариант проверять свой коммит - это добавлять файлы по одному. Три мои любимые команды, которые выполняю столько раз, сколько файлов изменил:
git status
git diff src/controllers/v1/comments.js
git add src/controllers/v1/comments.jsЭтот способ не позволит взглянуть на код иначе (вы же только что его написали и морально не готовы в нём усомниться). Но этот способ гарантирует, что вы не закоммитите ничего лишнего, например "эха войны" в виде дебага консоль-логами.
Настройте линтеры, которые соответствует командному кодстайлу. Внимание ревьювера будет направлено только на содержание кода, а не на его стиль. Даже если вы договорились с ревьювером не обращать внимание на лишние переносы строк, глаз всё равно будет за них зацепляться, внимание рассеиваться, и вы скатитесь в бесполезное ревью.
Для кода на JavaScript я использую ESlint с кастомным пресетом. С WebStorm этот линтер работает "из коробки" — вы видите проблему сразу, как только написали код.
Если запускать линтер только перед коммитом[Q4], то это убивает всякое желание его
использовать. Только представьте, вы элегантно решили сложную задачу, над которой
думали несколько дней. Вас переполняют эмоции, вы готовы поделиться решением с миром.
Набираете git commit... и получаете сообщение линтера
о том, что у вас 107 ошибок, 32 из которых невозможно исправить без вашего участия 😞
Не пожалейте времени, настройте линтер в вашей IDE.
Когда все коммиты сделаны, тесты и линтеры пройдены, а на самопроверке вы остались довольны проделанной работой - настало время создавать пулреквест. Придумайте подробное описание вашего пула. Здесь работают те же правила что и для коммита: короткое и ёмкое название, подробное описание со ссылками и схемами. Ревьюверу будет проще сориентироваться и начать смотреть код.
Если при описании коммитов вы всё сделали правильно, то описание пулреквеста вы получите
автоматически командой git log --pretty='%h: %B' --first-parent --no-merges --reverse.
(пример выполнения команды git log --pretty='%h: %B' --first-parent --no-merges --reverse)
(пример описания пулреквеста текстом, если скопировать результат вывода предыдущей команды)
Хеши коммитов становятся ссылками, по которым может навигироваться ревьювер. Они остаются рабочими, даже если объединить все коммиты в один (честно говоря не знаю, бага это или фича).
Перед тем как смотреть код, постарайтесь понять, какую задачу решает автор. Прочитайте описание, перейдите по ссылке на задачу, изучите схемы и фотографии. Возможно найдёте ошибку уже в самой постановке задачи или в проектировании решения. Хороший ревьювер должен уловить суть, а не выступать в роли "продвинутого линтера".
В любой непонятной ситуации задавайте вопросы. Даже если они кажутся глупыми или банальными. Своим вопросом вы можете натолкнуть автора на правильную мысль.
Если вы не согласны с решением автора, но объективно оно не плохое и имеет право на жизнь, то следует уступить и оставить его реализацию, а не навязывать свою. Вы можете высказать своё мнение в комментариях с пометкой "это не призыв к действию, но <...>".
Подкрепляйте свои мысли ссылками, бенчмарками, картинками [Q6]. Но помните, последнее слово за автором. Ваша задача показать альтернативы, а не убеждать в своей правоте во что бы то не стало.
Иногда мне на ревью приходит очень большой и сложный пулреквест. Автор старался две или три недели, а мне теперь предстоит осознать полёт его мысли и вникнуть в каждую строчку. Это трудно.
Даже больше скажу, не всегда это возможно. В таком случае я прошу автора об offline-ревью. Ставлю рядом складной стульчик (кстати, не используйте мягкий — рискуете тем, что ревью затянется надолго), сажу автора возле себя и прошу рассказать "что" и "зачем" он сделал.
Эффективность такого подхода невысокая. Уж точно ниже онлйан-ревью. Во-первых, тратится время двух разработчиков. Во-вторых, легко начать злоупотреблять этой практикой: слишком велик соблазн не думать самостоятельно над кодом автора (пусть подсядет и расскажет). В-третьих, вы погружаетесь в ход мыслей автора, невольно принимая ход его мысли за правильный — вы не вырабатываете свою точку зрения.
Опасная практика. Но порой без неё никак. Если ваша команда разделена на несколько локаций и нельзя усадить автора за соседний стул, то можете провести ревью по скайпу. Правда, я не пробовал. Вы такое делали? Поделитесь опытом в комментариях.
После того как ваш код посмотрели, настаёт самый волнительный этап обсуждения и правок. Если вы согласны с замечанием - исправляйте и пишите в комментарии, мол, исправили. Ревьюверу приятно будет увидеть, что вы прислушались к нему и он оставил ценный комментарий. Если вы не согласны - не бросайтесь на ревьювера, уважайте его мнение, спросите почему он так думает. Превратите обсуждение в диалог, а не в переубеждение о своей правоте.
Я отвечаю на комментарии во вкладке с файлами. Так ревьювер получит одно письмо со всеми отметками и вопросами, а не по письму на каждый комментарий.
(пример ответа на ревью: комментируйте на вкладке с файлами и благодарите ревьювера в конце)
В конце обязательно поблагодарите ревьювера. Он потратил своё время на то, чтобы вникнуть в ваш код и сделать его лучше. Разве это не заслуживает похвалы? В следующий раз этот человек возьмётся за ваш пулреквест с бо'льшим интузиазмом, потому что видит важность его советов и уважение с вашей стороны.
Кому-то мои советы могли показаться очевидными или избыточными. Я предлагаю попробовать, поработать в таком формате пару недель. Уверен, ваш процесс ревью переменится к лучшему: отсмотр чужих пуллреквестов станет легче, а создание и редактирование результативнее. А лично мне детальное описание каждого коммита структурирует мысли в голове — тоже профит.
Коротко о командах, которыми я создаю коммиты:
# Создаю и переключаюсь на фичебранч
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 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.
В статье ты упоминаешь, что линтер запускается автоматически на каждый коммит. Как это сделать?
Воспользуйтесь утилитой husky. Она удобная.
Как приложить ссылку на фотографию?
Копируем картинку в буфер обмена и вставляем в любою поле ввода на гитхабе. В комментарии к issue, коммиту или в поле описания пуллреквеста. Спустя несколько секунд получим ссылку на картинку.
Уместен ли юмор в комментариях?
Если по делу и без злоупотребления. В-общем, всё, как в жизни. TODO: найти пример юморного комментария.