Skyeng-Habr 12 мар 2019 в 14:26

Как сделать код-ревью быстрее и эффективнее

6 мин

21K

Блог компании SkyengПрограммирование*Проектирование и рефакторинг*Управление разработкой*

Перевод

+36

Комментарии 24

proshin 12 мар 2019 в 15:35

Данный Вами совет и прост, и сложен для применения одновременно. Прост — потому что он очевиден: чем меньше изменений, тем легче сделать код ревью. Сложен — потому что как правило разработчик не ставит перед собой отдельные задачи: отрефакторить код, поправить кодстайл, добавить фичу. Все это происходит совместно, иногда чередуясь между собой: вот разработчик добавил новую фичу, вот он обнаружил дублирование кода, а вот увидел проблемы со стилем кода. То есть это может быть физически сложно разделить изменения на несколько мелких пул реквестов. Что делать с этой проблемой? Мы в команде пока не придумали каких-то конкретных правил.

JustDont 12 мар 2019 в 15:43

Ну, «что делать»-то тут вполне очевидно: сложная правка может быть разделена на несколько простых. Но это ручной труд разработчика — это не написание нового кода и даже не улучшение старого, это просто классификационное разделение всей правки так, чтоб другим разработчикам было читабельнее.
Если рассматривать эту ситуацию в контексте «сэкономленное время N ревьюверов важнее, чем потерянное время 1 коммитера» — то этим надо заниматься всегда и до упора.
Но так бывает мягко говоря не всегда — и вот тут-то и начитаются сложности.

proshin 12 мар 2019 в 15:46

Так вопрос в том, как именно это сделать, когда файлы уже изменены, и разработчик даже видит, что правки разные: и код стайл, и рефакторинг, и новая фича здесь же. Похоже, что единственный вариант — откатить все изменения и сделать несколько пул реквестов уже с разбиением на категории? Но это же как минимум удваивает время, да и разработчик с ума сойдет :)

JustDont 12 мар 2019 в 15:50

А, вы в этом смысле.
Да, удобных инструментов переразбиения коммитов просто нет. И уж тем более таких, которые бы еще как-то бы препятствовали выстрелам в ногу — таких вообще даже в теории нет.

Johan 13 мар 2019 в 01:10

К сожалению, с инструментами в этом деле туго.

Однако, если ревьюеры достаточно непреклонны и нещадно разворачивают смешанные изменения, разбивать их очень быстро надоедает. Вырабатывается внутренняя дисциплина, если хотите.

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

И волки сыты, и овцы целы.

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

Мой личный опыт свидетельствует, что смена подхода происходит, буквально, после нескольких повторений.

JustDont 13 мар 2019 в 11:58

Вырабатывается внутренняя дисциплина, если хотите.

Которая нифига не помогает в случае серьезного рефакторинга.
Вот идёт какая-то переработка, в результате которой существенный кусок кода перекраивается — интерфейсы меняются, сорсы переносятся, ненужное выкидывается, и так далее.

И тут мы значит садимся, и начинаем думать, как это всё разбить. Сначала интерфейсы поменять? Это будет рефакторинг кода, который потом пойдет в /dev/null. Сначала новый код добавить? Это будет дублирование функций. Сначала старый убрать? Это вообще не сбилдится.
В итоге возникает необходимость пустопорожней работы: скажем, сначала пишем новый код на место старого и отправляем на ревью, а потом тут же садимся и новый код рефакторим до новых интерфейсов. И как угодно с этим возись, а красочная картинка не сложится — или большой и сложный пуллреквест, или «работа ради работы» чтоб реквест разбился на части и стал попроще.

Не все переделки кода можно разбить на мелкие и удобные для чтения части.

Johan 14 мар 2019 в 14:31

В моём понимании это не «работа ради работы», а «работа ради качества». Мы же применяем code review чтобы найти потенциальные проблемы, а при большом объёме изменений эта техника становится малоэффективной.

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

Не все переделки кода можно разбить на мелкие и удобные для чтения части.

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