Правила хорошего ревью кода / Code review

Как правильно делать ревью кода, на что обратить внимание в первую очередь, а что автоматизировать раз и навсегда.

[spiritvlvl]

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

[mmospanenko]

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

[MaxPronko]

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

[iblbzws]

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

[MrUrfenJus]

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

[nikth]

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

[feosTAS]

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

[AlexanderSchepanovski]

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

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

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

[realCodeXP]

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

[pruchay]

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

[nnutkin]

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

[sergeyblagodarnyy]

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

[hnsojjb]

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

[GreenFlashk]

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

[SoulPervert]

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

туда же "джуновские" ревью - там и вопросы "а это вообще работает?", и именование, и корректность комментариев (не наличие, а соответствие реальности), и всё что угодно еще

[knmoves]

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

[gavluk]

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

[IngviMcFly]

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

[diamondthings]

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

[RuslanKovtun]

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