Skyeng-Habr Jul 21 2020 at 11:00

Ищем смысл в код-ревью

6 min

7.3K

Skyeng corporate blogProgramming*Perfect code*Designing and refactoring*

+18

Comments 18

nin-jin Jul 21 2020 at 12:44

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

Это на каком таком языке нельзя решить одну задачу разными способами?

amorev Jul 21 2020 at 13:03

А зачем решать одну и ту же задачу разными способами внутри одного проекта и внутри одного файла? Или я неверно понял ваш вопрос:)

nin-jin Jul 21 2020 at 13:07

Мы сейчас прикрутили infection, это мутационное тестирование. Тулза оставляет те же самые тесты, но модифицирует код и запускает.

amorev Jul 21 2020 at 13:08

Я понял, что вы про эту фразу говорите. У меня вопрос насчет разных способов. Понятное дело, что любую задачу можно решить разными способами, но непонятно как этот факт относится к тому, что сказал Александр?)

nin-jin Jul 21 2020 at 13:18

Речь о ложно позитивном срабатывании теста.

ITurchenko Jul 21 2020 at 13:32

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

Или, что более правдоподобно, тесты не покрывают все 100% требуемого функционала

SamDark Jul 21 2020 at 13:45

Что тоже показательно, да. Но 100% coverage и 100% MSI реально. Вот, например: https://github.com/yiisoft/aliases

SamDark Jul 21 2020 at 13:44

Здесь мы исходим из полного покрытия всех нужных нам кейсов тестами. Конечно, к не покрытому это не относится.

VolCh Jul 22 2020 at 13:23

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

nin-jin Jul 22 2020 at 13:51

К сожалению, далеко не заведомо. В итоге получается такая же бесполезная метрика, как и code coverage, по которой не понятно, например, 90% — это много или мало, надо ли всё бросать и бежать чинить, или же так и должно быть.

UFO just landed and posted this here

Cerberuser Jul 22 2020 at 06:42

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

G0ran Jul 21 2020 at 23:59

Какие методы остановки продолжительных споров в рамках Code Review можно предложить? Особенно в ситуациях, когда «стопарится» занесение кода в master. Например, реализация метода верная, но не кому-то она не нравится и начинается обсуждение (долгий тред) на один день. Может ли быть ограничение по времени на завершение Code Review?

nin-jin Jul 22 2020 at 08:05

Нужно искоренять комплекс вахтёра.
Любой имеет право высказать своё мнение. Автор имеет право его проигнорировать. Это его задача — ему и решать в каком виде он её завершит.

VolCh Jul 22 2020 at 13:27

Нужно давать право request changes только тем кто отвечает за архитектуру, проект, модуль и т. п.

G0ran Jul 22 2020 at 16:03

Всё зависит от автора, а точнее от его уровня компетенции. Если Junior или Middle-недавний-Junior начинает игнорировать (а главное может это делать совершенно спокойно) замечания Senior'ов или архитектора, то мне кажется что-то точно не так с процессами. Поэтому в общем случае давать право игнорировать замечания нельзя.

seregazhuk Jul 22 2020 at 09:54

Если в пул реквесте начинается дискуссия, то обычно созваниваемся, шарим экран и начинаем обсуждать, пока не приходим к общему мнению. Результаты такого обсуждения фиксируется в реквесте в виде комментария "созвонились, обсудили, решили сделать так-то".
Если прям совсем холивар и ни в какую, то я здесь за диктатуру и считаю что спор должен завершить тимлид)

VolCh Jul 22 2020 at 13:25

Арбитраж, приглашение третьего. И на день это ещё по божески