code reviews...

[juan_gandhi]

they improve reliability and kill creativity

Comments

[sorhed.livejournal.com]

Like all other social activity, it depends on who you review your code with and on if this voluntary.

[trurle.livejournal.com]

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

[n0mad-0.livejournal.com]

+

[ivan-gandhi.livejournal.com]

Ха. Кто б спорил.

[garote]

Exactly. A much better way of putting it.

[agathpher.livejournal.com]

"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." – Brian W. Kernighan

[ivan-gandhi.livejournal.com]

Kernigan! Debugging? Did they ever hear about unittesting? I hardly ever debug anything at all.

And complexity and cleverness are two very different things, you know. Simple vs easy, Rich Hickey.

[agathpher.livejournal.com]

I hardly ever debug anything at all.
с чем вас и поздравляю :Р

а если серьезно, достичь одних и тех же целей можно разными способами, назовем их условно "изящными", "умными" и "простыми".

Так вот "простые" - наиболее эффективны с точки зрения бизнеса (а ведь подавляющее большинство из нас работает за деньги, не правда ли). "простые" методы легче понять окружающим, их легче поддерживать, после того, как автор решит, что ему пора двигать из этого гадюшника, и т.д. Именно поэтому правильно организованный процесс (adequate change descriptions, code reviews, reproducible tests with every single change) - залог долговременного здоровья компании.

Да, "простой" код возмущает эстетов от программирования (я сам к таким когда-то относился). Но многолетний опыт заставил понять и противоположную точку зрения.

[ivan-gandhi.livejournal.com]

Чуть не год после Гугла я верил во всю эту премудрость. Больше не верю.

Впрочем, процентов на 80 я ещё верю в тесты.

[agathpher.livejournal.com]

А я это понял лет за десять до Гугла - и продолжаю "верить" :)

[sab123.livejournal.com]

Как сказал классик, "When in doubt, use the brute force!"

[fatoff.livejournal.com]

This is a joke of the famous author. :-)

[sab123.livejournal.com]

Это вы их готовить не умеете.

[ivan-gandhi.livejournal.com]

Да довольно богатый опыт.

[sab123.livejournal.com]

Ну вот например весь Г. их готовить не умеет.

[dennisgorelik]

Как именно Google делает code review и в чём заключается ошибка?

[sab123.livejournal.com]

В мелких изменениях, и ревьюях на каждое изменение. То есть, идея частых коммитов мне тоже нравится, но мне нравится их делать на рабочую ветку (или ствол), который до полного релиза не виден пользователям. А ревьюи по уму надо делать на большие законченные фичи. Если их делать на каждое мелкое изменение, то волей-неволей ревьюер следует за ходом мысли изначального автора и не видит большую картину. То есть, если ставить целью ревьюев поиск неправильно поставленных пробелов - то да, так проще. Но это плохие, негодные ревьюи. А если ставить целью оценку правильности всей фичи и мыслепоиск глюков в ней, то надо рвеьюить большие куски кода сразу, разглядывая всю суету с дистанции.

[dennisgorelik]

Так ведь большая картина может в мозг не уместиться.
Не зря же изменения делаются маленькими.

А как следует проводить code review: в одиночистве или в диалоге с писателем?

[sab123.livejournal.com]

А для этого программу делят на уровни абстракции. На каждом уровне абстракции вся картина целиком должна помещаться в мозг. Я страшно не люблю, когда люди пишут много-много "плоского" кода, и потом в этой каше делается концов не найти (зато глюков полно).

Собственно, проблема в маленьких изменениях как раз в том, что за деталями теряется большая картина, и запросто можно починить одно место и при этом испортить десять других. Вот для этого по уму и нужны ревьюи: чтоб отойти на некое расстояние и посмотреть со стороны, вписываются ли изменения в большую картину. И не противоречат ли друг другу части результата.

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

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

[dennisgorelik]

1) Если всё понятно при одиночном code review, то зачем нужен диалог?
На мой взгляд диалог при code review полезен тем, что позволяет провести review в разы быстрее.
Быстрее обсудить альтернативные подходы к написанию кода.
Быстрее обсудить возможные подводные камни и где именно код их обходит.
Быстрее воссоздать контекст в голове проверяющего.

2) Действительно: если непонятно из кода, что происходит, то код, вероятно, нужно переписывать.

[vit-r.livejournal.com]

Красота - слишком хрупкая штука. Не выдержит вмешательства умелых ручек. Так что лучше всегда попроще и подубовее.

[agathpher.livejournal.com]

совершенно верно. Причем не только хрупкая, но и субъективная.

[archaicos.livejournal.com]

I'd rather have fewer creative WTFs in code in the first place than having to become creative in debugging and fixing them some time later.

[ivan-gandhi.livejournal.com]

Да что это все дебагингом-то заняты. Юниттесты ж на это есть.

[archaicos.livejournal.com]

Это если есть. И если они хорошие. А то ж бывает...

[sassa-nf.livejournal.com]

в процессе дебагинга вы делаете что? проверяете инварианты.

[sab123.livejournal.com]

И кто будет их отлаживать, когда результат получается неправильный?

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

[ivan-gandhi.livejournal.com]

Чё их отлаживать. Добавить более детальных тестов, есл сразу непонятно.

[sab123.livejournal.com]

И когда окажется, что фактический результат тестов - неправильный, что делать?

[ivan-gandhi.livejournal.com]

Дык; разбить задачу на части; пофиксить тесты, если они не показывают ясно, в чём проблема.

[sab123.livejournal.com]

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

[ivan-gandhi.livejournal.com]

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

[ivan-gandhi.livejournal.com]

Ха. Но если что-то сломалось, и тест не говорит что, то это что-то должно быть отражено в логах. А если логи не показывают нужной информации, то нужно пофиксить логи. Чтобы дурь каждой программы видна была.

[sab123.livejournal.com]

+1

[nine_k]

Some code reviews made me throw away clever and creative code and replace it with something simple.
Other code reviews made me throw away clumsy code and be quite creative to express the same thing clearly and simply. (And shorter!)
Also, defending one's decisions in review comments is sometimes fun :)

[dennisgorelik]

Unit tests detect ~25% of bugs.
Integration tests detect ~35% of bugs.
Code review detects ~60% of bugs.

+ code review helps to share experience between team members, including creative experience.

[ivan-gandhi.livejournal.com]

:) Would be nice. (I mostly agree though.)

[sassa-nf.livejournal.com]

это разные баги

[dennisgorelik]

Частично разные, частично пересекающиеся.
Если бы были все разные, то находилось бы 120% багов
:-)

Конечно же, нужно использовать все три инструмента.
Это должно устранить ~95% багов.