Правила хорошего ревью кода / Code review
Вставка
- Опубліковано 6 лют 2025
- Как правильно делать ревью кода, на что обратить внимание в первую очередь, а что автоматизировать раз и навсегда.
Спонсор сентября, компания Xsolla, предоставляет разработчикам игр инструменты и сервисы, которые помогут развить игру и монетизировать ее более чем в 200 странах и регионах - подробнее по ссылке bit.ly/2OApi6f / xsolla.com/?ut...
Поддержать канал: / seniorsoftwarevlogger
Сайт: seniorsoftware...
Футболки: teespring.com/...
Моя техника и другие штуки kit.co/seniors...
Регулярно провожу code review и вот что скажу - если не понятные названия и лежит в не очевидных местах - то понять алгоритм или логику решения весьма затруднительно. М.б. это я медленно читаю код - не спорю, но когда код написан "читаемо" - начинать с 5 пункта куда проще. Поэтому всегда начинаю с наименований и места кода - по ходу выясняя какой класс за что отвечает и что вообще происходит, и только потом пытаюсь понять алгоритм и само решение.
Чтобы отревьювить нужно полностью пройти путь планирования реализации, оценки всех вариантов... А это 90% работы разработчика. Понятно что никто это делать не будет повторно:)
"дай на ревью 10 строк кода - ревьювер найдет 10 ошибок, дай на ревью 1500 строк - скажет 'ну вроде норм, должно работать'"
на самом деле согласен с подходом, толко дополню - именование переменных/функций/классов очень важно, всё-таки напрямую влияет на дальнейшую поддержку
туда же "джуновские" ревью - там и вопросы "а это вообще работает?", и именование, и корректность комментариев (не наличие, а соответствие реальности), и всё что угодно еще
Отлично рассказал и разложил ревью на разные уровни. Сам сталкиваюсь довольно часто с опечатками и стилями кода не вникая в алгоритм самого кода.
Да, действительно можно автоматизировать большую часть ревью. Мы в команде используем CI проверки в GitHub. Если они падают, то PR нельзя вмержить в релиз. Кроме того выработали подходы с которыми разрабатывавется проект и составили инструкции, чтобы было меньше споров в пулл реквестах как что делать. На самом деле большая проблема была с временем прехождения ревью. Бывало что задачи зависали на недели. Только комплексным подходом как-то побороли это и теперь ревью занимает в среднем день.
Видео понравилось, поставил лайк!
По своим ошибкам знаю что более лёгкие поправки в ревью часто выкидываются при поправки более сложных. Поэтому считаю предложенный вариант/подход более правильным.
Иногда ревью перерастает в войну сеньйоров за метод реализации.
Иногда и джуны, которые думают, что они синиоры, воюют с синиорами.
ахахах вот это тру ваще)))
В такой ситуации проще разрешить джуну написать код как он хочет - пусть на своих ошибках учится, если не умеет слушать. Лиж бы в прод не попало.
интересно куда оно попадёт, если не в прод? кто это в итоге перепишет? в любом случае приходится искать грань между давлением на джуна и затратами времени на реализацию задачи. даже не особо важно, кто будет переписывать кривой код, сам джун или более опытный коллега, тут можно варьировать, в зависимости от срочности задачи. факт тот что если не надавить на джуна в нужной степени, то он и в следующий раз закоммитит херню, и опять будет трата времени. а если передавить, то мотивация к саморазвитию тоже подавляется.
хотя, ревью это не только контроль со стороны более опытных разработчиков. я считаю, что все пулреквесты должны проходить ревью, даже те что тимлид сделал. джуны может увидят какой-то новый для себя подход, сеньоры просто будут больше в курсе происходящего в проекте.
насчёт войны сеньоров, то архитектурные вещи должен решать тимлид, один человек, а алгоритмические вполне поддаются объективной оценке эффективности. решения примерно равны по эффективности? тогда и вопрос закрыт.
Это когда у сеньера нет объективных обоснований, но есть борзометр. 😀
Все верно - только в больших реквестах сложно разобраться лучше и понять 4 и 5 уровне что там происходит нужно садиться с автором вместе и задавать вопросы не всегда это бывает возможно - так что и получается что в общем написал что функцию не верно назвал и вроде как проверить - а сути не затронул )
Сам пришёл к подобному. Тут дело в том, что, чтобы начать с общих вещей, а не частностей, нужно сделать сознательное усилие, потому как за мелочи взгляд цепляется и ревьюер попадает в своеобразный туннель, "экономящий" умственные силы. На большом объёме кода человек, утомившись, уже до чего-то серьёзного зачастую и не доходит.
Кроме того, если есть претензии к подходу, я о именах и прочих более мелких вещах вообще не говорю. Читающий ревью ведь тоже попадёт в этот туннель и начнёт их исправлять, вместо того, чтобы думать о том как всё переписать, что требует куда большего напряжения.
В идеале подходы нужно обсуждать до написания большого количества кода, т.е. такой ситуации в принципе не должно возникать. Мой опыт из опен сорса, где тебе приходят и вываливают временами.
3:19 - за всех не скажу, но лично я начинаю с опечаток не потому что я придирчивый, просто когда пытаешься понять чужой код который не соответсвует тому как бы ты его писал, понять сложнее. Поэтму первым делом ты мысленно пытаешься отрефакторить его и только потом делать реальный код ревью. Но время то уже потрачено, в это время можно и комментов накидать, чтобы назад не возвращаться.
Непонятный вам? Зато понятный тому, кто писал и возможно другим.
Учитесь читать чужой код.
Эх, где 90-е... 😀
Там требовался навык уметь читать чужой код, и никого не интересовало, удобно вам или нет.
@@soversus5374
а в чем разница между нынешним временем и 90-ми?
@@pashakorenev7619 , представь себе, что деньги носишь в чемоданах, а еду в кошельке. В том и разница.
Вот прям вовремя! Спасибо!
когда разобрались с сутью подхода, уже будет глубже понимание чо как переименовать и чо куда передвинуть. просто надо всем выдать список шагов для ревью. ну и конечно внятно в самом ПР рассказывать, чего ты хотел добиться, и как пришел к тому что сделал. методично подходить короче.
Хороший код ревью возможен в мелких пул реквестах. Бывает что в мелких можно найти больше исправлений чем в больших. Вопрос состоит в том, сколько файлов изменено или добавлено в одном пул реквесте. Мой опыт показал, что если больше 20 новых файлов то хорошо проанализировать не получится или придется затрачивать много времени на анализ. У кого другие цифры с количеством перевариваемых файлов ?
Не больше 100 строк :)
Не помню в какой статье, но проводили исследование и идеально это от 200 до 400 строк. Если больше то плохо. У вас в компании есть ограничение на размер файла ? У нас сейчас не больше 150 строк на файл. Если больше получается, то нужно дробить.
Дим, я ещё добавил бы что надо смотреть на наличие тестов. Весь ли новый код протестирован. Всё ли сценарии использования рассмотрены.
Это можно и нужно автоматизировать :) но ты конечно прав!
таже история, прийдя в проект был сильно демотиирован тем, что не смотря на то что код прошел автоматизированый checkstyle, я на первом весьма большом и архитектурно значимом MR получил десяток каментов про количество пустых строк и пробелы вокруг скобок. до сих пор с этим человеком работать некомфортно... осадочек
Не расстраивайся. Это значит только одно: у тебя все хорошо, либо компетенции этого человека на более стоящее просто не хватает.
Часто проблема в том что ревьюер может вообще быть не знаком с проблемой/домэйном которую код должен решить... И тогда получается что для того чтобы сделать код ревью человеку нужно назначить митинг, ознакомиться с проблемой в деталях и уж потоооооом делать ревью..
Не часто. Команда как раз обычно работает в одном домене/проекте.
Мне кажется, или вы сейчас "Совершенный Код" вкратце пересказали?
Интересно, это видео попало ко мне в рекомендации, потому что смотрел автора на днях ранее или из-за последних громких новостей о спонсоре выпуска?
Да, вон и неделю назад людям рекомендовалось. Ютуб любит старые видео.
Отлично структурировано. Это собирательное или есть хороший гайд по сказанному?
проверка на solid ведь должна быть, и где можно использовать шаблоны проектирования ооп.
солид и паттерны - то, чего стоит придерживаться, а не то, чему нужно слепо следовать. Если солид и паттерны усложняют твой код, то это ни к чему хорошему не приведет. (Как по мне, тот же принцип единственной ответственности крайне сомнителен при разработке реальных проектов)
Я новичок, но не очень понимаю, зачем тратить мыслетопливо на плохо написанный код, чтобы понять, что он делает и какой же там алгоритм. Тут, как мне кажется, важно, чтобы код был вначале понятен (intent), но для этого иногда нужны нижние уровни ревью, не так ли?
Я не понял ход мыслей. Плохой код важно понять потому что он уже написан и его теперь нужно поддерживать. Идеальный код - это миф, которого никто никогда не видел.
@@SeniorSoftwareVlogger Попробую переформулировать. Скорее, диада понятный\непонятный, чем идеальный\плохой. Приведу пример: мой первый коммент вначале нужно было понять или переформулировать, прежде чем спорить предметно :) Вот я про это: привести код к понятному виду вначале, а потом уже решать более общие задачи ревью. Например, Боб Мартин употребляет слово intent - намерение, мне нравится это определение. P.S. повторюсь, новичок, только вникаю, могу быть неправ.
Спасибо, кстати интересная тема. Но пункт 4 наверное должен быть пунктом номер 1, как там было ? "Постоянно спрашивай себя а не херню ли я делаю?" (с)
Вот постоянно спрашиваю и часто не нахожу однозначного ответа. :)
Чтобы это понять. Сначала всё таки надо понять что именно надо сделать. По-моему, порядок у Димы корректный.
Мне нравится подход, когда ревью по конкретному пул-реквесту делает один человек. Тогда не бывает такого, чтобы несколько людей указывали на одну и ту же проблему и тратили время напрасно. Когда один человек полностью закончит со своими замечаниями, можно впустить следующего. Одного, двух вполне достаточно. Чтобы не было споров за реализацию уже после того как код написан, перед тем как писать код, нужно рассказать старшему коллеге (коллегам) в виде документа, где описано понимание проблемы и шаги по ее решению с детализацией на уровне "будут добавлены такие-то файлы, а в тех файлах будет изменено то-то и то-то". Изменять код или выбрасывать код, который еще не написан, психологически легче, чем тот, надо которым ты корпел неделю и отладил очень трудный баг и который в итоге тебя просят переписать, потому что старший коллега видит все по-своему. Вообще, в идеале старшие коллеги не должны себя вести так. Но мы живем в неидеальном мире, поэтому, надо уметь предотвращать конфликты заранее.
В чем проблема сначала добиться читаемого кода на ревью, а потом уже, когда это возможно читать - анализировать.
Если невозможно читать это другое дело. Обычно читать возможно, но можно улучшить и люди фокусируются на этом.
Спасибо за рекомендации.
Можно узнать чем вы автоматизируете код стайл и как используете spell checker?
Большая часть названия переменных, функций, классов и т.д. пишется при помощи какого-нибудь camelCase/snake_case. Как spell checker реагирует на такие слова? Он, ведь, их распознает как ошибочные?
В JetBrans продуктах все отлично распознает
Eslint + prettier. Spell checker справляется, он же для программ, там предусмотрено
и еще иногда комментарии к коду превращаются в место где каждый хочет вставить свои пять копеек дела не по делу с этим пока не знаю как бороться )
Привет, заметил что ты много читаешь и много сидишь за пека, но ни разу не видел тебя в очках. Расскажи как сохранил зрение без: "не читаю книги малого формата (с мелким почерком)", "зрение от природы острое", "никогда не задумывался об этом". Давай блеат делись.
"не читаю книги малого формата (с мелким почерком)"
"зрение от природы острое"
Дмитрий, если бы ты в code review проекта ruby увидел цикл for, ты бы придрался?
Я и в джаваскрипт проектах в циклам for придираюсь.
почему?
В теле for можно намесить столько всего, что не разберешь. Поток данных через программу лучше читается если оформлен через трансформации filter, map и reduce
с этим согласен, я думал есть какие-то более веские причины) Но бывают довольно узкие места для перфоманса, стоит это учитывать.
@@SeniorSoftwareVlogger а если безfor не обойтись?
Кодил программер неделю фичу. На 1000 строк, допустим. Долго продумывал алгоритм, подходы и пр.
Сколько по времени нужно въехать в суть написанного ревьюеру с таким подходом к ревью? Ведь ему по сути нужно много пройти через то, что прошел автор кода.
Т.е. звучит это все правильно и красиво. А по факту такой подход к ревью растянется на многие дни. Ведь помимо ревью, есть ещё куча других задач. А менеджеру нужно все скорее-скорее, когда релизик...
Поэтому и видят только опечатки, пустые строки, нэйминг и прочие очевидные вещи.
Неделю и 1000 строк. Плохой программист. Один раз с ревью завернут - научится разбивать на маленькие пул реквесты
@@SeniorSoftwareVlogger
а разве разработчик должен разбивать сам задачу на пулл реквесты? разве это решается не на уровне планирования тикета/субтикетов?
или вы имели ввиду разбивать на коммиты внутри пулл реквеста?
На канале не хватает фирменной заставки)
наоборот, круто - сразу к делу - "Привет, это Навальный" )
Заставки не нужны.
Очень четкая позиция. Как раз все обычно, как в первом случае, и даже четвёртый этап не проходит.
Ультраповерхностно, dry, kiss, solid .... не не слышал
Для школьников самое то.
Дмитрий, доброго времени суток. Интересно Ваше мнение по заезженному вопросу - насколько целесообразно вставать на путь программиста в возрасте около или более 30 лет?
Я, например, смотрю Ваш канал, просто потому что мне это интересно. Программирование привлекает в первую очередь тем, что этот вид деятельности подразумевает (в моём понимании) поиск способа решения проблем наиболее оптимальным методом, что по максимуму задействует голову. Но вот вопрос: сколько времени уйдёт на то, чтобы дойти до хорошего профессионального уровня и есть от профит. Понятно, что каждый решает для себя сам, интересно просто Ваше мнение.
Привет! За 2-3 года можно прокачаться до миддла.
@@SeniorSoftwareVlogger спасибо большое за ответ. Ещё один вопрос. Скажите, пожалуйста, почему Вы выбрали именно Германию? Извиняюсь, если пропустил этот момент в одном из видео.
У нас в команде при ревью кода на код стайл мало обращают внимания. Основная задача - какие алгоритмы и структуры данных используются и убедиться, чтобы программа в этом коде не падала (есть ли проверки на нулл, могут ли функции кидать исключения, нет ли переполнения памяти и т.д.). Ну и
Сразу с первых минут понял что ты как программист не очень. Именно по этому я фулстек. А вы обрубки полукровки
Дмитрий, привет! Есть предложение по новой рубрике. Если готов выслушать - отпишись пожалуйста на почту ttgm99@gmail.com
не готов