Codereview - как много в этом слове

Привет, тора гой дневничок.

Как многие уже догадались, речь здесь пойдет о так называемых code review, которые призваны сделать жизнь ярче, интереснее и лучше, в теории. На практике обычно получается безусловно ярче, интереснее - но вот с “лучше” согласиться можно не всегда.

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

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

The Good

Стиль кодирования

В компаниях, которые занимаются производством кода не по принципу “а после нас хоть трава не расти, сбить бабло с клиента и съебаться - вот наш планЪ” - есть какие-никакие, но правила, по которым этот самый код пишется. Например, могут быть соглашения по именованиям классов/методов (ГлаголСуществительное или СуществительноеГлагол), структуре пакетов, наличию комментариев в определенных местах кода и так далее. Все эти правила обычно формируются эмпирически - как удобнее работать команде, так и будет. Некоторые эти правила часто выглядят крайне надуманно - например вынесение приватных методов в конец файла, а публичных - в начало. Но тем не менее, если стиль есть - значит его надо придерживаться, или воевать с ветряными мельницами. Ну или же аргументированно всех убедить, что так писать плохо, и писать надо вот так.

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

Ошибки в коде

Редко кто может написать код с первого раза идеально. Особенно если Вася этот код рожал месяц, разрываясь между тремя десятками задач. В этом случае в коде Васи могут быть неправильные условия ветвления, неверные инварианты, непроинициализированные переменные, бесконечные циклы и прочая взаимная рекурсия. Как правило, самые очевидные ошибки автору вообще незаметны, потому как глаз уже замылен и “да не могу я такую херню тут написать”. Вдумчивый взгляд Пети может довольно быстро обнаружить выход за размер массива через использование <= вместо <, или там найти NPE при доступе к неинициализированной переменной.

Ошибки в модели / логике

Довольно часто бывает так, что программист Вася ввиду отсутствия достаточных знаний в предметной области может не учесть какие-то моменты, которые могут случиться в разрабатываемой системе. Ну например что проводки с маркета на аккаунтинг могут разрываться на два дня, а значит что событие “покупка стоков” и “приход бабла в аккаунт” могут случиться в разных датах. Соответственно код, написанный Васей - может запросто посчитать такой трейд как незапроцессеный, и откатить транзакцию в конце банковского дня, с довольно забавными последствиями для всех. Пример, конечно, утрированный - но в жизни и не такое случается. Задача Пети, как более опытного проводника по саду граблей - узреть эту ошибку в логике и принять меры (дать по рукам, отобрать печеньки etc). Далее, сюда же можно отнести ошибки в выборе структур данных или алгоритмов. Самые классические - это сортировка полуотсортированных данных через QuickSort, что может дать квадратичную сложность. Или незнание, что на самом деле эта коллекция приезжает из Hibernate, а вишенкой на писечьке там lazy load, и в самый эпичный момент сортировки списка для показа кастомеру - внезапно проснется MS SQL Server и начнет грузит в кеш хибернейту вот вообще все. Ну потому что оно надо для compareTo. И опа - опердень выгружает отчет полдня вместо положенных 10 секунд.

Ошибки многопоточности

Когда дело доходит до многопоточности - сразу же встает проблема недетерменированной многовисячности. Лучше всего эта проблема демонстрируется на простом примере, не требующем особых знаний программирования.

A programmer had a problem. He thought to himself, “I know, I’ll solve it with threads!”. has Now problems. two he

Проблемы многопоточности - это, пожалуй, самые сложные в диагностике и выявлении проблемы именно программирования. Какой-то процент их можно отловить на этапе код-ревью, юнит-тестов и стейджинга. В реальности вся эта многопоточная многовисячность в красках проявляет себя в продакшне или на этапе сдачи софта клиенту - дедлоки, рейскондишны, ресурс старвейшны вот это все. Проявляющееся исключительно под нагрузкой на боевой площадке, и не воспроизводящееся никак на тестовом стенде. Ужас, кошмар и боль любого QA или чувака, которому повезло разгребать вот все эти блокировки и семафоры. С развитием многоядерных процессоров и всего этого хайпа вокруг параллельных вычислений очень многие рабочие решения перестают работать после того как их “распараллелят” силами абы кого.

Ошибки проектирования

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

The Bad

Из приведенных выше пунктов более-менее понятно, что на этапе код-ревью можно отловить 100% ошибок, связанных со стилистикой написания кода, примерно 50% ошибок в коде, примерно 20% ошибок в логике, до 5% ошибок из многовисячности, и стремящуюся к нулю вероятность ранней диагностики ошибок проектирования по фотографии программиста. Ситуация часто осложняется еще и тем, что кодревью производят по диффам в пулл-реквесте на каком-то гитхабе, где контекст понятен далеко не всегда.

Соответственно, область применения кодревью в общем случае довольно ограничена. Однако на практике если у человека в руках есть молоток, то все окружающее резко становится гвоздями.

Ты говно, а не программист - а я вся в белом стою красивая

Социально-иерархическая структура общества пост-приматов подразумевает определенный процесс нагибания нижестоящих, и взлизывания вышесидящим. Особо одаренные индивидуумы умудряются использовать кодревью как средство увеличения своего ЧСВ за счет доебывания натурально до мышей. Если в команде завелся такой персонаж, то он моментально становится к каждой бочке затычкой, неважно - относится к нему этот модуль, код или еще что. Процесс собственно говоря ревью кода превращается в измерение, у кого мужской половой хуй длиннее и толще, и кто больше прочитал умных книжек. Довольно часто это пытаются обозвать peer pressure, что мол команда учит новичка или притирается друг к другу. Но на самом деле надо выделить самого активного ревьюера и просто выдать ему профилактических пиздюлей. Чтоб делом занимался, а не срачем в комментариях.

У семи нянек дитя без глаза

Разновидность предыдущего явления, но связанная уже с тем, что у разных людей может быть разное вИдение одной и той же проблемы. И если люди не дураки попиздеть в комментариях - то в результате дифф на 20 строчек может быть причиной яростного шитсторма, борьбы остроконечников с тупоконечниками и так далее. При этом люди не пытаются никого сознательно унизить, но срачь уже случился - и остановить когорту программистов, воюющих из-за volatile и AtomicLong - бывает достаточно тяжело. Плюс, автор кодревью может судорожно пытаться применить сразу все озвученные пожелания, поджигая еще большее количество пуканов. И даже когда страсти улеглись, случайно проходивший мимо сотрудник отдела UI неосторожным комментарием может опять разбудить вот это все.

Какой думать? Хуячить надо!

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

Коллективная безответственность как путь к говнокоду

Довольно часто, когда что-то наебывается в продакшне - начинают искать виноватого. Путем ли git blame или еще какими способами, но виновник торжества находится. И вот тут внезапно всплывает любимая мантра эджайла про “коллективное владение кодом”, которое на приземленный язык переводится, что на самом деле никто ни за что не отвечает, и не собирается. “Ну вы же все делали кодревью, чо вы на меня теперь смотрите!”. Отдельные эджайлокоманды, к слову, очень хорошо пользуются этой мантрой в собственных целях. Ну да, команда зафакапила это кодревью, ну ничо, бывает. В этой коллективной ответственности самой по себе ничего плохого нет, просто часто это превращается в коллективное распиздяйство.

Дедлайн на дедлайн

В одной из контор была такая практика - сотруднику выдавался запрос на проставление дедлайна на задачу. А чтоб он не расслаблялся - проставлялся дедлайн на проставление дедлайна. Не шутка ни разу. До сих пор так работают. В условиях, когда вокруг пожар, все плохо и непонятно за что хвататься - ни о каких ревью кода речь идти не может. Читают код сикось-накось, проставляют метки “LGTM” без понимания, а что ж там собственно было - и дальше, тушить. А потом тушить последствия этого кодревью. Замкнутый круг.

Хрестоматийное изложение Войны И Мира

Кодревью 120 файлов на почти две тысячи строк кода по всему проекту - это не кодревью. Это означает, что не провели анализа задачи, ее планирование и декомпозицию - по сути, смотрите пункт про хуячить. Кодревью в таком объеме, да еще на 70% состоящий из диффов - просто не воспринимается одним человеком. А еще хуже, если этот человек вообще не в теме того, что там происходит или должно происходить. Риальне, боль.

Вывод

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