среда, 18 мая 2016 г.

О рецензировании кода

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

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

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

clip_image002

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

Во-вторых, после стиля идет синдром, «я бы написал не так». Поскольку мозг автора находится в его собственной голове, а голова рецензента находится в пространстве где-то рядом, то мысли у них будут разные. И подходы к решению одной и той же задачи – тоже. Тут очень важно понять, насколько текущее решение качественно хуже/лучше предложенного, поскольку в противном случае переделывать существующее решение никто не будет, срач будет знатный, а результат – скорее отрицательный.

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

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

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

Что делать, чтобы ревью работало более эффективно?

Чтобы ревью работало нужно, чтобы все участники этого хотели и видели в ревью инструмент, а не самоцель. Вот несколько моментов/советов, которые должны помочь сделать этот процесс более эффективным.

Во-первых, для сложных задач лучше работать в паре, хотя бы в некоторые моменты времени или над ключевой частью задачи, например, над дизайном. В этом случае, получается «шаренное» владение кодом/решением, что упростит ревью с нескольких сторон: автор не будет воспринимать критику слишком близко к сердцу, поскольку ответственность будет лежать не на нем одном. Решение будет более качественным, поскольку над ним работало 2 пары глаз и целая пара мозгов. Ну и отважный второй пилот сможет пробекапить во время ревью и защитить идеи автора, давая понять, что они не случайны, а вполне продуманы.

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

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

В-четвертых, нужно установить формальные или неформальные рамки и правила игры. Например, разные тулы позволяют выражать свои действия с помощью типов комментариев и статуса. Если вы будете закрывавть комментарии путем установки его статуса в Won’t fix постоянно, то это будет выглядеть достаточно агрессивно. Аналогично, если инструмент рецензирования позволяет «ветировать» рецензию, то этого вообще может испортить отношения. В место этого лучше просто написать/позвонить/подойти к человеку и обсудить проблемы его решения лично. Вообще, устное общение – это хорошая инструмент разрешения спорных ситуаций, по сравнению с бесконечными комментариями.

Ну и в пятых, важно понимать, ради чего вся эта штука затеяна. Цель ревью – не показать превосходства артихектора над другими низшими формами кодеров, а сделать мир и команду лучше. Ревью помогает для распространения знаний, паттернов и подходов к решению разных задач. Для обмена опытом и предотвращения кривых решений, когда другие члены команды знают, какие неприятности последуют в случае текущего решения. Для того, чтобы решение было лучше, система стабильнее, трава зеленее, а девушки …, ну вы поняли, красивше.

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

13 комментариев:

  1. Хорошо. Но нового пожалуй ничего. Лучше расскажи, а как это работает в MS? :)

    ОтветитьУдалить
  2. "придет большой бородатый артихектор и отархитекторит нерадивому кодеру все, что можно" - Это сказано супер, ЛОЛ :-)

    ОтветитьУдалить
  3. В статье Bertrand Meyer под названием Design and code reviews in the age of the Internet очень неплохо описано, как они проводят код-ревью, с чего начали, и чем закончили.

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

      Удалить
  4. Есть еще шестая проблема, которую, в принципе, можно проследить в первых двух. Но все-таки лучше ее выделять отдельно.
    Код-ревью хорошо работает на простых задачках, с более-менее понятной формализацией. Например, написал один разработчик свой вариант glob, ревьювер сразу может понять, какие особенность есть у этой задачи, на что следует обращать внимание, какие ошибки совершают чаще всего.

    Если же задача сложная и автору решения пришлось неслабо потрудиться, чтобы погрузиться в ее специфику, то для хорошего ревью аналогичные усилия нужно приложить и ревьюверу. Если этого не сделать, то будут лишь поверхностные замечания по стилю оформления кода и споры вокруг возврата объектов или же использования in/out аргументов. Но до реальных проблем (например, напрочь забытая обработка какого-то неочевидного граничного условия) дело не дойдет. И получается, что на сложных задачах усилия и стоимость разработки, фактически, удваивается. На что, понятное дело, готовы идти далеко не все.

    ОтветитьУдалить
    Ответы
    1. Более того, в форме ревью рецензент просто не будет в состоянии получить требуемого контекста, что сведет все его попытки понять проблему достаточно глубоко, практически на нет.

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

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

    ОтветитьУдалить
  6. Присоеденяюсь к просьбе рассказать как это в MS.

    ОтветитьУдалить
  7. “Ну и в пятых, важно понимать, ради чего вся эта штука затеяна. Цель ревью – не показать превосходства артихектора над другими низшими формами кодеров, а сделать мир и команду лучше.”
    Цель ревью является расширение ответственности за участок кода. Идеальный вариант, когда каждый член команды просмотрит каждую строчку проекта. И в этом случае, в последующем, за возникшие проблемы ответственность будут нести все, а не только те, кто указан в “Blame”. А насчёт реализаций и стилей не стоит особо заморачиваться, самое главное, что если ты увидел проблему и промолчал, то уже не отвертишься.

    ОтветитьУдалить