Comments 84
Я бы выделил две проблемы практики код-ревью:
Активно развивающийся код почти всегда нуждается в последующем рефакторинге и пишется сразу с учетом этого. Это нормально. Ревью не должно покрывать текущую итерацию элементами следующих. Всегда что-то можно написать лучше и что-то оптимизировать, но до этого код должен "настояться", как в головах разработчиков, так и в боевых условиях. А если не превращать ревью в игру "кто-кого переоптимизирует" — то достаточно и тестов.
- На то, чтобы разобраться в чужом коде, часто нужно потратить кучу сил, отвлечься от собственных проблем и встать лицом перед проблемой, которая еще недавно не была твоей. Если код несложный — вы должны уметь доверять коллегам (стараться не работать с теми, кому не доверяете), а в сложных случаях, вам придется решать уже решенную кем-то задачу, что не очень то эффективно. Ведущий разработчик должен периодически смотреть на код своих подопечных, и обсуждать его, но делать из этого формальную часть процесса я бы не стал.
А все остальное решается инструментальными средствами: линтерами, анализаторами и т. д.
От Badoo хочется видеть более интересные статьи. В частности, когда планируется внедрение рекомендации на основе глубокого обучения (фото понравившийся людей) или прогресс тормозит коммерция? Вы должны тут быть одними из первых. Или у вас уже это есть?
В моем случае масса проблем была как раз в том, чтобы установить «изначальные правила». В командах, которые уже думают что итак все знают и умеют. Отсюда и проблемы.
По поводу машинного обучения — можем, умеем, практикуем. Обязательно напишем что-нибудь интересное. Спасибо что читаете наш блог.
к слову, практически в любом более или менее успешном проекте код ревью очень затяжный процесс. Например, в сообществе разработчиков llvm: www.youtube.com/watch?v=jqAUxr-vDe0
Если позволите, я от себя добавлю ещё одну причину, зачем нужен код ревью: сокращение цикла обратной связи. При этом прошу заметить что это не единственный способ, и он не даёт существенных преимуществ, если используется в одиночку.
Код ревью позволяет находить уязвимости и недостатки кода до того как с кодом начинает работать другой разработчик (а именно это приводит к упомянутым вами незапланированным активности). Есть и другие практики, сокращающие цикл обратной связи — например парное программирование.
На мой взгляд, используя кодревью как инструмент валидации стилистики кода, команда рискует утонуть в болоте "у нас так принято".
Про обучение новичков я вообще молчу.
Ну а новички, которые на ревью песочат "местных" это какие-то мифические звери, имхо.
Если нет, то получается, что малоопытные сотрудники тоже что-то ревьювят. Тут возникают вопросы. Во-первых, малоопытный сотрудник плохо знаком с предметной областью и/или кодом. Поэтому у него уйдёт непропорционально много времени на ревью кода опытного сотрудника (например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код). Как это обойти? Во-вторых, как убедиться, что малоопытные сотрудники ревьювят качественно?
Если да, то получается, что чем опытнее сотрудник, тем больше на нём задач на ревью (миддл ревьюит за двумя жуниорами, синьор за тремя миддлами и избранные задачи за одним жуниором, лид за тремя синьорами, самые сложные задачи за двумя миддлами и двумя жуниорами...). Наиболее опытные сотрудники, которые могли бы решать задачи наиболее эффективно, будут заниматься ревью 8 часов в день 40 часов в неделю. А если самый крутой лид из любви к искусству всё-таки напишет несколько строк кода, то кто будет ревьюить их?
Тезис: ревью кода, написанного сотрудником, должен выполнять более опытный сотрудник. Верно ли это?
В моем случае было по-разному. Поэтому я и постарался описать ситуацию с разных сторон.
По моему скромному мнению, можно и так и эдак. Главное чтобы все понимали что делают и для чего это делается.
например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот кодЕсли джун правда разобрался и понял, как работает код лида — это прекрасно.
"Загадочным" он может быть не только потому, что написан запутанно, но и потому, что джун не знаком с некоторыми идиомами. Свежий пример:
@computed get authorName() { this.article.author.name }
Что значит кеширует? А если имя поменялось? А если данные ещё не загружены с сервера? Как сделать асинхронный запрос? Почему нигде нет расстановки индикаторов загрузки? Как они вообще сами по себе появляются? А где обработка ошибок? Кто вообще рисует этот красный восклицательный знак? Да это криптокод какой-то!
Ну как откуда. Заменяем this.article.author.name
на "Alice"
и вот уже никаких запросов, индикаторов загрузки, сообщений об ошибках и прочих "магических" вещах.
То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния. Сам код как раз становится простым и понятым: установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...», если не установлен, то выводим данные. Куда проще-то?
Так эвэйты, промисы или колбэки вместе с установкой флагов в коде присутствуют.
Нет.
То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния.
О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.
Куда проще-то?
Без всего вот этого вот:
установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...»
> О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.
Да что в таком коде непонятного может быть? Вот файл с бизнес-логикой, вот файл с инфраструктурной логикой (запросы к серверу), вот файл с логикой отображения. Последние ссылаются на первый. Несколько аннотаций типа `@observable`, `@computed`, `@action` и `@observer` знающему человеку скажут о характере динамического взаимодействия, а незнающему не помешают разобраться что происходит в коде. Единственный непонятный момент для него останется, это как вызовы action или изменение `@observable` приводят к вызову `@observer` и всё. Объяснения «считай, что под капотом генерируются события изменения и обсервер на них подписывается» достаточно чтобы сделать общую картину работы компонента полной, если значения английских слов ни на какие мысли не навели. Но из полной картины можно человеку доверять довольно значительные изменения в коде, если они не должны изменять существующие реактивные связи.
> Без всего вот этого вот:
Это ваши слова. Я обычно не делаю флаг загрузки, а проверяю состояние промиса. А запрос делать надо по любому, есл взаимодействие с сервером требуется. Да, бывают, варианты, когда в коде или конфиге явно запроса не найти, когда даже урл формируется динамически по, например, имени класса. Но вот это как раз «криптокод».
Да что в таком коде непонятного может быть?
Вы почему у меня всё это спрашиваете? :-) Я точно так же в недоумении. Ну, точнее, причина-то понятна. Человек не хочет разбираться в новых идиомах, поэтому ему проще обвинить код в "нечитаемости", что бы это ни значило.
А запрос делать надо по любому, если взаимодействие с сервером требуется.
Из компонента никаких запросов делать не надо — он сам сделается моделью по необходимости.
Я обычно не делаю флаг загрузки, а проверяю состояние промиса.
И проверять руками ничего не надо.
Под компонентами я не имел в виду исключительно визуальные компоненты, а полноценные реюзабельные модули, сочетающие и как-то инкапсулирующие всю необходимую логику. Простой пример — какой-то информер погоды или курсов.
Что значит «не надо»? Не, ну можно сделать хелпер (собственно в экосистеме React+MobX уже не один есть), который будет в заисимости от стейта промиса выводить один из трёх компонентов. Но в коде (пускай и конфигоподобном) нужно указывать, что выводить на каждый стейт и проверка будет осуществляться.
Откуда она знает надо показать надпись loading..., загрузка, прогресс-бар или спиннер?
А зачем вам неконсистентность в представлении индикаторов ожидания?
Уж молчу про обработку ошибок хотя бы по HTTP-статус коду.
Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?
Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.
>Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?
Вьюшке не надо, вьюшке надо знать какое по типу и содержанию сообщение выводить и, опционально, разные варианты продолжения работы показать. Решение какое сообщение показать будет принимать контроллер, получив данные из модели от http-адаптера по факту напрямую или примитивно смапленные типа 400 => 0, 404 => 1 и т. д., если уж мы говорим в терминах MVC. Ну или по типам исключений типа 400 ClientError 404 EntityNotFound и т. п…
Напомню, для меня компонент — это почти полноценное приложение, реализующее всю триаду MVC, а значит кроме вьюшки включает и модель (в которой будет http-адаптер), и контроллер, который будет принимать решение какие ошибки показывать и показывать ли их в ответ на http-ошибки (не важно, напрямую будет из модели они возвращаться, или простой маппинг будет на какие-то ошибки модели)
Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.
Так и воткните его по дефолту, незачем его в каждой вьюшке копипастить.
разные варианты продолжения работы показать
Можно пример?
Пример «Abort, Retry, Ignore?» — знакомо ?:)
Так и воткните его по дефолту, незачем его в каждой вьюшке копипастить.А если в разных местах разные индикаторы?
например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код
В 90% случаев это значит, что лида надо увольнять или переводить в менеджеры (если есть задатки или принцип Питера давит). В 10% — что должен был ревьюить миддл.
Если ведущий специалист пишет код, который никто не понимает, то куда же он ведет продукт? Мне в командах нужны лиды, которые могут сделать всякие ответственные штуки, например, самая ответственная — публичный API или общая схема взаимодействия блоков. Такой хороший код не сложен для понимания, а прост. Сложный код — не гордость опытного разработчика, а позор или признание какой-то слабости.
Для большинства бизнес-приложений это так. Есть исключения: одноразовый код, иногда high-concurrency, всякие интринсики или асм-вставки, код для экзотических железяк, всякая криптография и суровая математика. Ну да, есть иногда куски для объяснения которых нужен опыт выше джуна. Каждый такой кусок — повод задуматься "а не фигню ли мы делаем".
Важно помнить, что любой возврат кода на доработку не лучшим образом сказывается на качестве реализации.… Опыт показывает, что большинство таких ситуаций оборачивается багами именно в тех местах, где были правки по результатам ревью кода. Так работает человеческая психология.
это не психология, зачем вы так расплывчато описывает причину. это невнимательность или высокая самооценка разработчиков (может быть чтото еще, не знаю вашу команду). с вашими рассуждениями можно дойти до выводов, что вообще не нужно возвращать кода на доработку. или не стоит баги фиксить, потому что изза психологии разработчики добавят новых багов.
можно ли было баги, внедренные после доработки, обнаружить на второй итерации кодревью? может быть второе ревью было поверхностным? что если его усилить чек-листами, гайдлайнами, автоматизированными проверками
Основная причина частых багов из-за правок по результатам ревью (равно и как по результатам тестирования) — они, эти правки, не проходят полный цикл работы над задачей как по психологическим, так и по экономическим (тоже психологическим в общем-то) причинам.
Психологически правки не воспринимаются как отдельная задача, требующая столь же серьёзного отношения как и любые другие на всех этапах, как минимум: постановка задачи, планирование решения, собственно решение, тестирование, ревью и доставка. Часто некоторых этапов вообще нет или решение о необходимости такого этапа принимается ситуационно на основании субъективных метрик типа «много ли переписано строк после прошлого ревью/тестирования».
Ну и экономически часто неоправданно перезапускать полный цикл из-за, например, замеченной кем-то грамматической ошибки в комментарии. И тут начинаются субъективные метрики оправданности или неоправданности, может это что-то сломать или не может. Убираем субъективные, ставя доработонную задачу на полный цикл — прямые безусловные экономические потери, расходы. Добавляем — увеличиваем количество багов, но не безусловно, а вероятностно. При том, что их отстутствие не гарантированно и так, в худшем случае вероятности лишь заметно увеличиваются, а при перезапуске цикла лишь немного уменьшаются.
— у нас бывает код плохого качества
— ну это изза психологии, да и делать качественно оказывается дорого.
— аа, хорошо, оставим всё как есть
вы говорите, проблема психологического характера, а решения не предлагаете. то что вы делаете — это попытка объяснить происходящее и примириться с ним. вообще это один из способов решения проблем. пусть так.
я же на это смотрю так: вы можете пожертовать качеством, чтобы сделать быстрее/дешевле, пойти не полному процессу производства софта, а срезать углы. бородатая тактика управления содержанием проекта.
может быть те дефекты, которые появляются изза доработок, не влияют на бизнес, бизнес модель их демпфирует без последствий для продаж и клиентов. ну так и вообще всё замечательно в этом случае — не нужны тяжеловесные процессы, можно больше сфокусироваться на фичах и меньше на тест-активностях.
Но давайте начистоту: не поздновато ли? Не будет ли более правильным рассказать новичку об этих правилах до допуска его к коду? Ведь цена ошибки очень высока — редко новички сразу работают так, как вам нужно. А значит, задача будет возвращаться и возвращаться на доработку. А значит, продукт станет доступен пользователю позже, чем мог бы.
Это как waterfall против agile.
I'd say that the ultimate goals of code review are:
* decrease number of bugs since two pair of eyes usually better that one pair of eyes.
* improve code quality since 2 opinions usually better than one opinion.
Everything else are secondary things.
В статье на мой взгляд излишне много дартаньянства
Вы так говорите, как будто это что-то плохое :)
И команды, и проекты в разных компаниях бывают очень разные. И код ревью в них служит разным целям. И на часто повторяющийся вопрос «а не поздновато ли» значительная часть таких команд с чистой совестью ответит — нет.
Совершенно верно. Разумеется, я описываю в статье наш опыт. И разумеется, он может отличаться от других команд и других компаний. Я надеюсь что основной посыл «думай, потом делай, потом подумай еще раз: все ли правильно», так или иначе я донес.
На что только не идете в Badoo, чтобы оправдать свой велосипед) Воды налили!
Просто в процессе ревью не надо быть ботаном и помнить о цели проекта и качестве выполнения рассматриваемой задачи.
Просто в процессе ревью не надо быть ботаном и помнить о цели проекта и качестве выполнения рассматриваемой задачи.
Золотые слова. Как этого добиваться на постоянной основе, не поделитесь?
Смотря на новичков и их прогресс с точки зрения написания более понятного кода, я могу сказать, что ревью в самом деле работают на улучшение качества кода и на обучение сотрудников.
Единственная настоящая сложность это пограничные случаи, когда проверяющие блокируют изменения по незначительным с точки зрения изменяющего поводам. Т.е. когда эмоции начинают брать верх. Такие моменты наносят ощутимый урон команде с точки зрения духа товарищества, который, на мой взгляд, должен присутствовать. А вы что думаете по этому поводу?
Поделитесь, очень интересно.
С точки зрения «товарищества», то помимо собственно работы проводим социальные мероприятия, всякие встречи вне работы, во время работы на обеде, и тому подобное.
Единственная настоящая сложность это пограничные случаи, когда проверяющие блокируют изменения по незначительным с точки зрения изменяющего поводам
Скажу как человек, у которого есть такой ревьюер. Все время кажется, что для него это своеобразный спорт — найти то, до чего можно докопаться и не аппрувить ревью до тех пор, пока не будет так, как он хочет. Это очень сильно демотивирует. Начиная даже интересную задачу, всегда помню что скоро открывать реквест и он снова будет меня нервировать. Для примера, последний случай: для материализации коллекции из одного элемента (это 99% работы, самый максимум не более 10) он заставил меня поменять .ToList() на .ToArray() (это C#). Причина была: сэкономить память. Фактически экономии там несколько байт и у нас не хай-лоад.
Я согласен, что эта разница — попытка сэкономить на спичках и в формулировке «меньше памяти» может вызвать только улыбку, но с точки зрения чистоты кода и ограничения на изменение коллекции — я бы спросил с автора.
Опять же, зависит от обстоятельств: может быть это метод типа
public IReadonlyList<MyClass> Filter(IQueryable<MyClass> someData)
{
return someData.Where(x => x.IsValid).ToList();
}
тогда по сути все равно — что ToList, что ToArray.
Если оно введено для галочки и отлично эту галочку закрывает — то так тому и быть.
Если введено для того, чтобы никто не мог реализовать свои индивидуальные порывы по улучшению продакшена, и ты теперь знаешь, что твои «улучшения» дальше твоего рабочего места не уйдут — то быть такому ревью.
Чтобы перекрыть кислород расторопным менеджерам, умело подбивающих разработчиков пропихнуть в рамках других задачи их личные хотелки — почему нет?
Если в команде два с половиной человека, и ревью — это отличная возможность для них обмениваться знаниями и опытом в подходах к решению поставленных перед ними задач — что в этом плохого?
Плохим ревью становится только тогда, когда превращается в необходимый ритуал (понажимать на кнопки, написать дежурные фразы), и те, кто этим занимаются, не могут объяснить, зачем лично им оно нужно.
Да, для чего бы ни использовался какой-угодно инструмент, ретроспективно всегда можно указать, что это не самое лучшее применение. Но если это что-то живое (а не зацементированный ритуал), то оно будет со временем эволюционировать вместе с задачами, которые необходимо будет решать. И пока есть осознание, что если мы этот инструмент уберем, то старая проблема вернется, — мы все делаем правильно.
Грубо говоря, микроскопом и гвозди забивать, и возбудителей болезней искать в крови — это вряд ли правильно, за исключением каких-то особых случаев. При том не просто гвозди забивать было бы эффективней молотком или строительным пистолетом, а и качество поиска микробов страдает от того, что гвозди забивают.
Ревью, как часть процесса разработки и доставки, может решать множество задач в рамках конкретной компании/проекта/продукта. И это не значит, что это неправильно. Может для каких-то задач есть более эффективные в теории инструменты, но, например, стартовые, постоянные и(или) переменные издержки в них неприемлемы здесь и сейчас.
Начинать решать правильно мы проводим ревью иили неправильно, множно ли что-то улучшить надо с составления формального описания актуального состояния дел, с опроса участников и бизнес-заказчика, формализованного списка бизнес-целей, которых ревью должно помочь достигать, бизнес-задач, которые оно должно решать. Бизнес-требования к ревью и описание актуального бизнес-процесса ревью. А потом уже анализ их соотвествия и возможности оптимизации, и лишь возможно, но не обязательно эти оптимизации будут связаны с качественными изменениями, типа снятия с ревью задач, которые либо решаются уже параллельно, либо куда эффективней по всем метрикам решаются иными способами. В общем обычный подход к оптимизации бизнес-процессов.
Если вы, владельцы бизнес-процесса ревью и бизнес-процессов вообще в вашей компании, считают, что у ревью единственная бизнес-цель, которую оно эффективнее всего решает, а другие цели должны достигаться с помощью более эффективных для них инструментов, то, да, неправильно использовать ревью для других целей. Но если столь редкого единодушия нет, то неправильно говорить «вы делаете это неправильно». Навскидку, например, у компании может не быть возможности внедрять более эффективные инструменты обучения и коучинга (каждый раз когда думаю, что уже знаю, что такое коучинг, узнаю его новые значения) или внедрить автоматические инструменты проверки подавляющего большинства правил форматирования кода.
Неправильно, разве что, иметь процесс код ревью, но не иметь представления каким бизнес-целям он служит в теории, и что производит на практике. Нужны сформулированные цели и нужны метрики для проверки соотвествия им актуального процесса.
У меня и моих супервизоров всю жизнь в роли программиста ревью как-то был инструментом именно оценки решения задачи. Так как до попадания кода (я писал на js) в репу посредством гит хуков была введена цепочка eslint (в конфиге airbnb) → prettier → юнит тесты → commitlint, а при попытке открыть пул реквест в основную dev-ветку из feature-веток прогонялись е2е тесты в селениуме, то код, попадающий на ревью был уже сравнительно чист, отформатирован и нормально работал — и по моим ощущениям в основном проверялся на некое "качество исполнения" — отметались разнообразные велосипеды, грязные хаки, недокументированные / непонятные куски кода, и прочее подобное гадство (обычно с комментариями, но тут уже как повезёт — попадались и оригиналы с синдромом вахтёра, заворачивающие код с ужасающими пометками а-ля "ЧТО ЭТО ВООБЩЕ ТАКОЕ?" или просто "deny").
Не скажу что всё вышеописанное было вот прямо как-то очень круто или правильно, но удобно было явно. Нет, были конечно неприятные случая когда люди уставали смотреть на ошибки линтера и коммитили с --force, но потом народ перешёл на гитлаб и поставил pre-recieve-хук.
Как-то так.
решение описанное в коде на ревью, оптимальное
полностью протестировано
новички с ним ознакомились
свежие идеи пошарены в команде
демо новой фичи
и т.д и т.п…
Не слишком ли много для ревью? Процесс итак занимает кучу времени, скучен и однообразен.
С другой стороны Pair-programming, mob programming помогают справиться с практически всеми вопросами которые люди пытаются взвалить на код ревью. Тут вам и нахождение оптимального решения, шаринг, обучение, тест покрытие… В итоге ревью превращается в обычную формальность которая может быть автоматизирована.
Дополнительное перечисление их в заключении было бы полезно для понимания.
- реализуется ли полностью фича Y в данном код ревью X
- нет ли ошибок? проверены ли граничные условия? есть ли юнит-тесты? покрывают ли они задачу?
- не будет ли проседания по скорости, если одобрить X?
- если ответы «да», «да», «да», то про некоторые места спрашиваю почему здесь поменялось то-то и то-то (хотя я сам понял почему поменялось или думаю, что понял)
- если автор бодро и резво отвечает, значит он знает, что делает, и я апрувлю pull request
Да, и крупных переделок у нас не бывает после code review, только косметика, потому что все детали реализации всех крупных изменений обсуждаются заранее, а мелочь и так люди обычно делают правильно.
1. Проверка качества кода. «А зачем ты тут так странно делаешь, когда есть strange.do_it()?» У тебя тут сортировка вручную написана. Вот этот код будет работать, но его совершенно невозможно понять. Раздели в этом месте код не несколько независимых функций. Переменная tmp3 недостаточно информативная. Etc. Фактически, это эквивалент редакторской правки для статьи или книги.
2. Проверка на соответствие кода архитектуре проекта. Это может делаться только тем, кто эту архитектуру знает. Предпочительно — PTL, или самый старший из программистов. Он не спрашивает про сортировку пузырьком, он спрашивает «зачем ты тут используешь старый паттерн, который мы усиленно выпиливаем?», «не трогай объект Foo, т.к. он под сильным рефакторингом прямо сейчас, лучше используй вот это и это».
Совершенно разные задачи, совершенно разные комментарии. Первое может делать любой более-менее освоившийся программист, второе — только человек глубоко в проекте.
Code review: вы делаете это неправильно