Практика, которой нужно следовать: code review

30 комментариев
Практика, которой нужно следовать: code review
skitchedМарк Чу-Кэролл, PhD в области computer sciences и инженер по разработке ПО, ранее работавший в Google, в одном из постов в своём блоге поднимает тему о важности code review в серьёзной разработке ПО, при этом признавая бесполезность данной практики в отлове багов. Какой на самом деле толк от практики code review и о чём надо помнить, когда вам самим придётся просматривать код ваших коллег в рамках данного процесса, – в свободном переводе небольшой статьи Марка. "Код Google так хорош в первую очередь благодаря code review. Конечно, это не какое-то ноу-хау, code review – идея не новая и давно зарекомендовавшая себя на практике. Но я не знаю другой такой крупной компании, где бы данный метод использовался столь повсеместно и универсально. В Google код для любого продукта, для любого проекта будет проверяться до тех пор, пока не получит положительного ревью. Каждый разработчик должен делать code review. И это не значит, что данный процесс какой-то неформальный и зависит от собственных желаний и устремлений, проведение code review – это универсальное правило серьёзной разработки программного обеспечения. Не просто код продукта, а вообще весь код, любой. На самом деле тут не так много работы, но разница в конечном результате получается значительная. Начнём с очевидного самого понятия code review: вторая пара глаз просматривает код перед тем, как он отправится на проверку на баги. Это наиболее широко встречаемая, упоминаемая и признанная практика code review. Но, по моему личному опыту, польза от code review по нахождению багов достаточно невелика. Подавляющее большинство багов, которые обнаруживаются при подобном просмотре кода, если говорить откровенно, примитивны и очевидны, и сам автор кода с лёгкостью и сам бы нашёл и поправил их за пару минут. А те баги, которые действительно достаточно сложны и требуют времени на нахождение, обычно при подобном code review не обнаруживаются. На самом деле code review несёт больше всего пользы в явлении "социализации" кода. Если вы программируете, зная, что ваши коллеги будут потом просматривать ваш код, вы программируете немного по-другому, нежели обычно. Вы пишете более аккуратный, более организованный и документированный код, потому что вы знаете, что люди, чьё мнение вам важно (а это ваша репутация), будут просматривать ваш код. Без code review вы знаете, что рано или поздно кто-то будет смотреть и разбираться в вашем коде, и это произойдёт не сразу, нет того уровня срочности и вовлечённости вашей команды в вашу работу. Соответственно, и стиль кодирования у вас будет попроще и "под себя". Есть ещё один неоспоримый бенефит от code review. Code review помогает распространению знаний о проекте. В большинстве команд у каждого программиста есть свой ключевой компонент, разработку которого ведёт он и за который, собственно, несёт ответственность. А пока компоненты коллег особо не пересекаются с его кодом, разработчик о них имеет весьма смутное представление. В результате код каждого компонента хорошо знает только один человек. И если такой человек уходит в отпуск или, прости господи, совсем покидает команду, никто толком не знает, что и как реализовано в его компоненте. Используя code review, у вас в каждом случае будет уже как минимум два человека, знакомых с кодом – автор и, так сказать, рецензент-ревьюер. Конечно, последний не знает код в той же степени, что и автор, но понимает структуру и дизайн компонента, что очень важно в случае необходимости быстро разобраться в каком-то вопросе относительно этого кода. Конечно, не всё так просто. Из своего опыта могу сказать, что должно пройти определённое время, прежде чем вы научитесь быстро и качественно просматривать код. На этом пути есть определённые подводные камни, о которые часто спотыкаются неопытные ревьюеры, и, соответственно, у них складывается отрицательное впечатление о данной практике, а неудачи становятся барьером на принятии концепции code review. Самое главное правило code review: вы ищите проблемы в коде перед тем, как он будет закоммичен, ваша цель – корректность кода. Наиболее распространенная ошибка, обычно свойственная новичкам, – это то, что они судят о том, как код написан исходя из своих собственных представлений о решении данной задачи. Для любой проблемы обычно существует добрая дюжина возможных решений, а для каждого решения – миллион вариантов реализации в коде. Ваша работа как ревьюера не в том, что убедиться, что код написан так, как вы бы его написали. Он всё равно таким не будет. Ваша задача убедиться в том, что написанный вашим коллегой код – корректный. И если этому правилу не следовать, само собой разумеется, что все участвующие будут разочарованы, и могут возникнуть даже какие-то обиды, а это уже совсем нехорошо. Возникновение подобной ошибки – явление по своей природе абсолютно закономерное. Вы программист и когда смотрите на проблему – видите решение, и впоследствии во время code review исходите именно из своего увиденного ранее решения. Но забудьте о своём решении, для хорошего code review оно вам не нужно, и вам надо это понимать. Второй основной подводный камень в бурном потоке code review – это чувство обязанности что-то сказать. Вы знаете, что автор потратил много времени и усилий, работая над кодом, и вы же должны ему сказать хоть что-то по коду, не так ли? Нет, не так. Нет ничего плохого в том, чтобы просто сказать "Ок, нормально". Если вы постоянно нацелены на поиск каких-либо зацепок для критики, то тем самым вы только разрушаете собственный авторитет в глазах у коллег. Когда вы раз за разом критикуете только для того, чтобы сказать хоть что-то, люди быстро поймут, что вы говорите всё это только для того, чтобы заполнить тишину, и ваши комментарии независимо от их адекватности не будут впоследствии приниматься серьёзно. А третья проблема – скорость. Не следует торопиться, но нельзя и затягивать процесс. Ваши коллеги ждут вас. И если вы и остальная команда не хотят проводить ревью, и проводить быстро, сама практика просмотра кода приведёт только к всеобщему разочарованию. Code review не должен выглядеть вмешательством в рабочий процесс вроде "а сейчас все всё бросят и будут ревьюить". Нет смысла бросать все дела, если кто-то попросил вас сделать code review. Через пару часов после того, как вы разберетесь со своей задачей, возьмёте паузу для чашки кофе или беседы с коллегами и только потом примитесь за ревью – вы сделаете его и сделаете достаточно быстро. И никому не придётся болтаться без дела в ожидании, пока вы сможете заревьюить код".
источник: scientopia.org, фото: flickr.com/photos/notme2000

Хотите сообщить важную новость?

Пишите в наш Телеграм

Горячие события

EMERGE 2020
1 июня — 3 июня

EMERGE 2020

Вебинар «Советы от рекрутеров: как найти квалифицированную работу в Европе»
4 июня

Вебинар «Советы от рекрутеров: как найти квалифицированную работу в Европе»

Читайте также

Разработчики из AIMatter рассказали, чем занимаются в Google
Разработчики из AIMatter рассказали, чем занимаются в Google

Разработчики из AIMatter рассказали, чем занимаются в Google

В августе прошлого года СМИ сообщали: открытый проект Google помогает распознавать язык жестов с помощью смартфона. Сейчас команда готовится представить свой проект на технологической конференции Emerge, которая пройдет 1–3 июня. Накануне выступления разработчики Валентин Базаревский и Иван Грищенко рассказали Dev.by, как далеко зашло распознавание жестов, и может ли их нейросеть отличить жесты человека от жеста робота.
9 комментариев
Вышла Android Studio 4.0
Вышла Android Studio 4.0

Вышла Android Studio 4.0

Объём скачанных приложений в 1 квартале вырос на 52%
Объём скачанных приложений в 1 квартале вырос на 52%

Объём скачанных приложений в 1 квартале вырос на 52%

Google и Apple выпустили первую версию технологии для отслеживания контактов 1-го уровня
Google и Apple выпустили первую версию технологии для отслеживания контактов 1-го уровня

Google и Apple выпустили первую версию технологии для отслеживания контактов 1-го уровня

1 комментарий

Обсуждение

agentcooper
agentcooper PM в SK hynix memory solutions Eastern Europe
1

Замечательная заметка - коротко, по делу и небанально.

1

> Марк Чу-Кэролл, PhD в области computer sciences и инженер по разработке ПО, ранее работавший в Google

Это тот, который раньше писал в блог Good Math, Bad Math (http://scienceblogs.com/goodmath/)? Марк -- классный!

> в одном из постов в своём блоге поднимает тему

А где сам пост можно прочитать?

EDIT:
Ну вот, поискал, сам нашел -- http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/

-1

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

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

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

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

agentcooper
agentcooper PM в SK hynix memory solutions Eastern Europe
1

В мою бытность n лет назад техническим менеджером на продукте с командой около 30 человек практиковалось следующее - при любом коммите сначала прогонялась автоматизированная компания, затем специально заточенному человеку (собственно я им и был) автоматически посылалось письмо с листингом всех изменений файлов (команда diff). В большинстве случаев делалось "light review" , занимавшее какие-то минуты, если возникали вопросы - приглашался разработчик для совместного просмотра и обсуждения - опять же как правило от пяти до тридцати минут. Интеграция автоматически происходила после валидации ревьюером. (На самом деле workflow был прилично более навороченным, но суть такая). Случалось конечно, что коммиты становились в очередь на ревью на дни и недели, но продукт был большой, итерации длинные и работало это в целом на 5 (по пятибальной системе). Вспоминаю с удовольствием.

Максим  Гулевич
Максим Гулевич Дизайнер в EPAM
2

Кстати, и работать интереснее! Не шучу ни разу — и команду сплочает.

-1

"причём делать ревью желательно ДО написание первой строчки кода, подойти к коллегам и устроить минимитинг: вот надо решить эту задачу, я собираюсь делать так... это сэкономит кучу времени." - повбывавбы!

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

Напоследок: просто представьте, что так как вы предлагаете ведут себя ВСЕ в команде из 10 человек. Ахтунг! Если каждый из 10 оторвет еще хотя бы 5 коллег на час в день, то в минус уйдет 60 человекочасов из 80 возможных. То есть вместо работы будет один сплошной митинг.

Митинги конечно нужны, но СВОЮ РАБОТУ НАДО ДЕЛАТЬ САМОМУ! Ибо нефиг.

-1

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

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

1

Если 1-5 минут именно во время перекура или обеда это нормуль. Но когда народ начинает реально ходить по каждому чиху, не пытаясь даже разобраться в проблеме самостоятельно - это пожалуй один из немногих моментов, которые лично меня нереально бесят с полпинка :) Почему собственно вчера вам ответ так несдержанно и написал на эмоциях. За что извиняюсь.

Про часовые митинги по любому поводу - это реальный, к сожалению, пример. Лично работал в такой "команде".

По последнему пункту есть оригинальное мнение:
1. Лично я программист, а не преподаватель ВУЗа, ПТУ, школы. Моя основная работа программы писать, а не молодых натаскивать.
2. Лично я учился и разбирался с большего почти во всем сам. Это не сложно. Совершенно не обязательно долбать мозг коллеге тупыми вопросами, когда можно самому почитать спеки и код коллеги, погуглить и так далее. Пусть на это уйдет больше времени, но усвоится лучше и коллегу отрывать не будешь.
3. Если компания не может: ни набрать хороших программеров, ни даже набрать джуниоров, которые способны сами учиться, то извините - это проблемы компании. Пусть тогда оплачивают этим джуниорам специальные курсы и специальных учителей.
4. В поднатаскивании команды компания заинтересована, но лично для себя не вижу тут совершенно никакого интереса. Максимум 1-5 минут во время перекура и при походе на обед. И то, в качестве жеста доброй воли. Компания за это не платит.

И последнее: считаю, что надо заниматься одним делом, чтобы сделать его хорошо. Если одновременно пытаться сделать свою работу хорошо и кого-то еще научить, то в итоге скорей всего и своя работа будет сделана не айс и человек, которого учишь все равно бестолочью получится :)

0

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

1

Раз был помянут Google, вот тут достаточно интересная статья о том как там работают с исходниками, в том числе немного затрагивается именно тема статьи: code review
http://scm-notes.blogspot.com/2011/05/google-scm-code-review.html

В частности там есть ссылка на видео с великим Guido Van Rossum о его туле для code review.
http://www.youtube.com/watch?v=sMql3Di4Kgc

В комментариях ещё упомянули секту facebook, у них тоже есть интересные идеи как работать с кодом
http://framethink.wordpress.com/2011/01/17/how-facebook-ships-code/

1

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

-1

:) не забудьте при этом добавить - сегодня ты не соблюдаешь коде стайл, завтра предашь Родину.

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

Anonymous
Anonymous администратор в БелМобайлСофт
1

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

Все описанное исключительно следствие слабого менеджмента.

-1

я очень рад, что у вас менеджмент поставлен лучше чем у apple и они могут любого перевосписать.

Anonymous
Anonymous администратор в БелМобайлСофт
1

Упс, а при чем тут Apple?

0

а вы пробовали читать что комментируете http://dev.by/blog/35174#comment-29053? я привёл пример, когда гения вышвырнули за дверь в apple и это пошло обоим на пользу. нельзя из-за талантливейшего кадра ставить под удар всю компанию. менеджмент же у apple один из лучших в мире и они будут повесомее вашего представления о "слабом".
кстати, линию можно продолжить, особенно вспомнив о временах доткомовских пузырей, когда преклонялись перед такими гениями и затем теряли всё.

Anonymous
Anonymous администратор в БелМобайлСофт
1

Т.е. Вы считаете, что отставка Стива Джобса произошла из-за того, что он был гениален и бунтовал? По моему он сам был директором, это не история про рядового, но талантливого программиста. Это вообще не типичный пример, как можно делать из него обобщающие выводы!

1

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

Anonymous
Anonymous администратор в БелМобайлСофт
1

Разборки на уровне топ менеджмента компании - это одно, там столкновение интересов, там вопросы финансов, а разборки на уровне: талантливый программист - "талантливый" менеджер - это совсем другое, это - слабый менеджмент!

1

А какие РЫЧАГИ ВЛИЯНИЯ есть у менеджеров? раз два и обчёлся, в итоге всё сводится к финансовому интересу. А на рынке нехватка кадров и плевать хотел средний разработчик даже на талантливого менеджера, если тот пытается заставить переступить через свои привычки. И менеджер может маневрировать только в неких рамках, а после этого проще либо расстаться с работником. либо он сам уйдёт.

Anonymous
Anonymous администратор в БелМобайлСофт
1

Я считаю, что работа с людьми - КРАЙНЕ деликатное дело. И есть множество инструментов и стимулов, которые нужно грамотно использовать. Задача бизнесмена - делать людей счастливыми! Подходы типа "вышвырнуть талант из команды" по причине того, что он мыслит иначе - это грубое наихудшее решение. Поверьте, я имел в жизни опыт неосторожных обращений с людьми, кроме угрызений совести это не приносит ничего, ничего хорошего ни Вам ни команде. Да и после таких шагов и команда на руководителя смотрит уже иначе, теряется доверие.
Конечно можно строить бизнес по принципу "быдло" снизу, а Я вверху... И чем все это закончится?

1

причём здесь мыслит иначе, вышвырывать, вы бросаетесь в крайности. мы же не обсуждаем что программисту нравится тёмное пиво, а вся остальная команда любит светлое, а рабочие вопросы: стиль кодирование и наименования (многие команды используют code sniffer не позволяющий коммитить код с плохим стилем), есть любители использовать свои фреймворки в которых разбирается 1-2 человека в команде, сейчас волна экспериментаторов NoSql, я лично недавно проводил разъяснительную беседу почему нельзя было делать на web socket-aх и почему эта нюшечка вдруг не взлетела в других браузерах.
Да, с такими нужно разговаривать и пытаться переубедить, но если результата нет, то лучше расстаться, чем доводить до хаоса в команде.

Anonymous
Anonymous администратор в БелМобайлСофт
1

Что значит "результата нет"? Как может не быть результата? Мы говорим об абсолютно тупом программисте, который не понимает простых требований? Если ПМ сказал делать код таким-то, ревьювить так-то, комитить после ревью, отчеты в конце дня, зарплата по результатам, отчетам, и т.д. Как такое может быть, что кто-то не выполнил такую постановку задачи! Только если задача не было четко поставлена, не были сформулированы прозрачные правила игры, не выстроена система бонусов, стимулов.

1

О, кстати, прекрасный пример. я ушел из одной компании после того, как меня попытались заставить писать отчёты. Три раза объяснял почему я не хочу постоянно писать отчёты, а после третьей попытки сказал, что буду уходить. Точно так же я бы не согласился коммитить после ревью выше писал почему http://dev.by/blog/35174#comment-29016. То что одному кажется протосй вещью другому может показаться неуместной тратой времени и проще найти место процессом поставленным так, как тебе ближе.

1

code review необходим хотя бы для того, что бы не было
- функций типа SeachEdenita(string Kod) - реальный пример, функция которая в какой-то мере делает поиск единицы в строке, при этом это не главное ее назначение
- компонентов названных Edit1, ComboBox22
- дублирования кода
- запросов "select * " или обновления данных в цикле
- и т.д.

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

george
george Engineer в EIS Group
1

"butteff: Ну я кота к лотку приучил так: Тыкал в говнокод и бил! Теперь он великолепно программирует на лотке."

...
Но, говнокод - это отдельный сабж... Код ревью - это чтото-более возвышенное в моем представлении =)

-1

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

1

Полностью поддерживаю. Считаю хороший код, хорош в "мелочах" (как для многих изначально кажется). Хотя бы требуется правильно назвать функцию/переменную в соответсвии с функциональностью.
Логичное и обаснованное разбиение на подпрограммы, разложение по пакетам.
Комментирование программируемого кода.
Уберай код-мусор после себя.

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

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

0

>> Хотя бы требуется правильно назвать функцию/переменную в соответсвии с функциональностью.

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

0

Гайдлайны компании в первую очередь и спецификация к проекту нужны чтобы проводить какие-то code review, иначе оно бесполезно.

Спасибо! 

Получать рассылки dev.by про белорусское ИТ

Что-то пошло не так. Попробуйте позже