Привет, Хабр! Сегодня поговорим о код-ревью, т. е. проверке и оценке качества кода выполненной разработчиком задачи перед её релизом. У код-ревью несколько положительных моментов:
поиск багов и проблем кода, что значительно снижает вероятность проникновения этих багов в релиз
вовлечение членов команды в работу над различными задачами, к которым участники код-ревью часто не имеют прямого отношения. Это даёт лучшее понимание общего процесса работы
повышение качества кода и, наверное, общих стандартов разработки в компании, поскольку представители команды видят, что и как нужно делать для повышения качества работы
социальное взаимодействие в команде. Её представители дают друг другу обратную связь, постепенно повышая свой скил коммуникаций
Но на что обращать внимание в процессе код-ревью? Об этом сегодня и поговорим.
Чек-лист для код-ревью
Стоит отметить, что в коммерческой разработке код-ревью давно стал неотъемлемой частью работы. Он не должен быть чистой формальностью, поскольку в этом случае снижается значение всего процесса. Вот основные пункты для проведения код-ревью.
Соответствует ли реализация исходным условиям задачи?
Речь идёт о том, что код должен делать то, что нужно конечному пользователю программы или сервиса. Разработчик должен написать код, который удовлетворяет ожидания пользователя. Если это не так, код нужно доработать. Вот три важных вопроса:
Не упустил ли разработчик реализацию какой-либо фичи, важной для конечного юзера?
Возможно, есть частично/плохо реализованные фичи?
Можно ли добавить дополнительные возможности в будущем, если они понадобятся?
Читаемость кода
Есть несколько важных моментов относительно читаемости кода. Например:
Разбит ли весь код на понятные стороннему ревьюеру блоки?
Может ли ревьюер различить роль конкретных функций, методов и/или классов?
Не встречаются ли в коде непонятные выражения?
Есть ли необходимые для понимания кода комментарии? При этом комментарии должны объяснять, почему кодер сделал именно так, а не как он это сделал.
Соответствует ли код принятым в команде/компании гайдлайнам?
Одинаково ли хорошо отображается код на экране ноутбука и большом мониторе? Этот пункт необязателен, но часто желателен.
Зачастую дискуссии в команде возникают из-за непонимания назначения и принципа работы готового кода. Так что лучше писать/переписывать код так, чтобы вопросов о назначении определённого участка просто не было. Это не всегда возможно, но желательно.
Простота обслуживания кода
Об этом уже упоминалось выше — после того как софт или сервис готов, его в будущем, возможно, понадобится доработать, расширив возможности. И чем проще это можно будет сделать, тем меньше проблем получит разработчик или команда. Вот список вопросов, которые помогают понять, насколько просто будет сопровождать готовый код в будущем:
Не базируется ли код на какой-то устаревшей технологии/фреймворке?
Легко ли тестировать и отлаживать готовый код?
Можно ли быстро изменить значения данных?
Не чрезмерно ли усложнён код? Возможно, какие-то участки можно упростить?
Есть ли документация (употребимо для сложных проектов/задач)?
Информационная безопасность
В коде будущей программы/сервиса могут быть уязвимости, которые разработчик не заметил. Это крайне важно для коммерческой разработки, поскольку ИБ практически везде поставлена во главу угла. Вот список вопросов, которые стоит задать себе при ревью кода на подобные проблемы:
Проводится ли валидация текстового поля?
Надёжно ли хранится пользовательская информация?
Какого рода используется аутентификация и авторизация для обеспечения безопасности пользовательских данных?
Не используются ли в коде технологии с известными проблемами в безопасности?
Скорость работы и потребление ресурсов
Современное ПО часто ругают по поводу того, что оно потребляет большое количество ресурсов устройства. Банальный браузер может подвесить не слишком производительный ноутбук, не говоря уже о другом ПО. Поэтому разработчику стоит следить за тем, чтобы баланс производительности и потребления ресурсов его продукта был оптимальным.
Вот моменты, на которые стоит обратить внимание:
Не дублируется ли код?
Есть ли в коде конкатенации строк, предусмотрено ли ведение журнала или присвоение объектов, что может влиять на производительность готового продукта?
Нет ли в коде плохо оптимизированных участков, ненужных/множественных запросов API и т. п.?
Оптимальный размер кода для ревью и ещё парочка нюансов
В целом, всё зависит от разработчика/ревьюера/команды, но вообще чем меньше участок для ревью, тем лучше. Дело в том, что огромные участки кода обычно сложно оценивать. Они часто получают оценки вроде «всё ок», без подробных комментариев/обратной связи. Всё дело в том, что время ревьюера не резиновое, и если в небольшом участке кода он может оставить несколько комментариев, то в крупном фрагменте осилить сотню-другую комментариев вряд ли кто-то способен. Особенно, если горит собственная задача.
Конечно, эффективно работать с фрагментом любого размера позволяют инструменты. Если мы говорим о git, то требуется понимать базовые операции, включая commit, rebase, squash, reset, chunk.
Ну а относиться к ревью кода — как своего, так и чужого — нужно серьёзно. Если вы принимаете изменения, значит утверждаете, что в коде всё хорошо. Не стоит принимать пул-реквесты, не изучив код. В противном случае есть риск попадания некачественного кода в базу, соответственно, повышается вероятность появления ошибок/багов в продукте.
Одобрив код, ревьюер получает свою долю ответственности за поведение продукта уже в продакшне. Многие компании используют этот принцип в ежедневной практике работы — и для многих это весьма полезно.
В целом, код-ревью — очень ценный инструмент в процессе разработки любого программного продукта. И если использовать этот инструмент правильно, то качество таких продуктов повысится, равно как и профессиональный уровень разработчиков команды, где применяется код-ревью.
K_Chicago
по-моему, какой-то бессмысленный бюрократический текст не имеющий никакого отношения к реальности (я надеюсь. А если имеет - бегите из этого места так словно за вами черти гонятся).
Я вообще плохо понимаю эту идею "code review" когда имеют ввиду этакое публичное мероприятие напоминающее "товарищеский суд" времен спелого СССР.
Выглядит это так: отрывают от работы девелоперов и велят всем собраться в meeting room. Выходит один из них, открывает на экране проектора несколько страниц кода и коротко как умеет говорит что это за код, для кого он и что он в самых общих чертах делает. Ну, минуты 2-3 говорит.
И вот.
Например, сферический девелопер в вакууме из этой группы, сидит, смотрит тупо на эти страницы кода. У него в голове собственные задачи, у него дедлайн, он понятия не имеет над чем этот парень работает и не видит никакой необходимости в это вникать, у него своих проблем выше крыши. Разумеется, логику кода сходу понять он и не может и не хочет. Что он может отревьюить глядя в первый раз на сотни строк неизвестных ему процедур и функций, над которыми автор кода работал может быть несколько недель?
И разумеется он может только начать обычную баланду про "неправильные" имена переменных, про "мало комментариев", про "вот здесь согласно нашим гайдлайнам нужно использовать Tab а не три пробела" - вот весь этот бред. Это и есть "социальное взаимодействие в команде. Её представители дают друг другу обратную связь, постепенно повышая свой скил коммуникаций". Поставили галочку, "дали друг другу обратную связь", дело сделано, менеджер может включить в свой отчетик.
Теперь, как я понимаю "код ревью" здорового человека.
Я говорю менеджеру - "API по вот этому тикету готово, деплоить в препрод будем?"
Он отвечает - "ага, пасиб, пошли Джону пусть он посмотрит", и пишет Джону - "Джон, пожалуйста review this code когда будет время".
Джон берет мои файлы из гита, день другой их листает неторопясь, спрашивает у меня тест скрипты, может задать пару вопросов " а вот это зачем", "а здесь что"... потом, погоняв тесты, пишет мне "давай вот здесь еще один exception добавим, а то вдруг мало ли",
- я добавил, попробовали - "ну, теперь вроде нормально", пишет менеджеру и cc мне.
Менеджер отвечает - ну OK, кладите в PrePROD, я напишу в QA Team пусть смотрят.
Вот именно так это проходит в фирме где я сейчас работаю. И такое случается вовсе не каждый раз, а скорее как исключение, когда данный кусок кода действительно и сложный и важный, и этот самый Джон тоже в курсе этих проблем, а в обычных случаях такая двойная проверка не производится, все люди опытные и работают над этой темой не первый год.
А когда я слышу вот это вот "вовлечение членов команды в работу над различными задачами, к которым участники код-ревью часто не имеют прямого отношения. Это даёт лучшее понимание общего процесса работы" - "я хватаюсь за пистолет" (с). Потому что мне и нафиг не нужно вовлечение в работу к которой я не имею прямого отношения. Мне нужно заниматься своим делом. Мне не нужно путем публичных экзекуций улучшать "понимание общего процесса работы" - мне платят совсем за другое.
MyraJKee
Ну такое.
Во-первых, чтобы в задаче не было 100500 строк кода, обычно делают декомпозицию. Потому что это в том числе и ревьюерам облегчает ревью.
Во-вторых, как правило к ревью привлекаются обычно люди, которые что-то по данной задаче понимают. Как правило, если это крупная компания, сотрудники разбиты на команды, каждая из которых работает в какой-то узкой области и хоть кто-то да должен понимать что делалось в рамках задачи. Если мелкая то и так понятно что каждый примерно в курсе что делает другой.
Я хз где вы работаете, у меня в компании задача без ревью идёт в тестирование в очень редких случаях.
K_Chicago
Во-первых, я по-моему не говорил про 100500 строк кода. А какое количество, по-вашему, уместно для ревью? 10 строк? 100? 200?
Во-вторых, таки да, обычно по данной задаче могут быть еще один, максимум 2 человека которые понимают. Я полагаю, код ревью на котором кроме автора присутствует еще 1 человек из его команды, было бы уместно. Но ведь речь о каком-то публичном мероприятии, о котором я и пишу.
Я работал и работаю в довольно крупных фирмах, например в monster.com (не уверен что вы в курсе что это такое), или в одном из 5 крупнейших банков США, - и нигде вот такого публичного ревью не встречал.
Может быть это случается в каких-то стартапах где ничего не устоялось и они сами не понимают как разрабатывать, вот и маются дурью со всеми этими "методологиями разработки". Хотя конечно и в больших фирмах корпоративный идиотизм встречается. Вот на последней работе большой начальник с самого верха приказал что вся разработка всех проектов должна производиться только по методологии Agile. Это примерно как приказать на заводе во всех процессах и всех цехах использовать только гаечный ключ 9х13 и никаких других. Ну, потерпел этот цирк 4 месяца, уволился. Сейчас опять в нормальной фирме, зарплата больше, нагрузка меньше.
А у вас как?
MyraJKee
Слышал про monser.com. полагаете что это должно что-то говорить о вашей квалификации?
Про 100500 вы сами же пишете "сотни строк". По моему чем меньше строк, тем лучше.
У нас нет понятия публичное ревью. Если нужен кто-то конкретный-два-три человека, просят проревьюить индивидуально. Если нет такой надобности, то кто-то любой, у кого есть время и понимание предметной области. Это может быть 1-2-3 человека. Если надо, то проводится кросскомандное ревью. У нас и близко нет вот этого всего аджайла и методологий. Но тем не менее задача без ревью в прод обычно он идёт
DmitriyGordinskiy
Ну да, все же программисты с первого дня работы становятся опытными и получают знание всего, будто работают над этой темой не первый год
K_Chicago
плохо понимаю ваше яростное сарказмирование, но в нашей команде (около 20 человек) нет никого со стажем меньше 10 лет. Новичков после колледжа мы не нанимаем. Разумеется, когда приходит новый человек (такое случается редко, может, раз в пару лет) - за ним первое время присматривают, не все подряд конечно, а его тимлид или просто кто-то кто работает в этой же теме. Когда я пришел, например, пол-года примерно за мной смотрели, мой код обязательно кто-то аппрувил. Это нормально. Потом после первых самостоятельных проектов такое случалось все реже. Сейчас обычно менеджер может коротко взглянуть на код, и то не всегда, и разумеется в QA идет только после его аппрувала.
Наверное, где-то где огромная текучка и набирают исключительно первогодок, такие код ревью и уместны...но за 18 лет работы в США я нигде такого не встречал.
YuryZakharov
Публичное ревью в статье ни разу не упомянуто. Это Вы каких-то своих тараканов гоняете, зачем нам о них знать?