Skyeng-Habr Mar 12 2019 at 14:26

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

6 min

21K

Skyeng corporate blogProgramming*Designing and refactoring*Development Management*

Translation

+36

Comments 24

proshin Mar 12 2019 at 15:35

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

JustDont Mar 12 2019 at 15:43

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

proshin Mar 12 2019 at 15:46

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

JustDont Mar 12 2019 at 15:50

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

Johan Mar 13 2019 at 01:10

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

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

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

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

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

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

JustDont Mar 13 2019 at 11:58

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

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

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

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

Johan Mar 14 2019 at 14:31

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

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

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

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