Comments 36
А для поиска багов есть отладка и тестирование.
Code Review делается не для поиска багов.
Вы так уверенно заявили, как будто правы. Ревью вполне может производиться именно с целью поиска определенного класса ошибок. Практикуем.
P.S. Результаты одного годного исследования эффективности разных способов поиска ошибок. Ревью — не худший способ.
Я не знаю что по горизонтали в этом исследовании. Но, если принять это за эффективность, то использовать дорогих специалистов для поиска багов, видится мне не очень хорошей идей. Так что да, ревью не для поиска багов.
А если эта ошибка уйдет в продакшен? Иногда лучше 5 раз отсмотреть, нежели потерпеть убытки. Иногда после таких багов компании просто закрываются. Само собой все зависит от типа приложения)
ой сколько уже на эту тему сказано ))) Тестер это далеко не «недорогой специалист». А уж если все это через аутсорс, то и дороже разработчика. Да, при ревью нет прямой цели найти баги (может я ща откровение выдам, но при тестировании тоже нет такой прямой цели), но уж если видишь в чужом коде потенциальную ошибку, то надо про это как то сообщить и разобраться.
А можете поделиться ссылкой на исследование? Интересно было бы узнать подробнее, как получены выводы и как их трактовать.
Хорошо, когда умеешь рисовать и даже скучные чеклисты/стайлгайды/списки советов смотрятся гораздо веселее и лучше усваиваются народом, когда они разбавлены забавными комиксами.
Я не говорю, что это плохо, это наоборот хорошо и по другому наверное никак. Просто как-то странно. В каких ещё технических профессиях такое встречается?
ну или же…
Действительно, любой род деятельности который подразумевает общение между людми требует соблюдения определенных правил, если мы конечно хотим чтобы общение было вежливым, эффективным и взаимовыгодным, а уж программисты там или электроники это действительно не важно.
Такое не встречается в тех технических профессиях, в которых нет места спору на тему «я художник, я так вижу». К сожалению в программировании такие споры возникают сплошь и рядом.
Сконцентрируйтесь на проблемах вроде перепроектирования интерфейса класса или разбиения сложных функций. Подождите исправления этих проблем, прежде чем переходить к низкоуровневым вопросам, таким как именование переменных или понятность комментариев в коде.
Логично. Однако как быть, если новый сотрудник использует имена: k1, k2, ...,k23, soxranitsnimokekrana, rasparsitviragenie, scanirovatmatritzy и т.д.? Читать код с подобными именами будут затруднительно.
Мне кажется, если в компании дошли до признания практики ревью необходимой, то и практика найма должна уже быть на том уровне, который исключает попадание в команду таких кадров.
Тут просто включается в работу шаг по определению правил стиля для группы. Один раз автор переделает весь текст и сам поймет почему так лучше не делать.
Может, прозвучит слишком категорично, но если программист на ревью воспринимает замечания к коду как личные нападки, то у него какие-то проблемы в эмоциональном плане, а может и ещё что-то с психикой не совсем в порядке. То есть ворчливые дни бывают у всех, мало ли почему. Но если ревью кода регулярно напоминает работу сапёра, то может и не стоит с такими людьми работать? А то вместо ревью какая-то психотерапия получается.
У этого рассуждения есть обратная сторона, конечно. Если ревьюер самоутверждается за счёт ревью, то у него тоже видимо не всё хорошо, и может ну его и далее по тексту. Но если замечания осмысленные и по делу, а не лишь бы придраться, то можно и потерпеть шершавость слога и отсутствие реверансов, фигурально выражаясь. Грань, безусловно, тонка. Я лично для себя решил, пока до прямых оскорблений не доходит, на свой счёт не принимать. Хотя, если доходит, то это тоже повод скорее обеспокоиться психическим здоровьем коллеги, чем оскорбиться.
На правах мыслей вслух. Может, проблема в том, что программисты в основном молодые и, кхм, эмоционально незрелые? Может и не проблема это вовсе — повзрослеют и успокоятся?
У меня прямо наоборот. Когда категоричность и порывистость юности ушла, я стал с бóльшим вниманием прислушиваться к "чужим идеям". Мало того, что с возрастом стало легче даже при возникновении эмоциональной реакции сделать вдох-выдох и посмотреть на вопрос рационально. Так ещё с опытом пришло понимание, что совсем не обязательно "есть два мнения — моё и неправильное", во-первых моё мнение не обязательно правильное, во-вторых, почти всегда может быть больше двух точек зрения.
Никого не хочу обидеть, но, возможно, проблема таки не в гибкости мышления (которая, конечно, уходит), а в старом-добром "комплексе опыта"? То есть на замечания/предложения подсознательная реакция "да я так уже 20 лет пишу, а ты молокосос пороху не нюхал"?
На правах мыслей вслух, если себя убедить, что собственные мысли не есть истина в последней инстанции (достаточно вспомнить пару стыдных ошибок), и объем личного опыта не связан напрямую с "истинностью" (в общем тоже несложно) — таких проблем обычно не возникает.
Хотелось бы еще поинтересоваться опытом коллег по части немедленного проведения CR. С одной стороны всё правильно — блокируется работа коллеги, задача зависает перед отправкой на прод и т.д. Но с другой стороны переключение контекста может быть весьма дорогим и неприятным для разработчика, да и какая-нибудь рабочая встреча в самый неподходящий момент случится.
У себя в команде (8 разработчиков) мы ввели несколько правил:
- ревьювить может любой разработчик, и аппрувы всех равны, независимо от опыта и длительности пребывания в команде;
- для мерджа в мастер необходимо минимум два аппрува;
- ни один комментарий не должен остаться без ответа: коммита с исправлением или достигнутого вместе решения что можно не править
С одной стороны это хорошо: здоровая атмосфера и восприятие критики в команде, обмен опытом, в котором может участвовать каждый, чаще обнаруживаются баги и улучшения, CR проходят за приемлемое время. Но с другой стороны размывается ответственность, люди чаще думают «А ладно, посмотрю попозже, или еще кто-нибудь посмотрит», или ревьювят сквозь пальцы. И вот уже немедленной реакции нет, одну задаче прямо хорошо поревьювили, а другую «пойдёт» и т.д.
Кто как находит баланс в этой проблеме?
Разработчик 1 написал код / сделал дизайн
Разработчик 2 во время разработки смежного функционала, заметил код, WTF? Я бы сделал по другому, меняет код.
Разработчик 3 во время ревью добавил что-то от себя
Разработчик 1 через некоторое время возвращается к своему коду, WTF?
Нужно попытаться построить процесс так чтобы минимизировать их количество. Для этого разбить проект на слабосвязанные компоненты, модули, назначить ответственных за каждый модуль и пусть они комитят туда без каких-либо ревью — если компонента работает хорошо, не ломается, то разве не все-равно как она написана? Другое дело если сторонний человек вынужден сделать там какие-то изменения, то тогда ответственный за компоненту человек должен эти изменения ревьювить.
если компонента работает хорошо, не ломается, то разве не все-равно как она написана?
Не всё равно. Через пару лет этот человек уволиться (или его переедет самосвал), а код останется. И оставшимся нужно будет его поддерживать: добавлять новый функционал, исправлять старые ошибки, допиливать под новые платформы. Если код нечитабельный, то уйдёт уйма времени на его чтение и понимание.
В пределах команды (а желательно в пределах кампании) видение о том как правильно писать код должно быть одинаковым у всех разработчиков. Код написанный одним должен легко, как интересная книга, читаться другим. Code review в основном как раз для этого и нужен. Если вы видите непонятные имена, длинные функции, не влезающие в экран, запутанные алгоритмы, там где можно было бы обойтись простыми, отсутствие комментариев, там где они должны быть и т.д. — напишите об этом автору. Просматривайте чужой код так, будто вам завтра поручат исправлять в нём ошибки.
Для синхронизации видения есть Coding Style Guide. Есть такой документ на всю кампанию и часто бывают дополнения к нему используемые внутри команды. CSG вырабатывается в результате обсуждения и голосованием выбирается тот вариант, который устраивает большинство. Вам придётся следовать этому документу, даже если он вас не устраивает.
Как самый верхний уровень компании ведет дела так всё происходит и ниже.
В который раз возвращаюсь к этой статье, чтобы проверить рецензию.
Code review по-человечески (часть 1)