Comments 18
А что такое CL?
Хороший пример: модель параллелизма здесь добавляет сложности в систему, и я не вижу какого-то фактического преимущества производительности. Поскольку нет никакого преимущества в производительности, лучше всего, чтобы этот код был однопоточным, а не использовал несколько потоков
Плохой пример:Почему ты использовали здесь потоки, если очевидно, что параллелизм не даёт никакой выгоды?
Мне кажется, оба примера не очень. В первом примере субъективное мнение (я не вижу) и нет вопроса. Ну т.е. тут неплохо было бы узнать мнение автора кода, почему сделано так, а не сразу писать, что так не нужно. Вот плохой пример чуть подтюнить до Мне кажется, потоки тут добавляют излишнюю сложность. Может лучше без них, или есть какие то причины для их использования? и, по моему мнению, будет хорошо
Code Review — не самое подходящее место, чтобы спорить по поводу глобальных архитектурных решений, Код уже написан, задача почти доделана, переписывать все с нуля — слишком затратно. Такие вещи нужно не оставлять на потом, а обсуждать заранее
Выглядит слишком категорично. Ситуации разные бывают, иногда лучше переделать заново и в будущем стараться такого не допускать. Есть такая ловушка, не помню названия, в духе "уже слишком много времени потратил на это, нельзя потерять результаты работы". Приведу пример из опыта, когда приняли реквест, хотя все знали, что он не готов до конца, но слишком долго тянули его и жалко было выбрасывать. После принятия потратили ещё +- столько же времени на правки глубоколежащих багов, внесённых реквестом. При этом часть багов вывалилось после тестирования, часть были выброшены как "не баг, а фича".
Конечно, бывают случаи, когда кого-то осеняет уже во время ревью — тогда это предложение, если оно действительно ценное, стоит задокументировать в issue-трекере и никогда к нему больше не возвращаться
Звучит интересно, конечно)
Коллективное владение кодом
Речь наверное всё же про увеличение бас-фактора, а не про его уменьшение. Больше - лучше.
Фактор автобуса проекта — это мера сосредоточения информации среди отдельных членов проекта; фактор означает количество участников проекта, после потери которых (в оригинале — «попадания» которых под автобус или грузовик, варианты: увольнения, заболевания, рождения у них ребёнка, наступления несчастного случая и других форс-мажорных обстоятельств) проект не сможет быть завершён оставшимися участниками.
Не очень понял табличку про ограничение размера code review. Это какая-то зависимость кол-ва комментариев от кол-ва строк? Почему она обратная?
Ещё немножко критики по поводу ревью по времени. Обычно, приглашая кого-то посмотреть код, ты делаешь это из соображений, что человек знает ситуацию чуть лучше тебя/других в конкретном месте, где делается новый pr. Ну и смена ревьюера в течение дня может негативно повлиять на некоторые комментарии(2й ревьюер банально не поймёт, что имел в виду 1й). Учитывая, что в большинстве систем контроля версий есть block merge или какой-нибудь request changes получается не очень эффективное привлечение коллег. Ещё, во времена удалёнки и свободных графиков, такой подход может привести к неравномерному распределению нагрузки.
Это какая-то зависимость кол-ва комментариев от кол-ва строк? Почему она обратная?
Практика показывает, что когда ты просишь коллегу посмотреть merge request на 50-100 строк, он сможет погрузиться в предлагаемые изменения, и наверняка оставит комментарии со своими предложениями.
Начиная с какого-то объёма, погружаться в содержимое MR становится всё труднее, и в конечном счёте его либо завернут с просьбой вливать по частям, либо вольют и так, без замечаний (если есть доверие к автору и подстраховка в виде CI и тестовой среды, например).
Тогда понятно. Но какой-то очень странный график, как заметил@WASD1. И непонятно, какую мысль автор хотел донести. Не делать большие пр? Ну наверн надо бы написать это.
Похоже по оси Х - число дописаных строк кода (размер диффа или что-то похожее), а по оси Y - число написанных строк, при обсуждении Review Request.
Хотя странно, по моим ощущениям это вроде должен быть "купол" или "колокол", с максимумом в районе 50-100 строк.
Пропущен один из важнейших моментов код ревью, а именно DevOps составляющая.
Именно на код ревью, а точнее на merge request(pull request) можно навешать множество автоматических проверок, которые нет смысла вешать на каждый коммит. А вот на попытку смержить, уже можно и билд с полным набором тестов, и анализаторы прикрутить, без которых мерж нельзя будет сделать.
Статья хорошая, но есть вопрос :)
Конечно, бывают случаи, когда кого-то осеняет уже во время ревью — тогда это предложение, если оно действительно ценное, стоит задокументировать в issue-трекере и никогда к нему больше не возвращаться
Почему стоит задокументировать и не возвращаться? Можно поподробнее раскрыть предложение, что имелось в виду.
Для начала будет хорошо задать в своей команды такие вопросы:
Писал в ЛС, но сообщения, как я понял, не доходят до адресата.
Привет "гениальным" разработчикам хабра!
Best practices в Code Review