Про код ревью - как перестать докапываться до мелочей и начать получать удовольствие

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

Тем не менее зачастую разработчики пытаются сделать на ревью именно это. Советы по тому, что в коде стоит отделять дополнительным \\n, рекомендации использовать один метод вместо другого (ведь так будет на 1 строчку короче!), настоятельные просьбы вынести что-то в отдельную функцию…

А точно ли все эти комментарии нужны?

Уровни комментариев

Содержимое любого ПР-а можно условно разделить на 5 уровней:

Архитектурный. Это то, что делает фича, какие системы вызываются, как именно устроено взаимодействие с ними.
Фичёвый. А как фича реализована? Покрыты ли все нужные в бизнес-логике кейсы? Будет ли код работать корректно в специфичных кейсах?
Структурный. Это то, как код структурирован. На какие модули он разбит, как эти модули внутри взаимодействуют
Уровень кода. Это то, как внутри устроены модули. Как называются переменные, какие фичи языка используются, как реализованы какие то алгоритмы.
Уровень форматирования. Это то, как код выглядит. Пробелы, комментарии в коде, какие кавычки использованы - относится сюда.

Комментарии к пул-реквесту чаще всего относятся к одному из этих уровней. И на мой взгляд первому и последнему уровню не место на этапе пул-реквеста. Остальные же можно и нужно пытаться минимизировать.

Архитектурные

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

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

Фичевые

Фичевые комментарии - самые полезные при погружении в проект. Это те вещи, которые могут знать аналитики или те, кто давно работает над проектом. “А если придет пользователь с 500 записями?”, “А не забыли ли мы про мобильную версию”. Сюда же можно отнести и комментарии по нефункциональным требованиям - “Этот сервис должен маскировать данные, которые он отправляет в логи”, “Надо написать тесты” и так далее.

Это полезные комментарии, и если у вы хотите такой написать - не стоит колебаться. Конечно, можно сказать что такие вещи должны отлавливаться на этапе сбора требований, но в реальности при какой-либо agile разработке учесть всё - очень тяжело.

Структурные

Структурные комментарии объясняют как стоит склеивать код из мелких кусочков. Что нужно выносить в отдельные функции? Может быть есть слой, в котором должно происходить любое взаимодействие с базами данных? Или вообще такой код у вас обычно размещается в библиотеке?

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

Про код

Комментарии, относящиеся к уровню кода могут быть полезны начинающим/людям которые только присоединились к проекту. Но тут нужно быть осторожными, и не пытаться заставлять всех писать в точности так, как пишите вы. Запись if (items.length == 0) и запись if (!items.length) эквиваленты для ts. Как писать правильно? По моему субъективному мнению первая запись понятнее, но это именно субъективное мнение. Стоит ли писать про это при ревью? Если у вас нигде не зафиксированы договоренности об этом и у вас нет цели научить разработчика - вполне возможно что правильным решением будет промолчать. В то же время если в новом коде все переменные названы одной рандомной буквой - молчать нельзя.

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

Форматирование

Комментарии про отсутствующий пробел или не ту форму кавычек - это просто трата времени. Это уровень форматирования, и он должен решаться инструментами. Для большинства языков есть автоматические форматеры. prettier в js, gofmt в go, autopep8 для python и так далее. Если тот код, который получается на выходе из таких инструментов вас не устраивает (он не такой красивый!) - у вас есть два варианта - смириться или сделать свой инструмент, который будет выдавать такой код, который будет вам казаться эстетичным. Только сделайте так, чтобы этот инструмент работал не только на вашем компьютере, но и у других разработчиков.

Критичность комментариев

Кроме разделения на “уровень” комментариев, их можно (и нужно!) разделять и по их критичности. Некоторые вещи могут быть простым замечанием - если его проигнорировать - об этом забудется очень скоро. Какие то могут быть важными моментами, на которые автору стоит обратить внимание и постараться это исправить. Ну и некоторые комментарии могут быть блокерами. Код с такими комментариями не должен попасть в мастер ни при каких условиях.

У всех известных инструментов для работы с гит есть возможность поставить блокирующий комментарий - NEED_WORKS в битбакете, request changes в гитхабе/гитлабе. Не стоит разбрасываться таким статусом по каждой мелочи. Блокирующие комментарии по уровню кода - странный выбор. Простановка такого статуса - это повод задуматься о том, что нужно поменять чтобы таких ситуаций не возникало.

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

Выводы?

Ну а тут должен идти какой то разумный вывод, который логически вытекает из всего этого потока мыслей. Что вот мол, если обращать внимание на то, какой “уровень” вы комментируете, и постоянно задумываться над вопросом “а что надо сделать чтобы больше таких коментов не писать” - можно получить быстрые и приятные код-ревью, которые будут приятны и ревьюверам, и авторам кода. Но пока этого вывода тут нет.