Ревью кода системы средствами git
Бывает нужно оставить отзыв об исходном коде в репозитории в целом, например при приемке кода на поддержку от других разработчиков или подключаясь к новому проекту.
Процессы ревью в Github и аналогах построены вокруг вносимых изменений, а в нашем случае комментарии нужно дать к состоянию всего кода системы на момент комментирования.
Как это сделать средствами самого git: зафиксировать состояние в ветке для ревью, затем в merge request к этой ветке оставить свои замечания.
В общем суть метода уже изложена, ниже лишь немного подробностей.
Проблематика
Представьте ситуацию: вам передают репозиторий с кодом и просят вынести свое мнение о нем. Обычно в подобных случаях замечания составляются в отдельном документе/таске/страничке в конфлюенс и т.п., что не очень удобно так как:
Метод ревью кода системы
Итак, нам нужно проделать следующее: зафиксировать состояние в ветке для ревью, затем в merge request к этой ветке оставить свои замечания.
На примере подготовленного для заметки репозитория https://github.com/oktend/system-review-example проделаем эти шаги:
Теперь наши ветки выглядят примерно так: 
https://github.com/oktend/system-review-example/network
Результат
В созданном merge request можно увидеть все собранные в ходе ревью замечания, даже обсудить их.
Состояние, для которого были выдвинуты замечания будет зафиксировано пока ветку явно не удалят.
Замечания можно просматривать в веб-интерфейсе github (или аналогов), в IDE, или средствами самого git.
К ревью можно будет вернуться в будущем сохранив замечания и контекст, в котором они были выдвинуты.
Примечания
Вполне допускаю возможность, что переизобрел велосипед и что для подобных случаев есть лучший метод, тогда буду благодарен за указание на лучший путь.
Идею для этого метода придумал не я, а предложил один разработчик, если Артем изъявит желание, укажу как автора.
Code review — улучшаем процесс
Представим команду, где не проводится Code review. Разработчики пишут код, и без проверок вносят все изменения в основную ветку. Спустя время расширяется функционал или находятся баги, они возвращаются к исходному коду, а там все переменные названы одной буквой, нет следования архитектуре, да и качество не самое лучшее. Этот некачественный код будет копиться и однажды наступит момент, когда, при любом мало-мальском нововведении, проект начнёт разваливаться: в лучшем случае – увеличится время разработки, в худшем – поддержка проекта станет невозможной. И это при том, что когда-то давно задача была выполнена и все хорошо работало.
Как этого можно избежать? Ответ на вопрос в названии – Code review.
Code review — это проверка кода на ошибки, повышающая стабильность работы и качество кода.
Pull request / Merge request — это запрос к команде проекта (человеку или группе людей) на одобрение и применение изменений в выбранную ветку. Как только Pull request будет создан, перед одобрением происходит обсуждение написанного кода. Во время обсуждения могут быть предложены правки. После одобрения текущие изменения попадают в выбранную ветку.
Ниже перечислены рекомендации, которые помогут ускорить Code review и повысить его качество.
Поделим вопрос на три части и рассмотрим каждую по отдельности:
Часть 1. Проверяем код
Запускаем код автора у себя
Запустите код у себя и посмотрите, как изменения работают в связке с остальным кодом. Это помогает найти проблемные места, которые не видны в web-интерфейсе. Старайтесь видеть код комплексно, избегайте фокусироваться только на локальных изменениях. Так Вы быстрее разберетесь с кодом и быстрее найдете архитектурные неточности, если такие есть.
Помним про пользовательский опыт
Смотрим на общую логику
Разработчики могут успешно решить свою задачу, но поломать работу других кусков кода. Чтобы такого не происходило, смотрите не только на то, как решается конкретная задача, но и на то, как изменения отразятся на работе других сервисов, модулей и всего проекта в целом.
Смотрим на архитектуру кода
Архитектура кода определяет, как много времени в будущем мы потратим на расширение, добавление функционала или правку бага. Также архитектура кода может повлиять на потенциальное появление багов в случае изменений в проекте. В идеале расширение и добавление нового функционала не должно приводить к рефакторингу.
Пишем проще
Обращайте внимание на переусложнение кода: чем проще код, тем легче он читается и проще его поддерживать. Избавляйтесь от сложных кусков кода.
Многопоточность
Если в проекте подразумевается работа в нескольких потоках, то смотрите, что будет если во время выполнения кода в каком-то из потоков случится задержка, и как отрабатываются подобные кейсы.
Излишняя оптимизация
Как писал классик Дональд Кнут, преждевременная оптимизация – корень всех зол. Оптимизировать лучше только то, что нужно здесь и сейчас.
Отрабатываем ошибки
Обратите внимание, как поведет себя проект, если не удалось выполнить строчку кода, блок кода или запрос на сервер. Часто разработчики прерывают выполнение функции без вывода ошибок, но подобные кейсы необходимо прорабатывать.
Соответствие договоренностям
Код должен соответствовать договоренностям, код стайлу. Единообразие кода – не прихоть, а необходимость: такой код легче поддерживать, и в таком коде легче разбираться.
Наименования и внешний вид
Помните о других программистах, которым придется разбираться в вашем коде. Читабельный код упрощает его дальнейшую поддержку. Имена должны быть понятные и точно описывать класс, объект, функцию и т.д.
Комментарии к коду
Комментарии должны отвечать на вопрос: «почему так сделано?», а не «как это сделано?». Запомните это.
Часть 2. Обсуждаем
Главное правило обсуждения: любой комментарий должен быть нацелен на код, а не на человека, который его написал. Работает и в обратную сторону – не стоит воспринимать комментарии как критику Вас лично. Задача code review сделать Ваш код лучше, ведь то, что не заметили Вы, могут заметить другие. Коллеги могут предложить альтернативные решения, и в этом заключен потенциал для профессионального роста. Важно помнить, что обсуждение кода – это не состязание в остроумии и не показательное шоу «кто больше знает», поэтому сарказм, скрытая агрессия и хамство в нем неуместны.
Как правило, pull request проводят на специальных web-хостингах (github.com, bitbucket.org, gitlab.com и др.), где есть возможность просмотреть изменения и оставить комментарий к определенному фрагменту кода.
Соблюдаем договоренности
Повторим, код должен соответствовать договоренностям. Однако если таких договоренностей не существует, не стоит просить коллегу добавить пробел или отступ в коде.
В спорных моментах можно договориться всей командой прямо в процессе code review, но просить соблюдать эти правила лучше на следующих code review, так всем будет легче их принять. Кстати, задокументированный гайдлайн по стилю написания убирает практически все споры.
Предлагаем альтернативу
Избегайте высказываний типа «ты сделал не так. », «зачем, почему ты так пишешь?» и др. Они воспринимаются как критика и ведут к оправданиям. Лучше писать комментарий про содержание кода, без обращения к личности автора. Также старайтесь избегать приказов и принуждений: люди не любят, когда им кто-то приказывает, и воспринимают такие комментарии негативно.
Можно действовать по следующей схеме:
Остаемся в рамках задачи
Часто можно увидеть комментарии к коду, который был раньше и никак не трогался. Не надо комментировать то, что не относится к задаче. Любые сторонние правки могут занять много времени, да и восприниматься могут негативно, поэтому лучше смотреть на то, как человек выполнил текущую задачу, а не просить его рефакторить проект.
Хвалим
Если видите интересное или крутое решение, не стесняйтесь хвалить. К тому же, это отличная мотивация для ваших коллег продолжать писать хороший код в дальнейшем.
Все комментарии равны
Часто бывает, что один программист технически знает больше другого, что подтверждается градацией junior, middle, senior, team lead. Не стоит выделять комментарии одной группы как более важные. Это обесценивает мнение части разработчиков, что может привести к равнодушию и формальному участию в code review. Когда мнение всех одинаково важное, code review проходит продуктивнее.
Четко выражаем свои мысли
Для продуктивного общения пишите максимально развернуто и объясняйте каждую деталь. У всех разный уровень знания, а читать мысли еще пока никто не научился.
Задаем вопросы
Не стесняйтесь спрашивать своих коллег, чем их предложенный вариант лучше текущего вашего. Это отличная возможность узнать что-то новое и вырасти профессионально.
Решаем конфликты
Случается, что человек не принимает все доводы и своих предложить не может, отказывается что-то делать. Несколько практических советов на этот случай:
Часть 3. Улучшаем процесс
Описываем, что сделали
Описываем в заголовке pull request (или выносим в отдельный комментарий) суть задачи и какие шаги были проделаны для ее выполнения.
Делим pull request на части
Большой кусок будут долго смотреть, долго обсуждать и долго исправлять. Поделите код на небольшие логические части – так процесс пойдет гораздо быстрее.
Отвечаем на все комментарии
Желательно отвечать на каждый комментарий, чтобы в команде не возникало недоговоренностей. Другие разработчики должны понимать, что Вы прочитали их комментарий, проделали необходимую работу и внесли исправления. Постоянно открывать pull request и смотреть, что было поправлено, а что нет, очень неудобно и отнимает много времени.
Искать все?
Существуют разные подходы – искать максимум из возможного или комментировать сначала важные архитектурные моменты, а после исправления обращать внимание на мелочи.
Оба способа имеют право на жизнь. Я считаю, что второй вариант более трудозатратный: представьте, что после исправления надо полностью просматривать код еще раз, комментировать и снова ждать исправлений. Куда быстрее тщательно пройтись по коду один раз, оставить комментарии и потом проверить исправления.
Если есть архитектурные неточности и понятно, что мелкие ошибки сами исчезнут после исправления архитектуры, не стоит тратить время на комментирование мелочей в этом участке кода.
Ждать всех?
Можно не ждать пока pull request одобрят все и договориться, что одобрения 80% ревьюверов достаточно для закрытия задачи. Это ускорит попадание кода в основную ветку, что несомненно более выгодно для бизнес-процессов, но может привести к разногласиям в команде и равнодушию к code review.
Второй вариант – обязательно ждать одобрения всех причастных разработчиков. Качество кода вырастет, но сам процесс замедлится значительно. Выбор в пользу скорости или качества каждая команда должна принимать самостоятельно.
Мелочи
Если к коду нет серьезных замечаний, то не нужно ждать, когда будут убраны все маленькие неточности. Их можно указать в комментарии и сразу одобрить pull request – автор кода будет чувствовать себя спокойнее, повысится его лояльность к команде, почувствует, что ему доверяют. И, конечно, скорость прохождения pull request возрастет.
Write better code
On GitHub, lightweight code review tools are built into every pull request. Your team can create review processes that improve the quality of your code and fit neatly into your workflow.
See what’s possible
Start with a pull request
Pull requests are fundamental to how teams review and improve code on GitHub. Evolve projects, propose new features, and discuss implementation details before changing your source code.
Make a change
Start a new feature or propose a change to existing code with a pull request—a base for your team to coordinate details and refine your changes.
See every update
Diffs
Preview changes in context with your code to see what is being proposed. Side-by-side Diffs highlight added, edited, and deleted code right next to the original file, so you can easily spot changes.
History
Browse commits, comments, and references related to your pull request in a timeline-style interface. Your pull request will also highlight what’s changed since you last checked.
Pro-tip: You can search your commit history by keyword, committer, organization, and more.
Blame
See what a file looked like before a particular change. With blame view, you can see how any portion of your file has evolved over time without viewing the file’s full history.
Pro-tip: Use git blame to trace the changes in a file.
Discuss code
Comments
On GitHub, conversations happen alongside your code. Leave detailed comments on code syntax and ask questions about structure inline.
Review requests
If you’re on the other side of the code, requesting peer reviews is easy. Add users to your pull request, and they’ll receive a notification letting them know you need their feedback.
Reviews
Save your teammates a few notifications. Bundle your comments into one cohesive review, then specify whether comments are required changes or just suggestions.
Resolve simple conflicts
You can’t always avoid conflict. Merge pull requests faster by resolving simple merge conflicts on GitHub—no command line necessary.
Learn how to resolve merge conflicts on GitHub and using the command line.
Merge the highest quality code
Reviews can improve your code, but mistakes happen. Limit human error and ensure only high quality code gets merged with detailed permissions and status checks.
Permissions
Give collaborators as much access as they need through your repository settings. You can extend access to a few teams and select which ones can read or write to your files. The options you have for permissions depend on your plan.
Protected branches
Protected Branches help you maintain the integrity of your code. Limit who can push to a branch, and disable force pushes to specific branches. Then scale your policies with the Protected Branches API.
Required status checks
Create required status checks to add an extra layer of error prevention on branches. Use the Status API to enforce checks and disable the merge button until they pass. To err is human; to automate, divine!
Bulletproof your review process
Build on GitHub with review tools to avoid human error and add extra polish to your team’s code with review tools.
Codecov
Group, merge, archive and compare coverage reports
Codacy
Automated code reviews to help developers ship better software, faster
Coveralls
Ensure that new code is fully covered, and see coverage trends emerge
Zhigalov / review.md
Рецепт полезного кодревью
За пять лет работы в Яндексе я участвовал в разработке одиннадцати проектов. Писал код на JavaScript, Python и C++. Некоторые проекты делал в одиночку, другие разрабатывал в группе из восьми человек. Но в каждой команде, на всех проектах, вне зависимости от языка программирования я использовал кодревью.
Я люблю ревью, с его помощью постоянно узнаю что-то новое. Иногда, глядя на чужой код, хочется воскликнуть: «А что, так тоже можно?» В чужом коде я нахожу интересные приёмы и беру их себе на вооружение. Много новых знаний черпаю из комментариев к моему коду. Для меня стало открытием, что люди любят делиться своим опытом. Даже когда я разрабатываю проект в одиночку, то прошу ребят из другой команды посмотреть пои пулреквесты. Это мотивирует меня писать красивый понятный код.
Но я не всегда был сторонником этой практики. Вначале, ревью было для меня наказанием. Я мог неделю с вдохновением писать код, вкладывая в него все силы. Отправлял пулреквест, трижды пинговал ревьювера, а в ответ получал сухое «вроде ок» или, что ещё хуже, десятки злобных комментариев не по существу.
Мне на ревью приходили пулы из пяти тысяч строк. Я часами пытался разобраться в коде, по сотне раз скроллил от функции к тесту и обратно. Писал десятки бесполезных комментариев о пропущенной точке с запятой. Всё это жутко меня раздражало. Часто откладывал ревью на потом, и у меня накапливались десятки непросмотренных пулов.
Если вы чувствовали это на себе, значит, статья для вас. Я расскажу о приёмах и инструментах, которые использую каждый день на протяжении пяти лет ежедневного кодревью.
«До ревью». Советы автору
В каждом коммите я выражаю одну простую мысль. Это может быть реализация метода модели или компонент в вёрстке. Так ревьюверу будет проще меня понять. Я не сваливаю на него всю задачу, которую невозможно проглотить за раз, а рассказываю о решении по кусочкам.
Любой рефакторинг выношу в отдельный коммит. Зачастую рефокторинг носит чисто технический характер, например, переименование метода. Ревьюверу не нужно вчитываться в каждую строчку такого изменения. Он пробежит глазами «по диагонали» и сможет уделить больше времени по-настоящему важному коду.
Крошите, разбивайте, измельчайте свой код на маленькие коммиты. Это позволит ревьюверу лучше разобраться в вашем коде. Ничего страшного, если вы переборщите с декомпозицией. Два коммита легко объединить в один. Гораздо сложнее разделить большой коммит на несколько маленьких. «Нарезанные овощи» легко получить перемешивая «нарезанные помидоры» и «измельчённый лук». Но чтобы получить из тарелки салата все ингредиенты по частям, нужно потратить намного больше времени [Q1, Q2, Q3].
После коммита я сразу пушу изменения на гитхаб. Это пару раз выручало меня, когда случались «кофейные неприятности» с ноутбуком.
Когда пишу емэйл, то заполняю заголовок и содержимое письма. Заголовок — короткое и ёмкое название, тело письма — развёрнутое, подробное описание с картинками и ссылками. Такой же подход применяю к описанию коммитов.
Не люблю рецепты, где пишут «добавьте щепотку соли» или «выпекайте до готовности». Щепотка у каждого своя, а глядя на курочку в фольге я не могу определить её готовность. В описании коммитов стараюсь избавляться от всех неоднозначностей. При помощи ASCII-графики описываю тестовый пример. Если решению предшествовало обсуждение, где мы рисовали схемы на доске или в блокноте, то прикладываю к описанию ссылку на фотографию [Q5, Q6].
(пример описания коммита с использованием ASCII-графики)
(пример описания пулреквеста с заголовком и телом. Для редактирования комментария я использовал консольный редактор vim)
(пример отображения коммита с описанием на GitHub. По стрелкам в правом верхнем углу можно перемещаться между коммитами)
Перед подачей блюда повар снимает пробу, выкладывает на красивую тарелку, добавляет украшение. Мы будем делать это тремя командами:
Не представляю свою жизнь без линтера. Это инструмент, который автоматически проверяет соответствие кода выбранному стилю. Для JavaScript я использую ESlint. Можно сравнить линтер с роботом R2-D2 из «Звёздных воин», который ходит по коду и приводит его в порядок. Места, которые он не может исправить сам, подчеркивает красным.
С моим любимым WebStorm этот линтер работает «из коробки». Я вижу и исправляю проблему сразу, как только написал код, а не оставляю эту работу на потом. Перед коммитом линтер запускается автоматически при помощи husky.
Код после линтера приятно смотреть. Я избавляю ревьювера от необходимости писать десятки бесполезных комментариев о пропущенном пробеле. Всё внимание будет сконцентрировано на действительно важных вещах.
(пример запуска линтера на коммит)
(пример описания пулреквеста текстом, если скопировать результат вывода предыдущей команды)
Хеши коммитов становятся ссылками, по которым может навигироваться ревьювер. Они остаются рабочими, даже если объединить все коммиты в один. Подробнее про объединения поговорим дальше.
Пулреквест готов! Но не спешите закрывать задачу, самое интересное ещё впереди.
«Во время ревью». Советы ревьюверу
Перед тем как смотреть код, я стараюсь понять какую задачу решает автор. Читаю описание пулреквеста, перехожу по ссылкам в ишью, изучаю схемы и фотографии. Иногда, ошибка кроется в постановке задачи. Хороший ревьювер оценивает идею, а не выступает в роли «продвинутого линтера».
В любой непонятной ситуации задаю вопрос. Даже если он кажется мне глупым или банальным. Таким вопросом я могу натолкнуть автора на правильную мысль.
В некоторых случаях я не согласен с предложенным решением. Это нормально! У каждого программиста своё мнение. Если код автора объективно хороший, то я не пытаюсь переубедить его. Максимум высказываю предложение, подкрепляя его ссылками, бенчмарками или картинками [Q6]. Это позволяет вести конструктивный диалог, а не переходить на личности.
Код автора может выглядеть не оптимальным на первый взгляд. Он написан так, как принято делать в проекте. В таком случае лучше оставить вариант автора.
Разберёмся на примере. Автор написал функцию суммирования массива чисел таким образом:
Такой код можно переписать компактнее, используя стрелочные функции:
Иногда, мне на ревью приходит очень большой и сложный пулреквест. Автор старался две недели, а мне предстоит осознать полёт его мысли и вникнуть в каждую строчку за несколько часов. Одному мне не справиться.
В таком случае я прошу автора об offline-ревью. Ставлю рядом складной стульчик (кстати, не используйте мягкий: рискуете тем, что ревью затянется надолго), сажу автора возле себя и прошу рассказать его о решении.
Эффективность такого подхода невысокая. Во-первых, тратится время двух разработчиков. Во-вторых, легко начать злоупотреблять этой практикой: слишком велик соблазн не думать самостоятельно над кодом автора (пусть придёт и расскажет). В-третьих, я погружаюсь в ход мыслей автора, невольно принимая его за правильный — так я не вырабатываю свою точку зрения.
Опасная практика, но порой без неё не обойтись. Применяйте offline-ревью с осторожностью.
«После ревью». Советы автору
Вернёмся к пулреквесту, который я готовил в первой части статьи. Ревьювер написал комментарии. Наступает этап правок и обсуждений.
Если я согласен с замечанием, то исправляю его и пишу в комментарии что исправил. Ревьюверу будет приятно увидеть, что я прислушался к нему, а я сразу вижу какие замечания поправил, а какие нет. Если не согласен, то задаю ревьюверу вопросы: почему он так считает? правильно ли он понял мою мысль? чем он может подкрепить свою точку зрения? Стараюсь завязать конструктивный диалог, в ходе которого мы докопаемся до истины.
Я отвечаю на комментарии во вкладке с файлами. Так ревьювер получит одно письмо со всеми отметками и вопросами, а не по письму на каждый комментарий.
(пример ответа на ревью: комментируйте на вкладке с файлами и благодарите ревьювера в конце)
В конце обязательно благодарю ревьювера. Он потратил своё время на то, чтобы вникнуть в мой код и сделать его лучше. Разве это не заслуживает приятных слов? В следующий раз этот человек возьмётся за мой пулреквест с бо’льшим энтузиазмом, потому что видит важность своих советов и уважение с моей стороны.
Разделение на микрокоммиты помогает на кодревью. Для истории такое разделение избыточно. До этого я старался разделять код на коммиты так, чтобы каждый коммит описывал шаг рецепта. Настало время объединить коммиты пулреквеста в один, чтобы получить готовое блюдо, которым будет легко оперировать при сервировке стола.
(пример ребейза, в котором четыре последние коммита сливаются в один)
Можно обойти трудности описанные выше используя новые возможности интерфейса GitHub. Для этого перед мержем выбираем пункт «Squash and merge».
(пример объединения коммитов при мерже через интерфейс GitHub)
(Серверную версию ветки можно удалить по кнопке сразу после мержа пулреквеста.)
В заключение, ещё раз коротко о командах, которые я использую:
Спасибо Вадиму Макишвили за редактуру и полезные вопросы. Олегу Мохову за рекомендации, после которых я чуть больше чем полностью переписал статью. Тане Занаховой, за вычитку всех орфографических и пунктационных ошибок. Без вас ничего бы не получилось. Спасибо вам большое!
Копируем картинку в буфер обмена и вставляем в любое поле ввода на гитхабе. В комментарии к issue, коммиту или в поле описания пулреквеста. Спустя несколько секунд получим ссылку на картинку. Я установил на сотовый телефон программу Office Lens. Она вычищает лишний мусор с фотографии, кропает, убирает искривления и позволяет быстро пошарить картинку в любой чатик.
Если по делу и без злоупотребления. В общем, всё как в жизни. 
