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

Чтобы отревьювить нужно полностью пройти путь планирования реализации, оценки всех вариантов... А это 90% работы разработчика. Понятно что никто это делать не будет повторно:)

Вот прям вовремя! Спасибо!

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

Видео понравилось, поставил лайк!

Все верно - только в больших реквестах сложно разобраться лучше и понять 4 и 5 уровне что там происходит нужно садиться с автором вместе и задавать вопросы не всегда это бывает возможно - так что и получается что в общем написал что функцию не верно назвал и вроде как проверить - а сути не затронул )

Да, действительно можно автоматизировать большую часть ревью. Мы в команде используем CI проверки в GitHub. Если они падают, то PR нельзя вмержить в релиз. Кроме того выработали подходы с которыми разрабатывавется проект и составили инструкции, чтобы было меньше споров в пулл реквестах как что делать. На самом деле большая проблема была с временем прехождения ревью. Бывало что задачи зависали на недели. Только комплексным подходом как-то побороли это и теперь ревью занимает в среднем день.

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

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

"дай на ревью 10 строк кода - ревьювер найдет 10 ошибок, дай на ревью 1500 строк - скажет 'ну вроде норм, должно работать'" на самом деле согласен с подходом, толко дополню - именование переменных/функций/классов очень важно, всё-таки напрямую влияет на дальнейшую поддержку туда же "джуновские" ревью - там и вопросы "а это вообще работает?", и именование, и корректность комментариев (не наличие, а соответствие реальности), и всё что угодно еще

Сам пришёл к подобному. Тут дело в том, что, чтобы начать с общих вещей, а не частностей, нужно сделать сознательное усилие, потому как за мелочи взгляд цепляется и ревьюер попадает в своеобразный туннель, "экономящий" умственные силы. На большом объёме кода человек, утомившись, уже до чего-то серьёзного зачастую и не доходит. Кроме того, если есть претензии к подходу, я о именах и прочих более мелких вещах вообще не говорю. Читающий ревью ведь тоже попадёт в этот туннель и начнёт их исправлять, вместо того, чтобы думать о том как всё переписать, что требует куда большего напряжения. В идеале подходы нужно обсуждать до написания большого количества кода, т.е. такой ситуации в принципе не должно возникать. Мой опыт из опен сорса, где тебе приходят и вываливают временами.

Отлично структурировано. Это собирательное или есть хороший гайд по сказанному?

Дим, я ещё добавил бы что надо смотреть на наличие тестов. Весь ли новый код протестирован. Всё ли сценарии использования рассмотрены.

@SeniorSoftwareVlogger

5 жыл бұрын

Это можно и нужно автоматизировать :) но ты конечно прав!

Иногда ревью перерастает в войну сеньйоров за метод реализации.

@alexeymezenin

5 жыл бұрын

Иногда и джуны, которые думают, что они синиоры, воюют с синиорами.

@IngviMcFly

5 жыл бұрын

ахахах вот это тру ваще)))

@cojib9361

5 жыл бұрын

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

@user-xr4oe8qs2o

5 жыл бұрын

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

@soversus5374

2 жыл бұрын

Это когда у сеньера нет объективных обоснований, но есть борзометр. 😀

Хороший код ревью возможен в мелких пул реквестах. Бывает что в мелких можно найти больше исправлений чем в больших. Вопрос состоит в том, сколько файлов изменено или добавлено в одном пул реквесте. Мой опыт показал, что если больше 20 новых файлов то хорошо проанализировать не получится или придется затрачивать много времени на анализ. У кого другие цифры с количеством перевариваемых файлов ?

@SeniorSoftwareVlogger

5 жыл бұрын

Не больше 100 строк :)

@nnutkin

5 жыл бұрын

Не помню в какой статье, но проводили исследование и идеально это от 200 до 400 строк. Если больше то плохо. У вас в компании есть ограничение на размер файла ? У нас сейчас не больше 150 строк на файл. Если больше получается, то нужно дробить.

таже история, прийдя в проект был сильно демотиирован тем, что не смотря на то что код прошел автоматизированый checkstyle, я на первом весьма большом и архитектурно значимом MR получил десяток каментов про количество пустых строк и пробелы вокруг скобок. до сих пор с этим человеком работать некомфортно... осадочек

@soversus5374

2 жыл бұрын

Не расстраивайся. Это значит только одно: у тебя все хорошо, либо компетенции этого человека на более стоящее просто не хватает.

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

Спасибо за рекомендации. Можно узнать чем вы автоматизируете код стайл и как используете spell checker? Большая часть названия переменных, функций, классов и т.д. пишется при помощи какого-нибудь camelCase/snake_case. Как spell checker реагирует на такие слова? Он, ведь, их распознает как ошибочные?

@curtisjackson1992

5 жыл бұрын

В JetBrans продуктах все отлично распознает

@SeniorSoftwareVlogger

5 жыл бұрын

Eslint + prettier. Spell checker справляется, он же для программ, там предусмотрено

Мне кажется, или вы сейчас "Совершенный Код" вкратце пересказали?

проверка на solid ведь должна быть, и где можно использовать шаблоны проектирования ооп.

@alexander_farkas

5 жыл бұрын

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

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

@soversus5374

2 жыл бұрын

Непонятный вам? Зато понятный тому, кто писал и возможно другим. Учитесь читать чужой код. Эх, где 90-е... 😀 Там требовался навык уметь читать чужой код, и никого не интересовало, удобно вам или нет.

@pashakorenev7619

Жыл бұрын

@@soversus5374 а в чем разница между нынешним временем и 90-ми?

@soversus5374

Жыл бұрын

@@pashakorenev7619 , представь себе, что деньги носишь в чемоданах, а еду в кошельке. В том и разница.

Спасибо, кстати интересная тема. Но пункт 4 наверное должен быть пунктом номер 1, как там было ? "Постоянно спрашивай себя а не херню ли я делаю?" (с)

@sergeyblagodarnyy3278

5 жыл бұрын

Вот постоянно спрашиваю и часто не нахожу однозначного ответа. :)

@ilqlazar

3 жыл бұрын

Чтобы это понять. Сначала всё таки надо понять что именно надо сделать. По-моему, порядок у Димы корректный.

Интересно, это видео попало ко мне в рекомендации, потому что смотрел автора на днях ранее или из-за последних громких новостей о спонсоре выпуска?

@SeniorSoftwareVlogger

2 жыл бұрын

Да, вон и неделю назад людям рекомендовалось. Ютуб любит старые видео.

Привет, заметил что ты много читаешь и много сидишь за пека, но ни разу не видел тебя в очках. Расскажи как сохранил зрение без: "не читаю книги малого формата (с мелким почерком)", "зрение от природы острое", "никогда не задумывался об этом". Давай блеат делись.

@SeniorSoftwareVlogger

5 жыл бұрын

"не читаю книги малого формата (с мелким почерком)" "зрение от природы острое"

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

Очень четкая позиция. Как раз все обычно, как в первом случае, и даже четвёртый этап не проходит.

Дмитрий, доброго времени суток. Интересно Ваше мнение по заезженному вопросу - насколько целесообразно вставать на путь программиста в возрасте около или более 30 лет? Я, например, смотрю Ваш канал, просто потому что мне это интересно. Программирование привлекает в первую очередь тем, что этот вид деятельности подразумевает (в моём понимании) поиск способа решения проблем наиболее оптимальным методом, что по максимуму задействует голову. Но вот вопрос: сколько времени уйдёт на то, чтобы дойти до хорошего профессионального уровня и есть от профит. Понятно, что каждый решает для себя сам, интересно просто Ваше мнение.

@SeniorSoftwareVlogger

5 жыл бұрын

Привет! За 2-3 года можно прокачаться до миддла.

@ed_tomeyan

5 жыл бұрын

@@SeniorSoftwareVlogger спасибо большое за ответ. Ещё один вопрос. Скажите, пожалуйста, почему Вы выбрали именно Германию? Извиняюсь, если пропустил этот момент в одном из видео.

Дмитрий, если бы ты в code review проекта ruby увидел цикл for, ты бы придрался?

@SeniorSoftwareVlogger

5 жыл бұрын

Я и в джаваскрипт проектах в циклам for придираюсь.

@PankyHankyMr

5 жыл бұрын

почему?

@SeniorSoftwareVlogger

5 жыл бұрын

В теле for можно намесить столько всего, что не разберешь. Поток данных через программу лучше читается если оформлен через трансформации filter, map и reduce

@PankyHankyMr

5 жыл бұрын

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

@user-be2ke1cf4d

4 жыл бұрын

@@SeniorSoftwareVlogger а если безfor не обойтись?

Я новичок, но не очень понимаю, зачем тратить мыслетопливо на плохо написанный код, чтобы понять, что он делает и какой же там алгоритм. Тут, как мне кажется, важно, чтобы код был вначале понятен (intent), но для этого иногда нужны нижние уровни ревью, не так ли?

@SeniorSoftwareVlogger

4 жыл бұрын

Я не понял ход мыслей. Плохой код важно понять потому что он уже написан и его теперь нужно поддерживать. Идеальный код - это миф, которого никто никогда не видел.

@mikemerinoff

4 жыл бұрын

@@SeniorSoftwareVlogger Попробую переформулировать. Скорее, диада понятный\непонятный, чем идеальный\плохой. Приведу пример: мой первый коммент вначале нужно было понять или переформулировать, прежде чем спорить предметно :) Вот я про это: привести код к понятному виду вначале, а потом уже решать более общие задачи ревью. Например, Боб Мартин употребляет слово intent - намерение, мне нравится это определение. P.S. повторюсь, новичок, только вникаю, могу быть неправ.

На канале не хватает фирменной заставки)

@Das.Kleine.Krokodil

5 жыл бұрын

наоборот, круто - сразу к делу - "Привет, это Навальный" )

@artorias8532

5 жыл бұрын

Заставки не нужны.

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

@SeniorSoftwareVlogger

Жыл бұрын

Не часто. Команда как раз обычно работает в одном домене/проекте.

В чем проблема сначала добиться читаемого кода на ревью, а потом уже, когда это возможно читать - анализировать.

@SeniorSoftwareVlogger

Жыл бұрын

Если невозможно читать это другое дело. Обычно читать возможно, но можно улучшить и люди фокусируются на этом.

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

Для школьников самое то.

Кодил программер неделю фичу. На 1000 строк, допустим. Долго продумывал алгоритм, подходы и пр. Сколько по времени нужно въехать в суть написанного ревьюеру с таким подходом к ревью? Ведь ему по сути нужно много пройти через то, что прошел автор кода. Т.е. звучит это все правильно и красиво. А по факту такой подход к ревью растянется на многие дни. Ведь помимо ревью, есть ещё куча других задач. А менеджеру нужно все скорее-скорее, когда релизик... Поэтому и видят только опечатки, пустые строки, нэйминг и прочие очевидные вещи.

@SeniorSoftwareVlogger

Жыл бұрын

Неделю и 1000 строк. Плохой программист. Один раз с ревью завернут - научится разбивать на маленькие пул реквесты

@pashakorenev7619

Жыл бұрын

@@SeniorSoftwareVlogger а разве разработчик должен разбивать сам задачу на пулл реквесты? разве это решается не на уровне планирования тикета/субтикетов? или вы имели ввиду разбивать на коммиты внутри пулл реквеста?

Ультраповерхностно, dry, kiss, solid .... не не слышал

Дмитрий, привет! Есть предложение по новой рубрике. Если готов выслушать - отпишись пожалуйста на почту ttgm99@gmail.com

@SeniorSoftwareVlogger

5 жыл бұрын

не готов

Сразу с первых минут понял что ты как программист не очень. Именно по этому я фулстек. А вы обрубки полукровки