Pull to refresh

Comments 36

Code Review делается не для поиска багов. Да, во время ревью можно найти баги, но цель не в этом. Цель ревью в том, чтобы найти проблемные места. Те места, что могут работать не очень оптимально либо привести к проблемам в будущем. Так же, возможно, увидеть «велосипед», который нужно заменить на готовое решение. В средних и больших компаниях часто не все знают, что встреченная ими проблема уже была решена в соседнем отделе или группе.
А для поиска багов есть отладка и тестирование.
А еще для шаринга знаний. Проще итеративно понять небольшие изменения, чем целиком вникать в логику незнакомого кода.
Code Review делается не для поиска багов.

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

P.S. Результаты одного годного исследования эффективности разных способов поиска ошибок. Ревью — не худший способ.
image

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

А если эта ошибка уйдет в продакшен? Иногда лучше 5 раз отсмотреть, нежели потерпеть убытки. Иногда после таких багов компании просто закрываются. Само собой все зависит от типа приложения)

«использовать дорогих специалистов для поиска багов, видится мне не очень хорошей идей»
ой сколько уже на эту тему сказано ))) Тестер это далеко не «недорогой специалист». А уж если все это через аутсорс, то и дороже разработчика. Да, при ревью нет прямой цели найти баги (может я ща откровение выдам, но при тестировании тоже нет такой прямой цели), но уж если видишь в чужом коде потенциальную ошибку, то надо про это как то сообщить и разобраться.
Тестер обычно в коде вообще не найдет ничего. Находят как раз топовые спецы, которые на порядок луче автора кода

А можете поделиться ссылкой на исследование? Интересно было бы узнать подробнее, как получены выводы и как их трактовать.

Котик и собачка тут классные…
Хорошо, когда умеешь рисовать и даже скучные чеклисты/стайлгайды/списки советов смотрятся гораздо веселее и лучше усваиваются народом, когда они разбавлены забавными комиксами.
Кажется, что работа программиста превращается в работу психолога… Думаешь уже не о самом программировании, а о чувствах других людей)

Я не говорю, что это плохо, это наоборот хорошо и по другому наверное никак. Просто как-то странно. В каких ещё технических профессиях такое встречается?
UFO just landed and posted this here
Да о чем ты только подумал? Причем тут вообще программист и психолог? Это описание тонкостей нормального человеческого общения!

ну или же…

Действительно, любой род деятельности который подразумевает общение между людми требует соблюдения определенных правил, если мы конечно хотим чтобы общение было вежливым, эффективным и взаимовыгодным, а уж программисты там или электроники это действительно не важно.
У тебя ошибка в слове «людми»!
Ой, простите…
Мне кажется, что в слове «людьми» должен стоять мягкий знак. Нарушаются правила русского языка.
(применение на практике)
У проверяемого может появится чувство, что проверяющий сам дилетант и может предложить ему перекреститься.

Можно проще, сказать, что тут опечатка.

Такое не встречается в тех технических профессиях, в которых нет места спору на тему «я художник, я так вижу». К сожалению в программировании такие споры возникают сплошь и рядом.

А в каких профессиях такое не встречается? Разве что на уровне рабочего на конвейере, где есть технологическая карта и всё расписано до шага.
UFO just landed and posted this here
Сконцентрируйтесь на проблемах вроде перепроектирования интерфейса класса или разбиения сложных функций. Подождите исправления этих проблем, прежде чем переходить к низкоуровневым вопросам, таким как именование переменных или понятность комментариев в коде.


Логично. Однако как быть, если новый сотрудник использует имена: k1, k2, ...,k23, soxranitsnimokekrana, rasparsitviragenie, scanirovatmatritzy и т.д.? Читать код с подобными именами будут затруднительно.

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

К сожалению, при найме невозможно выявить все особенности. Нужно разбираться в причинах. Если это была своеобразная «штука юмора», то можно надеяться, что больше она не повторится после серьезного предупреждения. Гораздо хуже, если у кадра проблемы с английским. В оправдание можно услышать, что взял код из сетки и не стал исправлять имена, пока код не утвердили. Но для примера я взял крайний случай, бывают менее яркие, но все равно неудобные для чтения. Отдельная проблема для наших фирм — язык комментариев. Многие резонно настаивают на английском: не надо каждый раз переключать язык, не возникает путаницы с переводом терминов. Но многие возражают: в программе м.б. специфика зоны .ru, ревью пишут на русском и да знания английского м.б. достаточное, чтобы читать инструкции и спецификации, но этого мало, чтобы свободно выражать свои мысли на этом языке.

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

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


У этого рассуждения есть обратная сторона, конечно. Если ревьюер самоутверждается за счёт ревью, то у него тоже видимо не всё хорошо, и может ну его и далее по тексту. Но если замечания осмысленные и по делу, а не лишь бы придраться, то можно и потерпеть шершавость слога и отсутствие реверансов, фигурально выражаясь. Грань, безусловно, тонка. Я лично для себя решил, пока до прямых оскорблений не доходит, на свой счёт не принимать. Хотя, если доходит, то это тоже повод скорее обеспокоиться психическим здоровьем коллеги, чем оскорбиться.


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

UFO just landed and posted this here

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


Никого не хочу обидеть, но, возможно, проблема таки не в гибкости мышления (которая, конечно, уходит), а в старом-добром "комплексе опыта"? То есть на замечания/предложения подсознательная реакция "да я так уже 20 лет пишу, а ты молокосос пороху не нюхал"?


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

Спасибо за отличную статью! Разрешите предложить ещё одну крутую вещь, которую можно включить во вторую часть, хотя она больше относится к прохождению CR, а не проведению: раннее code-review. Например, спроектировали интерфейс — не надо сразу бросаться его реализовывать, покажите на code-review.
И еще вдогонку
Хотелось бы еще поинтересоваться опытом коллег по части немедленного проведения CR. С одной стороны всё правильно — блокируется работа коллеги, задача зависает перед отправкой на прод и т.д. Но с другой стороны переключение контекста может быть весьма дорогим и неприятным для разработчика, да и какая-нибудь рабочая встреча в самый неподходящий момент случится.

У себя в команде (8 разработчиков) мы ввели несколько правил:
  1. ревьювить может любой разработчик, и аппрувы всех равны, независимо от опыта и длительности пребывания в команде;
  2. для мерджа в мастер необходимо минимум два аппрува;
  3. ни один комментарий не должен остаться без ответа: коммита с исправлением или достигнутого вместе решения что можно не править

С одной стороны это хорошо: здоровая атмосфера и восприятие критики в команде, обмен опытом, в котором может участвовать каждый, чаще обнаруживаются баги и улучшения, CR проходят за приемлемое время. Но с другой стороны размывается ответственность, люди чаще думают «А ладно, посмотрю попозже, или еще кто-нибудь посмотрит», или ревьювят сквозь пальцы. И вот уже немедленной реакции нет, одну задаче прямо хорошо поревьювили, а другую «пойдёт» и т.д.

Кто как находит баланс в этой проблеме?
Привет, у нас в команде 5 разработчиков и мы на ретроспективе выработали правило ревьюить задачи асапом (+ у нас есть правило не мержить с мастером, пока не получишь аппрув от всей команды). На второй ретроспективе мы обсудили наше правило и единогласно решили его оставить. Почему? Как итог атмосфера в команде стала много лучше, люди стали оформлять коммиты более качественно, ты начинаешь быть благодарным что так много людей уделяют внимание твоему коду, что делаешь все необходимое чтобы им было понятней. Также вовлеченность в продукт возросла, люди в большем контексте засчет постоянного ревью и ввиду того что наши задачи пересекаются мы стали быстрее вникать в чужой код
Как по мне, code review только тормозят процесс разработки. У каждого разработчика свой стиль, свои предпочтения, свое видение, и каждое ревью это столкновение двух миров где побеждает обычно более авторитетный или вышестоящий разработчик. Если несколько человек изменяют/ревьювят один и тот же код — они в конечном итоге переделывают его под себя, под свое видение один после другого. Не редко в практике бывают такие случае:

Разработчик 1 написал код / сделал дизайн
Разработчик 2 во время разработки смежного функционала, заметил код, WTF? Я бы сделал по другому, меняет код.
Разработчик 3 во время ревью добавил что-то от себя
Разработчик 1 через некоторое время возвращается к своему коду, WTF?

Нужно попытаться построить процесс так чтобы минимизировать их количество. Для этого разбить проект на слабосвязанные компоненты, модули, назначить ответственных за каждый модуль и пусть они комитят туда без каких-либо ревью — если компонента работает хорошо, не ломается, то разве не все-равно как она написана? Другое дело если сторонний человек вынужден сделать там какие-то изменения, то тогда ответственный за компоненту человек должен эти изменения ревьювить.
Согласен, что есть много проблем с code review. И это может сильно тормозить процесс разработки. Но если компонента работает хорошо, то это не значит, что будет работать хорошо в следующей версии программы. Если разраб этой компоненты перейдет на другую работу, то другой м.б. вынужден сделать там какие-то изменения.
если компонента работает хорошо, не ломается, то разве не все-равно как она написана?

Не всё равно. Через пару лет этот человек уволиться (или его переедет самосвал), а код останется. И оставшимся нужно будет его поддерживать: добавлять новый функционал, исправлять старые ошибки, допиливать под новые платформы. Если код нечитабельный, то уйдёт уйма времени на его чтение и понимание.
В пределах команды (а желательно в пределах кампании) видение о том как правильно писать код должно быть одинаковым у всех разработчиков. Код написанный одним должен легко, как интересная книга, читаться другим. Code review в основном как раз для этого и нужен. Если вы видите непонятные имена, длинные функции, не влезающие в экран, запутанные алгоритмы, там где можно было бы обойтись простыми, отсутствие комментариев, там где они должны быть и т.д. — напишите об этом автору. Просматривайте чужой код так, будто вам завтра поручат исправлять в нём ошибки.
Для синхронизации видения есть Coding Style Guide. Есть такой документ на всю кампанию и часто бывают дополнения к нему используемые внутри команды. CSG вырабатывается в результате обсуждения и голосованием выбирается тот вариант, который устраивает большинство. Вам придётся следовать этому документу, даже если он вас не устраивает.
UFO just landed and posted this here
Мы можем исправить грамматическую ошибку в слове «соврешенно»? =)
Вся эта толерантность и терпимость к ошибкам других, запрос с поклоном вместо четкой команды произрастает из внутренней атмосферы коллектива.
Как самый верхний уровень компании ведет дела так всё происходит и ниже.
А уточните, пожалуйста кого вы считаете «самый верхний уровень компании»? Тот кто вам денежку платит или тот кто читает ваш код? Мне, с моим опытом, не попадался начальник которого интересовал бы мой «внутренний мир», начальника интересует что б ты делал работу качественно и вовремя, и что б у заказчика не возникало вопросов по поводу твоей работы. А вот тот кто читает твой код и тратит на тебя свое время, заслуживает уважение и взаимопонимания.
Снизу вверх скорее да, сверху вниз скорее нет. Рецензент должен быть выше по квалификации чем рецензируемый, поэтому все ужимки «не обидеть замечанием» неуместны.
Хорошие и практичные советы для рецензента на самом деле.
В который раз возвращаюсь к этой статье, чтобы проверить рецензию.
Sign up to leave a comment.

Articles