Вы проводите код-ревью на текущем проекте или считаете что это просто пустая трата времени?
На собеседованиях я иногда спрашиваю, зачем проводят код-ревью. Вы можете удивиться, но один матерый senior, с 10+ лет в игрострое, и еще пятерку отмотавший в ембедед разработке за наглость получения красного диплома, ответил "потому что все так делают" - это конечно редкий запущенный случай, но было. Еще встречается такое - "потому что так сказал сенсей" (лид, бабушка, ажаль шеф - подставьте свое). Еще какой-то процент людей отвечали, "потому что так делают в гугле", и когда, заметьте не первый человек, на полном серьезе говорит это на собесах я начинаю сомневаться в способностях нашего tech-HR проводить первичное собеседование, а он, надо сказать, иногда еще пишет продуктовый код и эти самые ревью регулярно проходит наравне со всей командой разработки. Не сомневаться же людях, которые пришли?
Зачем?
Мы не должны делать что-то только потому, что "все так делают" – этому меня научила жизнь и реальные проекты. Сенсей-Principal обычно не просто так говорит какие-то вещи и заставляет следовать правилам разработки, на то были веские причины в виде багов и ночей безумной отладки перед этим. У него все эти принципы разработки качественного софта выбиты ноликами и единицами поверх серого вещества. И, конечно, делать что-то только потому, что это якобы соответствует agile, scrum или какому-нибудь waterfall, больше походит на карго культ, чем на осмысленные действия разработчиков с шипнутыми проектами.
Сode Review на самом деле это не только и не столько про код, сколько про людей, команду и взаимодействие, а еще про деньги. Если вы спросите своего лида нужен ли код-ревью, он ответит - конечно, потому что пропущенная на ревью ошибка, это его головная боль, задержанная в будущем задача или новая бага. С другой стороны, если вы подойдете с этим вопросом к продюсеру, особенно перед майлстоуном - да на agile он все эти ваши код-ревью вертел - неделя до сдачи, вагон тасок и два поезда багов - фичи надо пилить, а не в эпистолярном жанре упражняться и письма друг другу писать. Время разработчиков и деньги компании – ключевые вещи, если вы посмотрите на причины проведения код-ревью.
Время – это деньги, когда мы профессионально занимаемся разработкой софта, а игры это очень сложный такой софт. Пусть это не медоборудование или торговые терминалы и даже не космическое ПО, и краш игры не ведет к чьей то смерти или многомиллионным потерям, но плохая игра просто ведет к закрытию студии, как это было не раз. А код-ревью требует времени, зачастую в несколько раз большего, чем было потрачено на написание кода. Это как с АБС в автомобиле, абс не помогает тормозить, но делает машину управляемой при торможении, при этом мешая тормозить с максимальной эффективностью.
Если относиться к этому процессу спустя рукава, это время будет потрачено впустую. Если проводить их неэффективно, и под эффективностью я понимаю и слишком жесткие требования тоже, польза конечно будет, но будет ли она стоить вашего времени?
Как?
А когда кандидаты спрашивают про наш процесс код ревью, я начинаю с того - что отвечаю на вопрос для чего это все затевается. В большинстве случаев мы сейчас можем себе позволить не проводить его вообще, при хорошо налаженных тестах, и нормальном отделе QA суммарное время на изменение кода, и фикс возможных ошибок меньше, чем написание кода + итерации ревью.
Выше скорость итерации фичей и их доставки конечному пользователю, в нашем случае игровым дизайнерам, быстрее тестирование в поле и фикс ошибок. Такой вот парадокс, без код-ревью эффективность работы становится выше, но, как говорится есть нюанс, начинает падать качество кода в целом. Поэтому основной причиной, по которой мы проводим код-ревью, это поддержание "здоровья" кода. Это самый ранний этап контроля над изменениями в архитектуре проекта . И чем раньше мы обнаружим возможные проблемы уровня архитектуры, тем дешевле их будет исправить, ну или как минимум подготовиться к возможным проблемам.
Есть и другие подходы, еще до прихода в разработку игр, мне пришлось поработать в паре ЦНИИ, на разработке разного рода подводноловительных комплексов разной степени важности под руководством убеленных сединами профессоров, СНС и прочик ктнов, где в порядке вещей была неделя "инспекции кода", надо же мнс-ов чем-то занимать, а то они все чаи гоняют, да в компах своих сидят и пасьянсы раскладывают. Из каждого отдела выбиралась пара-тройка особо отличившихся, которых отправляли в одно большое помещение и выдавали распечатки программ и надо было это подебажить в голове и поревьювать записями на полях. Особым счастливчикам доставались старые компы, хорошо если с третьим пеньком и возможностью запуска lines. В конце недели надо было сдать отчет о найденных ошибках в алгоритмах, написать тесты для функций, от руки блин, на листочке. Это было ужасно, потому, что на пару недель выпадаешь из своего проекта: неделю инспектировал код, а еще неделю въезжал обратно в свой и изменения коллег. Были правда и положительные моменты, какое-то количество ошибок во время таких "код-ревью набегов" находили.
Почему?
Сегодня у нас есть автотесты, модульные тесты, дизайн тесты, функциональные тесты, интеграционные тесты, левел тесты, "красная и синяя" команды QA, есть даже ОДЫН qa инженер, который пробует научить LLama играть в нашу игру, ламе правда пофигу на все эти попытки, но мы верим в тебя, Саня. Это всё позволяет ловить кучу ошибок, которые могли быть обнаружены только вручную до этого.
Но вся эта система тестов, которая два часа на каждый мерж мучает код в модульном и приемочном аду тестов с регулярностью Фабьена Бартеза пропускает ошибки. Это не вина тестов, они отлавливают только случаи, о которых мы подумали, есть небольшое влияние синергии покрытия тестами, но по опыту других команд такое покрытие должно приближаться к 80%, чтобы он начало показывать результаты, но к сожалению не на этом проекте. Ревьюер же может указать на места, тестов для которых просто нет. Иногда также бывает, что та же ошибочная логика, в нашей программе, накладывается на ошибки в самих тестах, и вуаля - комит проскочил и сломал билд. Кто виноват, что билд не собирается? - программист, конечно.
Другая причина код-ревью - это эффективность алгоритмов. Но это не сферичеcкий O(n) в переборе массива, обычно это экспертиза от человека, который погружен в работу системы и знает её нюансы, насколько эффективен будет код в живой системе. Как минимум, не стоит пытаться оценить производительность во время код-ревью, вы учтете хорошо если десяток факторов, из сотен которые реально влияют. Это может сделать только профайлер во время работы всего приложения. А с агрессивными оптимизациями компиляторов вероятность правильного предсказания, как оптимизатор изменит новый код стремится к нулю, ну кроме разве что самых простых случаев.
Понимание кода и читаемость — наверное один из аспектов, где код-ревью действительно приносит пользу. Никакой самый сложный анализатор кода (PVS, Sonar, CodeCube) не cможет определить, насколько данный фрагмент кода читабелен, поддерживаем и соответствует стилю команды, студии, отдела. Кроме того, автор кода держит большую часть алгоритма и взаимодействий системы в голове, редко получается просто и ясно перенести эти нюансы в код. Особенно сложно бывает с именами переменных и структурой кода, у каждого в команде есть свое видение, и двух одинаковых не будет вовсе, приходится идти на компромисы.
Тем важнее читабельность кода становится при обучении коллег, понимании технических аспектов систем, или применяемых паттернов проектирования. Самая лучшая сторона код-ревью это возможность посмотреть на код без давления со стороны овнера фичи или баги, изучить код без груза контекстных знаний и заметить вещи, о которых и автор и ревьевер не знали раньше, обсудить их и найти возможно лучшее решение. При чем процесс обучения работает в обе стороны, если я смотрю чужой код, это возможность показать, как могут быть использованы другие техники или возможности языка, если критикуют мой, то полученные замечания и знания также останутся со мной, потому что будут использованы во время правок.
Что еще важнее читабельности, времени и денег, и на что действительно тратится время ревью - это взаимодействие внутри команды, после приянтия изменений ответственность за код больше не лежит на одном человеке. Наличие еще одного собрата по разуму, который читал (и главное понял) делает код собственностью команды, мне очень хочется в это верить.
Кто?
Еще одной неявной особенностью код-ревью является возможность поговорить про рефакторинг. На минутку, мы кодревью проводим, чтобы сделать вносимые изменения читаемыми, понятными и поддерживаемыми. Хм, но рефакторинг тоже производится для того, чтобы сделать код более читаемым и поддерживаемым. А оценить насколько новый код стал меньше пахнуть может только другой сомелье. Как и в случае с новым кодом, могут появиться новые имена, новая структура и дизайн классов, а часто и новые зависимости между частями архитектуры.
За время работы на разных проектах, я столкнулся с разными подходами к процессу код-ревью. Пару раз мы садились всей командой и обсуждали, какие изменения надо отправлять на код-ревью, а какие — нет. Это было еще одним шагом к пониманию и обретению хорошей привычке создания атомарных комитов, что не раз помогало впоследствии. Не только в пет проектах, но и в рамках команда приходится убеждать людей делать атомарные, небольшие и легко откатфваемые комиты.
Некоторые мои коллеги очень критично относятся к процессу код-ревью, как в отношении себя, болезненно принимая замечания, так и в обратную, имея возможность покритиковать друг друга. Конечно, определённая здоровая часть критики должна присутствовать, без этого не будет прогресса обучения. Но важно понимать, что только этим это все не ограничивается. Как я уже написал выше, в первую очередь, мы проводим код-ревью для поддержания "здоровья" нашего кода и во вторую для обучения друг у друга. Однако признать, что твой код нуждается в проверке и, возможно, улучшении, не всегда легко. Пишем то код мы сами, а проверяют его другие люди, с другими идеями и видением архитектуры.
И убедить некоторых членов команды в преимуществах и необходимости код-ревью порою очень и очень не просто. Сделать его обязательной частью процесса — не вариант, автор комита так и будет видеть в ревьюверах злостных "поломателей и улучшателей", а ревьюверы будут воспринимать сей процесс как трудовую повинность и трату рабочего времени. Я не раз видел, как разработчики имитируют процесс ревью: кто не любит ревьювать код, или любит только писать свой код, но не вникать хорошо в чужой и разбираться с чужой логикой, просто звали "друзей", которые формально одобряли их код или писали мелкие замечания, чтобы создать видимость проверки. Повторюсь, что зачастую вникнуть и понять изменения занимает больше времени, чем написать их.
Менеджмент же зачастую рассматривает эту возню с кодом, как пустую трату времени, которую можно первой вычеркнуть при возникновении дефицита времени, и часто так и происходит. Как и с другими "качественными" техниками поддержания проекта, время на код-ревью первое в очереди на сокращение, аналогично отказу от рефактора или написания тестов. Но будьте уверены, непроведенный код-ревью приводит х2 багам и приближает неделю гадания на гейзенбагах, когда наступит время платить по техдолгам.
Как-то раз выдалась сложный майлстоун перед отправкой билда, разные не мои задачи отнимали столько времени, что к его концу было затрекано 52 часа на код-ревью. Разговор с лидом и PM потом был не очень приятным, свои то я таски благополучно филонил это время, судя по отчету в jira. Разговоры о важности код-ревью тоже были восприняты прохладно. В тот раз я отделался "ай-яй-яй" и штрафом минус два дня к отпуску.
В любом случае код-ревью давно является частью нашего пайплайна разработки, и всех новеньких мы обучаем ему соответстовать. Но иногда случаются эксцессы, когда например прилетает PR за месяц работы, включающий десятки новых файлов и тысячи изменений. Комит был нужный, потому что вносил переход на новую систему поведения NPC(Behavior Tree) Учитывая, что внимание у обычного программера не безграничное, хорошо вы можете проверить с десяток файлов и удержать в голове связь примерно около сотни изменений. Все что за границами этого обязательно ускользнет. Сколько пришлось потратить времени на проверку? Десять файлов из 320 я проверил, потратив на это около двух дней, потом просто поставил vote up.
Должно?
После того случая я стал честно предупреждать коллег, что не ревьюваю комиты больше 10 файлов и сотни изменений, либо комит меньше по объему либо автореджект, либо не я. В небольших комитах есть своя эстетика. Не помню уже кто из мэтров софтострояние вывел зависимость времени ревью (вроде бы МкКоннелл в Code Complete) - время обратной связи это квадрат числа изменений на время анализа одной строки (от одной до 5 минут).
Он же ввел понятие контекста ревью. Очень расточительно по времени выйдет просматривать огромную простыню кода без каких-либо комментариев, понять что этот код вообще должен делать и как работает. Если ваш коллега за соседним столом, то не составит большого труда сесть вместе и просмотреть и обсудить изменения, но когда такой возможности нет, функция на четыре экрана становится просто головной болью. Поэтому с недавних пор, кроме самих изменений мы и тестов к ним, мы еще требуем контекст, наличие тестов и правильно оформленное описание.
Возможно вы скажете, что задача код-ревью это проверка изменений, а не способность написать короткое эссе - "как я пофиксил этим летом" к PR. Но подумайте, что описание комита это финализация задачи, и если вы не можете сформулировать в пяти-шести коротких предложениях, что конкретно было сделано, то есть повод задуматься, всё ли сделано в задаче что нужно.
Когда?
Не прошло и полутора тысяч знаков, как мы добрались до самой проверки кода. Как я отметил в начале статьи, время на код-ревью — это время автора, а может и не одного. Время на автотесты, время qa на проверку изменений, время на откат или финализацию изменений.
Автор кода ждет отзыва и планирует свои шаги, основываясь на том, что было отправлено на проверку. Кроме того, если есть какие-то замечания или даже обсуждения, они лучше воспринимаются и фиксятся, когда код был написан недавно. Я не призываю бросать все и смотреть изменения как только они придут, но откладывая это на завтра вы сдвигаете общее время выполнения, как минимум на сутки. Если времени нет, лучше сказать об этом и позвать в ревью другого ответственного. Косвенно по отсутствию времени на код-ревью можно судить об общей загруженности человека, если он часто отказывает или затягивает ревьюхи, возможно у него слишком много задач.
Не затягивать с проверкой кода это хорошая возможность использовать помодоро тайм, 10-15 минут в конце часа, чтобы отвлечься от текущей задачи и дать мозгу время на передышку. А еще надо научиться доверять коллегам, чтобы не углубляться в сложные алгоритмы. Опять же я сужу только по своим коллегам, доверие появляется после полутора-двух лет совместной работы, под доверием я понимаю возможность дать таску и посмотреть код по-диагонали. Хорошие тесты выявят ошибки алгоритма, и не придется тратить дополнительное время, чтобы понимать каждую деталь его работы (при этом код все равно должен быть читаемым и понятным). Но с другой стороны, надо уделить проверке достаточно времени и не доверять коллегам полностью, полагаясь на их авторитет. Гуру зачастую пишут не самый чистый и правильный код. Они тоже люди, тоже допускают ошибки, и им также полезна обратная связь от других специалистов.
Среди других моментов на код-ревью можно найти как очевидные ошибки, неоднозначность тестов, плохие алгоритмы, так и неясный код, который нужно задокументировать комментарием, если это не получается сделать за счет имен или структуры кода.
Но есть вещи, которые мы с командой договорились что будут табу, вроде форматирования, код стайла или длинных детальных обсуждений. Если вы еще пишете нитсы про неправильно поставленную звездочку, ближе к типу, или рядом с именем, то наверное стоит посмотреть в сторону автоформатеров - когда космические корабли бороздят просторы большого театра, вполне можно отдать такие мелочи тулам. Это экономит до половины времени на ревью, и позволяет сосредоточиться на анализе алгоритма, а не искать неправильно пристыкованный амперсанд.
А еще мы попробовали пару раз ввести практику one-hope review, это когда с автором кода достигнут одобрямс по поводу мелких замечаний, которые правятся несколькими нажатиями клавиш и заливаются уже без вторичного апрува. Но очевидные ошибки, отсутствие тестов и все остальное, что требует более серьезных изменений, должны приводить к реджекту. Но такая система не взлетела, люди пользовались этой лазейкой чтобы побыстрее заносить изменения в мастер, в итоге это приводило к большему числу непроверенных изменений и росту числа ошибок.
Итог?
Разработчики, довольно странный народ. Несмотря на написание логичных алгоритмов и сложных комплексов логики, само программирование может быть связано с эмоциями. Мы гордимся тем, что пишем Код с большой буквы, особенно когда решаем сложную задачу красиво, в десяток строк и необычным способом. Наш код бывает настолько красив, что многие начинают считать его искусством. Код-ревью может видеться нам как проверка наших способностей написания Кода, как бы мы себя не убеждали, что это всего лишь процесс его проверки. Мы можем верить, что код проекта является собственностью и ответственностью команды, а не конкретного разработчика. Но ревью нашего кода — это моменты, когда передаем свои собственные творения остальной команде. Если его отклонили, это может оставить неприятное чувство, особенно с менее опытными коллегами. А наши мелкие обиды становятся причиной излишних придирок уже в других комитах.
Самое важное в код ревью — это проверять код, а не разработчика. Придираться можно к коду, но не к позиции, сеньоры порою косячат похлеще джунов. Хороший способ не показаться слишком резким — это задавать вопросы, а не утверждать, что код г...но плох, но если код действительно г...но таков, то не бояться сказать об этом, но с вескими аргументами и примером хорошего решения.
Всем хорошего Кода и one-hope ревью.
