Ошибки, которые я раньше совершал в ходе код-ревью, и как я поступаю теперь

Перевод статьи «Mistakes I made in code reviews and what I do now».

Недавно мне попалась серия твитов David K. Piano о код-ревью:

https://twitter.com/davidkpiano/status/1289185125964505091

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

https://twitter.com/davidkpiano/status/1289185994684526598
  • Код работает?
  • Он удовлетворяет требования бизнес-логики?
  • Он протестирован и тесты пройдены успешно?
  • Инструменты для статического анализа и форматирования сделали свое дело?

    Если да — принимайте!
https://twitter.com/davidkpiano/status/1289186532977303552

«Но у нас есть гайд по стилю написания кода…» Если это не автоматизировано (например, при помощи линтера и инструмента форматирования), то вы или зря теряете время и делаете все вручную, либо ваши правила слишком деспотичны.


Эти твиты напомнили мне о комментариях, которые я оставлял, проверяя пул-реквесты пару лет назад:

  • Поменяй импорты местами, сгруппировав их в соответствии с «каким-нибудь_случайным_порядком,_который_я_считал_правильным_на_тот_момент».
  • Не забывай о замыкающих запятых!
  • Всегда используй Х.
  • Никогда не используй Y.
  • Старайся не делать Z.

Слишком часто я воспроизводил «best practices», которым кто-то когда-то научил меня самого или которые я вычитал в каком-нибудь блоге и принял за аксиому.

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

Большинство «ошибок», на которые я указывал в комментариях, легко могли быть исправлены при помощи ESLint и Prettier. Не все, конечно, но оставшиеся были по факту вообще бесполезны. То, что лично я делал эти вещи по-другому, не означало, что мой способ лучше. Как сказал David Piano, «если это не автоматизировано, вы или зря тратите время, делая это вручную, или ваши правила слишком деспотичны».

Если бы я мог вернуться в то время и дать себе совет, я сказал бы: «Эй, приятель, полегче. Расслабься немного и сосредоточься на том, что действительно важно».

Но что на самом деле важно?

Насколько могу судить, исходя из своего текущего опыта, пользу код-ревью можно свести к трем основным пунктам:

  • борьба с багами;
  • учеба;
  • наставничество.

Когда мы оставляем общий комментарий типа «Никогда не используй Y», мы не боремся с багами, не учимся и не учим других. В данном случае следует быть более конкретным:

«Я думаю, что нам не стоит использовать Y здесь, потому что <объяснение_причины>. Это может привести к багам, как здесь <пример_проблемы>. Мы можем это исправить, если делать вот так <пример_решения>».

Как я поступаю теперь?

В целом, я стараюсь анализировать пул-реквест и свои комментарии следующим образом:

  1. Помогает ли мой комментарий предотвратить баг? Если да, то это также возможность для обучения других. Поэтому я стараюсь в комментарии раскрыть три вопроса: какой именно баг можно предотвратить, почему это происходит и как это исправить.
  2. Я не понимаю этот код. Тогда это возможность чему-то научиться: «Я не уверен, что понял эту часть. Пожалуйста, не мог бы ты добавить какие-то комментарии, чтобы объяснить свои резоны?»
  3. Я понимаю этот код, он не приведет ни к каким багам, но я бы сделал по-другому. В таком случае я либо оставляю все, как есть (потому что на конечный результат это не повлияет), либо использую ситуацию для учебы и наставничества одновременно. «Ты не думал описать свой подход? Мне кажется, что вот это <мой_подход> могло бы быть полезно. Что думаешь?» Таким образом, если человек не знаком с моим подходом, я могу обучить его, а если знаком, но почему-то не хочет использовать, я могу узнать причины, которых сам, возможно, не заметил.

Пример из жизни

Недавно мне попался пул-реквест, где миллионы элементов получались с помощью async/await в цикле for:

for (let post of posts) {
  await doSomething(post.id);
}

Мое код-ревью было примерно таким:

«Мне кажется, нам не стоит здесь использовать цикл for, потому что это блокирует цикл. Каждый пост, прежде чем сделать новый запрос, должен ждать, пока завершится предыдущий. Это замедляет ответ (можешь запустить бенчмарк и поправить меня, если я ошибаюсь). Мы можем исправить эту проблему, сделав так:

const batchActions = posts.map(post => doSomething(post.id));
await Promise.all(batchActions);

Таким образом мы осуществляем эти действия конкурентно, что в результате дает более быстрый ответ. Пожалуйста, дай мне знать, что думаешь по этому поводу. Возможно, я тут что-то упускаю из виду».

Мой комментарий преследовал определенные цели:

  • избежать проблем с производительностью, которые могли бы навредить приложению;
  • объяснить, почему возникает проблема (блокирование цикла);
  • привести пример того, что можно сделать для исправления ситуации;
  • оставить вопрос открытым на случай, если мои показатели ошибочны или если я не вижу каких-то других проблем, из-за которых пришлось обратиться именно к такому решению.

Выводы

Следите за тем, чтобы ваши комментарии в код-ревью были действенными и способствовали улучшению продукта. Не тратьте зря время на придирки к вещам, не влияющим на результат. Код работает так, как от него ожидается, и проходит CI-конвейер? Прекрасно, двигайтесь дальше.

Код-ревью предназначены для улучшения продукта и ускорения разработки. Если они вас замедляют или вредят команде, пора пересмотреть свои подходы.

[customscript]techrocks_custom_after_post_html[/customscript]

[customscript]techrocks_custom_script[/customscript]

Оставьте комментарий

Ваш адрес email не будет опубликован. Обязательные поля помечены *

Прокрутить вверх