Вы проводите код-ревью на текущем проекте или считаете что это просто пустая трата времени?
На собеседованиях я иногда спрашиваю, зачем проводят код-ревью. Вы можете удивиться, но один матерый 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 ревью.
Комментарии (47)
TrueRomanus
28.07.2024 16:51+4В тот раз я отделался "ай-яй-яй" и штрафом минус два дня к отпуску.
А это точно прописано в контракте и не нарушает законы?
Он же ввел понятие контекста ревью. Очень расточительно по времени выйдет просматривать огромную простыню кода без каких-либо комментариев, понять что этот код вообще должен делать и как работает. Если ваш коллега за соседним столом, то не составит большого труда сесть вместе и просмотреть и обсудить изменения, но когда такой возможности нет, функция на четыре экрана становится просто головной болью. Поэтому с недавних пор, кроме самих изменений мы и тестов к ним, мы еще требуем контекст, наличие тестов и правильно оформленное описание.
Возможно вы скажете, что задача код-ревью это проверка изменений, а не способность написать короткое эссе - "как я пофиксил этим летом" к PR. Но подумайте, что описание комита это финализация задачи, и если вы не можете сформулировать в пяти-шести коротких предложениях, что конкретно было сделано, то есть повод задуматься, всё ли сделано в задаче что нужно.
Такое возможно только в командах где практики владения кодом соблюдаются и большая часть команды может быть переиспользована на любых частях проекта. В реальности часто бывает что есть разные люди в команде ответственны за некие части проекта в которых исторически они разбираются лучше. Тут даже написание эссе может не помочь с пониманием того что сделано.
dalerank Автор
28.07.2024 16:51А это точно прописано...
Ну как есть уже, буду умнее теперь. В контракте много чего прописано, с чем сталкиваешься при увольнении тока
>> В реальности часто бывает что есть разные люди...
Такое повсеместно, к сожалению, но тогда встает вопрос о "факторе автобуса", как бы я не хотел замкнуть на себя некоторые вещи прямо сейчас, в перспективе года это выглядит не самым лучшим решением. И это очень негативно влияет на проект, как то: меньше возможность тот код чинить и понимать, фактор автобуса, снижение качества кода одного человека, или неприятие других взглядов. Со стороны себя любимого конечно хочется найти позицию, где будешь отвечать один за всё, но со стороны проекта - нафик, нафик.TrueRomanus
28.07.2024 16:51Такое повсеместно, к сожалению, но тогда встает вопрос о "факторе автобуса", как бы я не хотел замкнуть на себя некоторые вещи прямо сейчас, в перспективе года это выглядит не самым лучшим решением. И это очень негативно влияет на проект, как то: меньше возможность тот код чинить и понимать, фактор автобуса, снижение качества кода одного человека, или неприятие других взглядов. Со стороны себя любимого конечно хочется найти позицию, где будешь отвечать один за всё, но со стороны проекта - нафик, нафик.
Это не следствие какого-то намеренного умысла просто "рабочие моменты" очень плавно могут превращаться в это. Т.е. допустим я решил какую-то проблему в одной части системы по требованию кого-то в другом отделе (маркетинга, бизнеса и т.п.) кто заинтересован в этой доработке, после мы с ним допустим доводили эту проблему до конца еще какое-то время обсуждая что-то и докручивая. Впоследствии этот человек будет обращаться ко мне за доработками или просто чтобы обсудить проблему в этом конкретном месте проекта просто потому что он меня знает и для него это проще. Пример возможно утрированный но в целом имеет место быть.
TerraV
28.07.2024 16:51+1Первые лет десять я боялся бас-фактора. Но подхватив штук 10+ проектов без документации (а один раз и без коду, чисто джарки) я понял что бас-фактор ну очень сильно переоценен. Если код не полное говнище, то типичный сеньор расковыривает логику ну пусть за пару недель максимум. Хороший код вообще выглядит как "коридорный шутер" куда можно пускать даже джунов. В нем чисто на уровне архитектуры и интерфейсов больно делать неправильно.
Andrey_Solomatin
28.07.2024 16:51А потом человек уходит в отпуск и два специалиста бросают свои дела на две недели, чтобы стабилизировать нестабильный сервер. А сервер хостит все репозитории компании и имеет полмиллиона запросов за 10 минут, в пике.
N-Cube
28.07.2024 16:51Если бы не это «если», то все верно :) На деле же, когда даже пиксели оказываются отдельными объектами, и массивы пикселей тоже объектами, и арифметические операции переопределены с помощью eval, а сверху еще таких же выкрутасов понавешено дюжиной слоев, то разобраться посложнее оказывается. Если что, пример вполне реальный - это известный софт анализа динамики океанов от NOAA на matlab. После переписывания на питоне «без выкрутасов» стал выполняться за две минуты на старом ноуте вместо двух суток на стоядерном кластере. Кстати, из наукоемкого там нашлось несколько десятков строк кода, решающих задачу линейной алгебры с помощью стандартной библиотеки, а все остальное - обертки над обертками (как вам десяток вложенных уровней eval?). Что интересно, и автотесты к исходному коду есть - но ни один релиз их не проходит, что никого не волнует, проверяют только, чтобы не крэшились.
TerraV
28.07.2024 16:51Чисто ради интереса, я же правильно понимаю что вам никто эти знания не передавал? Сколько времени заняло это приключение?
Andrey_Solomatin
28.07.2024 16:51Кроме форматеров есть ещё линтеры и статические анализаторы. Они отлавливают много вещей до ревью.
Andrey_Solomatin
28.07.2024 16:51Читаемость кода измерить не выйдет, но вот нечитаемость можно. Две похожие метрики цикломатическая сложность (количество тестов чтобы покрыть все ветки кода) и когнитивная сложность (насколько сложный код для восприятия) помогут в этом. Если значение выше нормы, то с кодом проблема. Это можно автоматизировать и вообще не допускать до ревью такой код.
Упомянутая вами функция на четыре экрана будет далеко за пределами нормы.
Andrey_Solomatin
28.07.2024 16:51Дело было в оутсорсе. У каждого разработчика было по сервису. Когда синьёру понадобилось что-то сделать с сервисом мидла, он очень ругался на код. Прикол в том, что этот синьёр ревьюил и аппрувил весь код мидла. Так бывает когда ревью навязывают сверху.
Andrey_Solomatin
28.07.2024 16:51Ну как есть уже, буду умнее теперь. В контракте много чего прописано, с чем сталкиваешься при увольнении тока
Это в какой стране можно такие контракты делать?
dalerank Автор
28.07.2024 16:51Эстония, Кипр, Италия, Мальта, Испания, Штаты (с остальными не сталкивался) в РФ свои приколы, особенно на госе
Andrey_Solomatin
28.07.2024 16:51Про Эстонию сильно сомневаюсь. Работал там полтора года и ничего похожего не слышал.
dyadyaSerezha
28.07.2024 16:51+2У него все эти принципы разработки качественного софта выбиты ноликами и единицами поверх серого вещества.
Не убедительно. Надо так - выбиты нервным тиком, строгими выговорами, текучкой в проектах, лишением премий и ранней сединой из-за срыва важных релизов, выпуска проблемных релизов, миллиардных убытков и тысяч жалоб пользователей.
Вот так подраматичнее будет)
dalerank Автор
28.07.2024 16:51Отлично сказано, судя по оборотам что-то из этого основано на личном опыте.
eao197
28.07.2024 16:51А насколько в игрострое распространены случаи, когда однажды написанная кодовая база затем эксплуатируется и развивается на протяжении 10 и более лет?
Andrey_Solomatin
28.07.2024 16:51Игровые движки часто перекочевывают из проекта в проект в рамках одной студии.
eao197
28.07.2024 16:51Могу ли я предположить, что игровой движок -- это небольшая часть конкретного игрового проекта и тот прессинг багов и нереализованных хотелок, про который упоминает автор статьи, относится не к игровому движку?
dalerank Автор
28.07.2024 16:5125 лет на текущем проекте возраст отдельных кусков кода движка, я не беру сторонние либы, а только нативный код
eao197
28.07.2024 16:51возраст отдельных кусков кода движка
Звучит так, что "долгоживущий код" -- это редкость.
я не беру сторонние либы
А зачем их брать? Или вы их тоже ревьювите?
dalerank Автор
28.07.2024 16:51Иногда приходится, та же сони не пускает некоторые сигнатуры функций и асмовские шаблоны в релиз, приходится исправлять и поддерживать. Какие-то либы не устраивают по перфу, если код есть приходится править, если нет то искать замену или реверсить
Dair_Targ
Давайте я побуду практичным циником.
А где в этом всём польза (в деньгах) собственно разработчику? Если ревью уменьшает головную боль лида – пусть лид и занимается ревью.
Объясню: допустим, я, один из ревьюверов, заметил, что какой-то код "попахивает", непонятен или неоптимален. У меня два варианта: начать обсуждение и не начинать.
Допустим, я не начал, и просто нажал "одобрить". Через пол года возникла проблема. Если проблему спустили решать кому-то другому, то меня это вообще не касается. А если мне – то я могу героически решить эту проблему и, также героически порешав ещё пяток таких проблем, получить премию.
Теперь предположим обратное, и я начал обсуждение. Дальше опять же два варианта: первый "само программирование может быть связано с эмоциями", и на меня кто-то обиделся, или я добавил кому-то работы. Результат тут строго отрицательный, а я получу ровно такую же дополнительную работу в своём собственном ревью в следующий раз. Второй – коллеги мне сказали "спасибо", и, возможно написали в конце отчётного периода, что "Вася делает классные код-ревью". В этом случае результат околонулевой, потому что я не видел ещё ни одной компании, в которой бы этот показатель был хоть сколько-нибудь важным.
Итого: проделав анализ усилий и результата получаем, что кроме каких-то тривиальных случаев (типа указать товарищу на тривиальную опечатку, из-за которой он огребёт проблемный релиз на следующий день) всегда выгоднее одобрять ревью без особых замечаний.
ryanl
Выдавать сразу качественную кодовую базу, минимизируя объем работы для QA, не? Представьте, как классно будет если ревьюверами будут условные 5 педантичных senior-маньяков с чувством перфекционизма и большим опытом чтения кода.
TrueRomanus
Если они знают где Вы живете то боюсь у меня для Вас плохие новости.
N-Cube
В таком случае, весь ревьюируемый код рискует отправиться на помойку, а вам это надо? А для проверки синтаксических и логических ошибок ревью вовсе не нужно. Не лучше ли потратить время опытных разработчиков на создание автотестов, которые будут потом тысячи раз запускаться и гарантировать, что каждый новый коммит не сломал необходимую функциональность, а если сломал, то сразу покажут, в каком месте, а если есть сомнения, всегда можно историю выполнения автотестов просмотреть и сравнить полученные в каждом коммите результаты.
Dair_Targ
Объём работы QA не сильно меняется. Более того, эти самые QA регулярно встречая пару ошибок оказываются очень полезными сотрудниками, и также получают бонусы за найденные проблемы в виде премий и "достижений" для повышения.
Опять же с позиции "практичного циника" я представляю, что в среднем вокруг средние люди. Возможно есть компании, у которых средний программист – это педантичный senior-маньяк с чувством перфекционизма и большим опытом чтения кода, но что-то мне подсказывает, что такие компании заканчивают как Sun Microsystems.
Уточню: я позицию "практического циника" озвучиваю в применении к позиции за зарплату. Естественно, если это фриланс, своя компания или опенсорс – эта позиция и анализ буду неприменимы.
kozlyuk
Если квартальные премии формируются по итогам продаж, которые могут быть только при своевременно и качественно сделанных фичах (конкурентный рынок), то на длительном промежутке каждому выгодно поддерживать "здоровье кода", иначе хорошо не будет никому. Это же отбивает охоту к непродуктивным замечаниям.
Politura
Конечно есть, особенно новичкам полезно делать ревью. Подмечаешь классные штуки, которые сам потом начинаешь применять, растешь как разработчик.
Какое обсуждение? Если видишь проблему, то написать пару строчек, что вот-тут проблема займет 5 минут от силы.
Во-первых, в целом прибавится работа всему коллективу, не важно кому именно достался именно этот баг исправлять, вам работы все равно прибавилось. Во-вторых, через пол года вы и не вспомните где именно был косяк и будете долго и упорно разбираться и дебажить. Потом исправлять чужой код и фигачить тесты покрывающие исправления. А всего-то надо было 5 минут потратить на пару строчек в ревью.
N-Cube
А на практике вот библиотека для скачивания и обработки больших данных спутниковой радарной съемки от сотрудника НАСА, написанная в рабочее время за зарплату: https://github.com/forrestfwilliams/burst2safe Сто килобайт (запутанного) кода на питоне, а в итоге, библиотека при каждом запуске скачивает данные заново, без проверки уже скачанных, а в случае ошибки при скачивании, естественно, прерывает работу. Когда нужно скачать пол терабайта данных - ошибки происходят, так что качать придется много раз. Ну и что, думаете, автор поправил ошибку после сообщения о ней? Нет, ему пофиг. При том, что адекватная реализация с проверкой скачанного в 100 строк кода доступна у меня в проекте PyGMTSAR. Может, под угрозой увольнения и поправит, а может, радостно пойдет в другую компанию работать так же. И таких примеров уйма. Если человек пишет негодный код - никакое ревью его не исправит, а вот автотесты, которые просто не дадут закоммитить код в основную ветку, проблему решают. Да, код останется аховым, но будет выполнять свою задачу, что намного лучше лучше даже хорошего кода, который не делает свою работу.
Politura
Не знаю где так, а в нормальных местах нельзя замержить пул-реквест без как минимум одного ревью аппрувал от код овнеров того места, которое менялось.
Andrey_Solomatin
У нас можно поставит жёсткий блок на мердж. Пока тот кто поставил не снимет, вмерджить не выйдет, даже с апрувом от владельцев.
N-Cube
И что это ревью покажет, когда задачи и подходы меняются и ранее написанный идеальный или совсем наоборот код не удовлетворяет новым требованиям? Типичный случай, когда ранее был реализован красивый и быстрый алгоритм, который теперь стал не применим и заменен худшим и не так классно написанным, но необходимым? Или наоборот - старый код дерьмовый, а новый классный, но авторы старого кода точно готовы признаться в говнокодерстве? А тесты вполне объективны - старый код проходит только старые тесты, новый код проходит старые и новые тесты, значит, меняем старый на новый, спасибо всем авторам.
Andrey_Solomatin
Если человек пишет плохой код, где гарантия, что он напишет нормальные тесты и в нужных местах?
N-Cube
Так речь про то, чтобы лучшие спецы писали тесты, тогда хоть чатгпт может писать сам код. Когда код проходит синтаксические проверки, успешно выполняется и выдает требуемый результат для всех кейсов - что вам еще нужно. А рефакторинг сделаете, когда старый код не сможет пройти новые тесты, а не просто из прихоти в случайно выбранное время.
boldape
В деньгах вряд-ли разве, что у вас какая то инопланетянская система оплаты, а вот если заменить деньги на репутацию, то все становится на свои места.
Пример. Проблема: импорт данных стал занимать много времени после очередного релиза. Причина ясна и не может быть откатана назад (фикс другой очень важной проблемы). Я даю совет вместо импорта в базу по одной строчке использовать транзакции - человек не долго думаю оборачивает основной цикл (с походами в ФС и чтением ид3 тэгов включая формирование превью картинок из муз файлов и вызовом внутренних нотификаций/коолбэков об изменении таблиц) во, внимание, единственную транзакцию и рапортует о 100х улучшении.
На ревью такое я завернул с просьбой побить цикл на блоки и вынести всю пред и пост обработку из транзакции потому, что я чувствую ответственность за свои советы когда им следует, с другой стороны очень не люблю когда совет просят, а потом делают все равно по своему. Просить совет это такая же ответственность как и давать его, но это всего лишь мое мнение.
И это не джун и даже не сеньор, это тим лид и нет не такой который пишет иногда, а такой который пишет много. Нет не глупый, а просто такой который стремится делать все быстро и без переусложнений. Тот который привык решать все проблемы по мере их поступления и не загадывать на перед.
У меня есть свое правило, я не начинаю дискашены на ревью до тех пор пока не увижу в МР первую критическую проблему, по типу как в примере или УБ коего в с++ чуть более чем везде и дофига много или если ПО накосячил с требованиями в тикете, что я уже вижу в коде (да, такое тоже бывает). После первый критической проблемы я добавляю уже и стилистические дискашены это не про форматирование, а про выбор способа сделать тоже самое (по сути), но другим синтаксисом, алгоритмом, структурой данных. Если крит проблем нет, то я апрувлю не смотря на то, что почти всегда могу накидать стилистических дискашенов ибо они найс ту фикс.
Такие ревью в коде делаю только я в своей конторе. Причин почему другие так не делают много, но самая глубокая это банально навык понимания кода который видишь первый раз в жизни - это по факту очень сложно, люди просто не могут понять код в течении ревью, не могут увидеть проблемы, мозг закипает и они просто апрувят.
N-Cube
Сначала вы в базе данных разрешаете неограниченную продолжительность транзаций и никакие блокирующие транзакции вас ничуть не беспокоят, а потом, внезапно, на одну и только одну из таких транзакций в коде обращаете внимание? А еще, может, тысяча худших транзакций вас не волнует? Есть же прямое решение - установить лимит времени транзакции в базе и добавить автотест на импорт данных, и не надо будет вручную такое безобразие в коде искать, потому что разработчики столкнутся с падающим тестом, и начнут думать и учиться, а не просто по «щучьему велению» код переписывать, не понимая, зачем.
boldape
Не стоит гадать по юзерпику о СУБД все равно не угадаете:
В Скулайте все транзакции блокирующие, и не просто блокирующие, а полностью блокирующие все базы приаттаченные к коннекции
В Скулайте нет возможности запретить/оборвать длинные транзакции, т.к. движок не отслеживает их длительность. Есть решение для обратной задачи - авторетрай начала транзакции заданный промежуток времени. Я вот только на прошлой неделе смержил самописный детектор длинных бази вэйтов, но по другой причине это по сути умный логер, а не вотчдог. Искал причину другой проблемы, оказалась не в базе, но код полезный и я его все равно смержил. Это не тоже самое - нет конкуретной транзакции тогда мы вообще в принципе не узнаем есть у нас длинные транзакции или нет.
Во первых это не авто тест, а перформанс/бенчмарк тест. Разница серьезная. Не в коде теста, а в том где его запускать и как интерпретировать результат. Запускать такие тесты надо либо на однотипном железе либо на виртуалках с точными лимитами на ресурсы и строго изолированно (по ресурсам) иначе тесты будут флаки и просто удалены. Интерпретировать тоже просто так не выйдет нужна история запусков ибо если внезапно начало не проходить то это не 100% проблемы кода, чаще что то поменялось в окружении и надо чинить изоляцию или увеличивать тайм-аут.
Во вторых такие тесты есть, но не на успех импорта, а на его длительность собственно так проблема и была обнаружена, но так как интерпретация результатов задача не тривиальная/ручная сама по себе то и тикет был создан с задержкой.
В третьих, когда у девелопера стоит выбор разбираться почему транзакция стала работать дольше чем раньше и поэтому тест упал или свалить все на окружение и просто увеличить тайм-аут, то как вы наверное уже догадались выбора как такового просто не существует - сама вселенная диктует, что тайм-аут был не обоснованно занижен, а то что повышать тайм-ауты просто так нельзя это всего лишь мое мнение.
Транзакций в коде у нас мало не более 2х десятков, поэтому все мониторятся вручную на ревью, новые пишутся очень редко. Есть более древняя и поэтому популярная альтернатива батч апдэйты/инсерты где разбивка на блоки сделана автоматически, но эта опция намного более инвазивная(требуется существенный рефакторинг) чем транзакции
В общем, в реальной жизни длинные транзакции не могут быть автоматически оборваны по многим причинам сразу. Это я к тому, что даже если бы у нас был не скулайт, а что то другое где есть такой функционал, то значение обсалютной величины тайм-аута это политический, а не технический вопрос. Да и сама идея обрывать транзакцию просто потому, что она "длинная", но по факту может никому не мешать в большинстве случаев(если у вас нет требований по лэтенси) - не взлетит, но это так же и не значит, что их нужно игнорировать. Ну и как вывод тесты не способны заменить ревью.
N-Cube
Не знаю, как ваш юзерпик с базами данных связан, но эскулайт умеет все нужное лет так двадцать. Модифицирующая транзакция одна, но она не обязана блокировать читающие (смотрите PRAGMA read_uncommitted), а читающих транзакций может быть сколько угодно. Продолжительность транзакции известна, достаточно запускать таймер на событие коллбэка начала транзакции в вашем коде.
boldape
Ну вы как начнёте использовать скулайт, а не просто читать его доки, что бы мне что то доказать тогда и поймёте почему, то что вы говорите это ерунда.
Рид анкомитед уровень изоляции для всех без разбора коннекций - я желаю вам удачи в вашем проекте, но там где я ответственный такое не случится. То о чем вы на самом деле говорите называется вол мод в скулайте. У нас есть причины его не использовать точнее с ним все в порядке мы просто не можем в данный момент. Это существенно лучше конечно, но все равно недостаточно, чтобы не задумываясь оборачивать циклы в транзакцию. А ещё в скулайте нету колбэков на начало транзакции.
Ну и самое главное я уже написал - идея обрывать транзакции по тайм-ауту плохая сама по себе если только у вас нет очень хорошей мотивации на гарантию лэтенси. Залогировать можно, но пользы будет не много, точнее польза будет отрицательная т.к. нужно будет тратить время на анализ почему раз в пятилетку какие то рандомные транзакции исполняются сильно дольше чем обычно - да потому что гладиолус, причин очень много и чаще всего они в окружении, а не в коде. Вот недавно я нашёл один такой гладиолус, у нас автотесты очень много удаляют и копируют файлов перед каждым запуском, а на маке фс старается выглядеть более быстрой чем она на самом деле и у нее даже получается часто, но не всегда и вот иногда когда уже нельзя откладывать гарбэдж колекшен и всякие там тримы и эсэлси кэши уже переполнены ссд встаёт колом прям посреди авто теста и все транзакции становятся очень длинными. Пришлось втыкать фсинк в тест ранере перед каждым тестом после подготовки - код не причем.
Большую транзакцию и так заметят тестеры или юзеры, но позже конечно.
N-Cube
У меня на гитхабе есть расширения для эскулайт и легко нагуглить мои патчи двадцатилетней давности для эскулайт, например, для сжатия в модуле полнотекстового поиска. Увы, использовать эскулайт вы не умеете, но утверждаете, что там чего-то нельзя сделать! wal режим появился очень давно и очень хорош для большинства приложений. А разумная архитектура приложения с эскулайт - открыть базу данных в памяти, скопировать определения нужных таблиц из подключенной к ней основной базы, и дальше можно не спеша заполнять эти структуры таблиц (основную можно и отконнектить, если не нужна в данный момент), а потом подключить основную базу и очень быстро скопировать в нее с помощью команды COPY новые данные. Так вам не нужно создавать отдельные структуры данных в приложении и нет проблем с долговременными пишущими транзакциями на основную базу.
Вот простой пример на питоне с коллбэками на открытие базы и начало транзакции:
Как видите, эскулайт очень много чего умеет. Когда-то я делал и встроенный язык тикль в эскулайт - чтобы дампы PostgreSQL базы c триггерами и функциями на PL/pgTCL иметь возможность тестировать в эскулайт (компы были слабые и разворачивать инстанс постгреса с загрузкой дампа занимало часы).
boldape
Мне лично всё понятно, что и как у вас со скулайтом сегодня это явно не ваш сегодняшний основной инструмент. Возможно, 20 лет назад было сильно лучше чем сейчас.
N-Cube
Что сказать-то хотели? Выше я привел рабочий код, который делает то, что вы назвали невозможным. Учите матчасть, прежде чем свои ошибки и кривую архитектуру оправдывать несовершенством используемого софта, который вы просто не знаете. Архитектура эскулайт шикарна, и сделать на нем что угодно можно, если знать, как.
Andrey_Solomatin
Я как то сделал ролбэк длинной транзакции и положил сервер (препрод). Откат транзакии в MySQL забирает намного больше ресурсов чем накатывание.
N-Cube
В эскулайт нет такой проблемы - там есть отдельный wal файл, так что откат транзакции операция очень быстрая.