Comments 84
Я бы выделил две проблемы практики код-ревью:
Активно развивающийся код почти всегда нуждается в последующем рефакторинге и пишется сразу с учетом этого. Это нормально. Ревью не должно покрывать текущую итерацию элементами следующих. Всегда что-то можно написать лучше и что-то оптимизировать, но до этого код должен "настояться", как в головах разработчиков, так и в боевых условиях. А если не превращать ревью в игру "кто-кого переоптимизирует" — то достаточно и тестов.
- На то, чтобы разобраться в чужом коде, часто нужно потратить кучу сил, отвлечься от собственных проблем и встать лицом перед проблемой, которая еще недавно не была твоей. Если код несложный — вы должны уметь доверять коллегам (стараться не работать с теми, кому не доверяете), а в сложных случаях, вам придется решать уже решенную кем-то задачу, что не очень то эффективно. Ведущий разработчик должен периодически смотреть на код своих подопечных, и обсуждать его, но делать из этого формальную часть процесса я бы не стал.
А все остальное решается инструментальными средствами: линтерами, анализаторами и т. д.
Очень много букв про некую мнимую вещь. Есть правила изначальные и автоматизация всего что можно. Рефакторинг кода будет всегда, как бы вы не старались его избежать
От Badoo хочется видеть более интересные статьи. В частности, когда планируется внедрение рекомендации на основе глубокого обучения (фото понравившийся людей) или прогресс тормозит коммерция? Вы должны тут быть одними из первых. Или у вас уже это есть?
От Badoo хочется видеть более интересные статьи. В частности, когда планируется внедрение рекомендации на основе глубокого обучения (фото понравившийся людей) или прогресс тормозит коммерция? Вы должны тут быть одними из первых. Или у вас уже это есть?
К сожалению, описанные в статье проблемы — не мнимые. Это личная практика. Как бы это странно или нереально ни выглядело.
В моем случае масса проблем была как раз в том, чтобы установить «изначальные правила». В командах, которые уже думают что итак все знают и умеют. Отсюда и проблемы.
По поводу машинного обучения — можем, умеем, практикуем. Обязательно напишем что-нибудь интересное. Спасибо что читаете наш блог.
В моем случае масса проблем была как раз в том, чтобы установить «изначальные правила». В командах, которые уже думают что итак все знают и умеют. Отсюда и проблемы.
По поводу машинного обучения — можем, умеем, практикуем. Обязательно напишем что-нибудь интересное. Спасибо что читаете наш блог.
мнимая вещь? если разработчикам не важна стабильность и поддерживаемость решения то да, можно сказать мнимая. Но если вы серьезно относитесь к своей работе — то нет. Линтеры вас не особо спасут. Например, бывало что люди изобретают свой vtable вместо использования виртуальных функций. Как такое отловит линтер?
к слову, практически в любом более или менее успешном проекте код ревью очень затяжный процесс. Например, в сообществе разработчиков llvm: www.youtube.com/watch?v=jqAUxr-vDe0
к слову, практически в любом более или менее успешном проекте код ревью очень затяжный процесс. Например, в сообществе разработчиков llvm: www.youtube.com/watch?v=jqAUxr-vDe0
Можно делать много всего, но через полгода это все будет уже совершенно другим. Наверняка такие крупные компании как Badoo это делают. У них есть на это ресурсы, и возможно это реально там нужно. Но для мелких коллективов и команд это не есть самое главное. Я лишь просто хотел бы что бы они написали интересные вещи по разработки. То, что могли бы посмотреть более мелкие команды и разработчики. А код ревью, какой у нас офис и как мы все замечательно проводим время это наверняка не самое интересное что они могут нам рассказать.
Если позволите, я от себя добавлю ещё одну причину, зачем нужен код ревью: сокращение цикла обратной связи. При этом прошу заметить что это не единственный способ, и он не даёт существенных преимуществ, если используется в одиночку.
Код ревью позволяет находить уязвимости и недостатки кода до того как с кодом начинает работать другой разработчик (а именно это приводит к упомянутым вами незапланированным активности). Есть и другие практики, сокращающие цикл обратной связи — например парное программирование.
На мой взгляд, используя кодревью как инструмент валидации стилистики кода, команда рискует утонуть в болоте "у нас так принято".
Про обучение новичков я вообще молчу.
Ну а новички, которые на ревью песочат "местных" это какие-то мифические звери, имхо.
Тезис: ревью кода, написанного сотрудником, должен выполнять более опытный сотрудник. Верно ли это?
Если нет, то получается, что малоопытные сотрудники тоже что-то ревьювят. Тут возникают вопросы. Во-первых, малоопытный сотрудник плохо знаком с предметной областью и/или кодом. Поэтому у него уйдёт непропорционально много времени на ревью кода опытного сотрудника (например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код). Как это обойти? Во-вторых, как убедиться, что малоопытные сотрудники ревьювят качественно?
Если да, то получается, что чем опытнее сотрудник, тем больше на нём задач на ревью (миддл ревьюит за двумя жуниорами, синьор за тремя миддлами и избранные задачи за одним жуниором, лид за тремя синьорами, самые сложные задачи за двумя миддлами и двумя жуниорами...). Наиболее опытные сотрудники, которые могли бы решать задачи наиболее эффективно, будут заниматься ревью 8 часов в день 40 часов в неделю. А если самый крутой лид из любви к искусству всё-таки напишет несколько строк кода, то кто будет ревьюить их?
Если нет, то получается, что малоопытные сотрудники тоже что-то ревьювят. Тут возникают вопросы. Во-первых, малоопытный сотрудник плохо знаком с предметной областью и/или кодом. Поэтому у него уйдёт непропорционально много времени на ревью кода опытного сотрудника (например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код). Как это обойти? Во-вторых, как убедиться, что малоопытные сотрудники ревьювят качественно?
Если да, то получается, что чем опытнее сотрудник, тем больше на нём задач на ревью (миддл ревьюит за двумя жуниорами, синьор за тремя миддлами и избранные задачи за одним жуниором, лид за тремя синьорами, самые сложные задачи за двумя миддлами и двумя жуниорами...). Наиболее опытные сотрудники, которые могли бы решать задачи наиболее эффективно, будут заниматься ревью 8 часов в день 40 часов в неделю. А если самый крутой лид из любви к искусству всё-таки напишет несколько строк кода, то кто будет ревьюить их?
Тезис: ревью кода, написанного сотрудником, должен выполнять более опытный сотрудник. Верно ли это?
В моем случае было по-разному. Поэтому я и постарался описать ситуацию с разных сторон.
По моему скромному мнению, можно и так и эдак. Главное чтобы все понимали что делают и для чего это делается.
например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот кодЕсли джун правда разобрался и понял, как работает код лида — это прекрасно.
Да, но в процессах, когда задача считается не решённой, пока не завершён ревью — это будет означать, что задача решалась неделями (и клиент ждал неделями), хотя лид всё сделал за полдня.
Если джун не понял как работает код лида, то лид некомпетентный. Чем выше квалификация, тем менее загадочный код должен писать программист.
Главное, чтобы джун понял, как писать такой же код
Про собственно код я согласен. Но, чтобы оценить код, надо понять постановку задачи. Для лида / синьора задача вполне может ставиться в бизнес-терминах, жуниору ещё предстоит научиться понимать все эти страшные слова (к написанию кода отношения не имеющие).
"Загадочным" он может быть не только потому, что написан запутанно, но и потому, что джун не знаком с некоторыми идиомами. Свежий пример:
@computed get authorName() { this.article.author.name }Что значит кеширует? А если имя поменялось? А если данные ещё не загружены с сервера? Как сделать асинхронный запрос? Почему нигде нет расстановки индикаторов загрузки? Как они вообще сами по себе появляются? А где обработка ошибок? Кто вообще рисует этот красный восклицательный знак? Да это криптокод какой-то!
Откуда из этого кода (моделька на MobX, да?) вообще возьмутся идеи о кешировании, серверах, запросах, индикаторах, ошибках и восклицательных знаках? Можно не понимать, что значит декоратор @computed, но откуда такой поток идей от вида простого геттера?
Ну как откуда. Заменяем this.article.author.name на "Alice" и вот уже никаких запросов, индикаторов загрузки, сообщений об ошибках и прочих "магических" вещах.
Мы говорили про чтение кода, а не про исследование его путём наблюдения за сайд-эффектами при модификации.
Вот смотрит человек в код компонента, видит лишь пяток подобных геттеров, и недоумевает как же это всё работает без асик-эвэйтов, установки флага загрузки данных, навешивания обработчиков событий и прочих привычных вещей. «Да это криптокод какой-то!» — реальная цитата.
Так эвэйты, промисы или колбэки вместе с установкой флагов в коде присутствуют. И рендеринг по этим флагам тоже понятен и очевиден, если понятно как вообще он происходит. Единственное, что часто непонятно разработчику, столкнувшемуся с реактивным подходом в первый раз — это механизмы перерендеринга, как вызов обычного сеттера ил иного мутирующего метода, вызывает перерендеринг. И самая частая конкретная проблема: вызывает этот сеттер, а перерендеринга не проиходит. Причина обычно в том, что не знает нюансов мониторинга слежения за свойствами, например, что мониторится изменение значения ссылки на объект, а не изменение его свойств (мониторить можно и свойства, но вот в конкретном куске кода может быть неочевидно что конкретно мониторится).
То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния. Сам код как раз становится простым и понятым: установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...», если не установлен, то выводим данные. Куда проще-то?
То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния. Сам код как раз становится простым и понятым: установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...», если не установлен, то выводим данные. Куда проще-то?
Так эвэйты, промисы или колбэки вместе с установкой флагов в коде присутствуют.
Нет.
То есть «криптокод» — это не сам код, а механизм реакций на изменения состояния.
О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.
Куда проще-то?
Без всего вот этого вот:
установили флаг загрузки и начали запрос, по окончанию запроса записали данные и сбросили этот флаг. А в функции рендинга проверяем флаг — если установлен пишем «loading...»
Если не присутствуют, то они кодом компонента не являются и для изучения работы компонента не нужны. MobX не предоставлять функциональности запросов к серверу, он лишь реактивное хранилище данных.
> О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.
Да что в таком коде непонятного может быть? Вот файл с бизнес-логикой, вот файл с инфраструктурной логикой (запросы к серверу), вот файл с логикой отображения. Последние ссылаются на первый. Несколько аннотаций типа `@observable`, `@computed`, `@action` и `@observer` знающему человеку скажут о характере динамического взаимодействия, а незнающему не помешают разобраться что происходит в коде. Единственный непонятный момент для него останется, это как вызовы action или изменение `@observable` приводят к вызову `@observer` и всё. Объяснения «считай, что под капотом генерируются события изменения и обсервер на них подписывается» достаточно чтобы сделать общую картину работы компонента полной, если значения английских слов ни на какие мысли не навели. Но из полной картины можно человеку доверять довольно значительные изменения в коде, если они не должны изменять существующие реактивные связи.
> Без всего вот этого вот:
Это ваши слова. Я обычно не делаю флаг загрузки, а проверяю состояние промиса. А запрос делать надо по любому, есл взаимодействие с сервером требуется. Да, бывают, варианты, когда в коде или конфиге явно запроса не найти, когда даже урл формируется динамически по, например, имени класса. Но вот это как раз «криптокод».
> О чём я, собственно, и говорил — незнание идиом делает даже предельно простой код непонятным. Но это не проблема кода, это обычный пробел в знаниях, который надо заполнять.
Да что в таком коде непонятного может быть? Вот файл с бизнес-логикой, вот файл с инфраструктурной логикой (запросы к серверу), вот файл с логикой отображения. Последние ссылаются на первый. Несколько аннотаций типа `@observable`, `@computed`, `@action` и `@observer` знающему человеку скажут о характере динамического взаимодействия, а незнающему не помешают разобраться что происходит в коде. Единственный непонятный момент для него останется, это как вызовы action или изменение `@observable` приводят к вызову `@observer` и всё. Объяснения «считай, что под капотом генерируются события изменения и обсервер на них подписывается» достаточно чтобы сделать общую картину работы компонента полной, если значения английских слов ни на какие мысли не навели. Но из полной картины можно человеку доверять довольно значительные изменения в коде, если они не должны изменять существующие реактивные связи.
> Без всего вот этого вот:
Это ваши слова. Я обычно не делаю флаг загрузки, а проверяю состояние промиса. А запрос делать надо по любому, есл взаимодействие с сервером требуется. Да, бывают, варианты, когда в коде или конфиге явно запроса не найти, когда даже урл формируется динамически по, например, имени класса. Но вот это как раз «криптокод».
Да что в таком коде непонятного может быть?
Вы почему у меня всё это спрашиваете? :-) Я точно так же в недоумении. Ну, точнее, причина-то понятна. Человек не хочет разбираться в новых идиомах, поэтому ему проще обвинить код в "нечитаемости", что бы это ни значило.
А запрос делать надо по любому, если взаимодействие с сервером требуется.
Из компонента никаких запросов делать не надо — он сам сделается моделью по необходимости.
Я обычно не делаю флаг загрузки, а проверяю состояние промиса.
И проверять руками ничего не надо.
Да не надо ему разбираться в этих идиомах если его задача, например, всегда выводить имя с заглавной буквы, как бы оно не хранилось. Или адаптировать парсинг тела ответа к новому формату серверного API. Разбираться надо только если ему нужно изменять, дополнять или удалять реактивные связи. Неочевидна для обычного программиста динамика взаимодействия классов, но не «статическая» логика получения данных или их отображения.
Под компонентами я не имел в виду исключительно визуальные компоненты, а полноценные реюзабельные модули, сочетающие и как-то инкапсулирующие всю необходимую логику. Простой пример — какой-то информер погоды или курсов.
Что значит «не надо»? Не, ну можно сделать хелпер (собственно в экосистеме React+MobX уже не один есть), который будет в заисимости от стейта промиса выводить один из трёх компонентов. Но в коде (пускай и конфигоподобном) нужно указывать, что выводить на каждый стейт и проверка будет осуществляться.
Под компонентами я не имел в виду исключительно визуальные компоненты, а полноценные реюзабельные модули, сочетающие и как-то инкапсулирующие всю необходимую логику. Простой пример — какой-то информер погоды или курсов.
Что значит «не надо»? Не, ну можно сделать хелпер (собственно в экосистеме React+MobX уже не один есть), который будет в заисимости от стейта промиса выводить один из трёх компонентов. Но в коде (пускай и конфигоподобном) нужно указывать, что выводить на каждый стейт и проверка будет осуществляться.
Да не надо ничего указывать. «Обёртка» сама вставит по необходимости. Там же типовой код.
Какой типовой? Откуда она знает надо показать надпись loading..., загрузка, прогресс-бар или спиннер? И это простые варианты только для загрузки. Уж молчу про обработку ошибок хотя бы по HTTP-статус коду.
Откуда она знает надо показать надпись loading..., загрузка, прогресс-бар или спиннер?
А зачем вам неконсистентность в представлении индикаторов ожидания?
Уж молчу про обработку ошибок хотя бы по HTTP-статус коду.
Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?
> А зачем вам неконсистентность в представлении индикаторов ожидания?
Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.
>Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?
Вьюшке не надо, вьюшке надо знать какое по типу и содержанию сообщение выводить и, опционально, разные варианты продолжения работы показать. Решение какое сообщение показать будет принимать контроллер, получив данные из модели от http-адаптера по факту напрямую или примитивно смапленные типа 400 => 0, 404 => 1 и т. д., если уж мы говорим в терминах MVC. Ну или по типам исключений типа 400 ClientError 404 EntityNotFound и т. п…
Напомню, для меня компонент — это почти полноценное приложение, реализующее всю триаду MVC, а значит кроме вьюшки включает и модель (в которой будет http-адаптер), и контроллер, который будет принимать решение какие ошибки показывать и показывать ли их в ответ на http-ошибки (не важно, напрямую будет из модели они возвращаться, или простой маппинг будет на какие-то ошибки модели)
Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.
>Этим занимается http-адаптер. Зачем вьюшке знать про http-коды?
Вьюшке не надо, вьюшке надо знать какое по типу и содержанию сообщение выводить и, опционально, разные варианты продолжения работы показать. Решение какое сообщение показать будет принимать контроллер, получив данные из модели от http-адаптера по факту напрямую или примитивно смапленные типа 400 => 0, 404 => 1 и т. д., если уж мы говорим в терминах MVC. Ну или по типам исключений типа 400 ClientError 404 EntityNotFound и т. п…
Напомню, для меня компонент — это почти полноценное приложение, реализующее всю триаду MVC, а значит кроме вьюшки включает и модель (в которой будет http-адаптер), и контроллер, который будет принимать решение какие ошибки показывать и показывать ли их в ответ на http-ошибки (не важно, напрямую будет из модели они возвращаться, или простой маппинг будет на какие-то ошибки модели)
Он дизайнер, он так видит. И даже без неконсистенции очень хочется тот индикатор, который дизайнер нарисовал, а не какой-то системный дефолтный.
Так и воткните его по дефолту, незачем его в каждой вьюшке копипастить.
разные варианты продолжения работы показать
Можно пример?
Не суть как его воткнуть, нужно воткнуть его показ как реакцию вьюшки на ожидание промиса, ну или как реакцию на установку флага. Всё равно промис или флаг будет частью стейта компонента, всё равно компонент долже реагировать на изменение стейта и решать что показать в конкретном состоянии. Можно вынести это в какие-то базовые классы с дефолтными реализациями ожидания и ошибок, то это лишь простое переиспользование кода, не более.
Пример «Abort, Retry, Ignore?» — знакомо ?:)
Пример «Abort, Retry, Ignore?» — знакомо ?:)
Так и воткните его по дефолту, незачем его в каждой вьюшке копипастить.А если в разных местах разные индикаторы?
код может быть непонятен читающему в силу нескольких факторов, и квалификация написавшего всего лишь один из них. Абстрактный джун может быть недостаточно знаком с используемыми ЯП/фреймворками/библиотеками/паттернами, терминологией и матчастью предметной области и т.д.
например, лид написал код за полдня, а жуниору понадобится несколько недель, чтобы понять, какую задачу решает этот код
В 90% случаев это значит, что лида надо увольнять или переводить в менеджеры (если есть задатки или принцип Питера давит). В 10% — что должен был ревьюить миддл.
Если ведущий специалист пишет код, который никто не понимает, то куда же он ведет продукт? Мне в командах нужны лиды, которые могут сделать всякие ответственные штуки, например, самая ответственная — публичный API или общая схема взаимодействия блоков. Такой хороший код не сложен для понимания, а прост. Сложный код — не гордость опытного разработчика, а позор или признание какой-то слабости.
Для большинства бизнес-приложений это так. Есть исключения: одноразовый код, иногда high-concurrency, всякие интринсики или асм-вставки, код для экзотических железяк, всякая криптография и суровая математика. Ну да, есть иногда куски для объяснения которых нужен опыт выше джуна. Каждый такой кусок — повод задуматься "а не фигню ли мы делаем".
А я думал наоборот, чем опытнее разработчик, тем проще код.
Важно помнить, что любой возврат кода на доработку не лучшим образом сказывается на качестве реализации.… Опыт показывает, что большинство таких ситуаций оборачивается багами именно в тех местах, где были правки по результатам ревью кода. Так работает человеческая психология.
это не психология, зачем вы так расплывчато описывает причину. это невнимательность или высокая самооценка разработчиков (может быть чтото еще, не знаю вашу команду). с вашими рассуждениями можно дойти до выводов, что вообще не нужно возвращать кода на доработку. или не стоит баги фиксить, потому что изза психологии разработчики добавят новых багов.
можно ли было баги, внедренные после доработки, обнаружить на второй итерации кодревью? может быть второе ревью было поверхностным? что если его усилить чек-листами, гайдлайнами, автоматизированными проверками
Скорее всего, если в команде часты случаи появления багов из-за правок по результатам ревью (равно как и по результатам тестирования, как самим разработчиком, юнит-тестами и ручным, так и штатными QA) из-за психологических особенностей процессов «полирования» по сравнению с основной задачей, то и повторное ревью имеет те же особенности. И они именно психологические. Если можно усилить процесс повторного ревью автоматизированными проверками или ещё чем, то усиливать надо ревью вообще.
Основная причина частых багов из-за правок по результатам ревью (равно и как по результатам тестирования) — они, эти правки, не проходят полный цикл работы над задачей как по психологическим, так и по экономическим (тоже психологическим в общем-то) причинам.
Психологически правки не воспринимаются как отдельная задача, требующая столь же серьёзного отношения как и любые другие на всех этапах, как минимум: постановка задачи, планирование решения, собственно решение, тестирование, ревью и доставка. Часто некоторых этапов вообще нет или решение о необходимости такого этапа принимается ситуационно на основании субъективных метрик типа «много ли переписано строк после прошлого ревью/тестирования».
Ну и экономически часто неоправданно перезапускать полный цикл из-за, например, замеченной кем-то грамматической ошибки в комментарии. И тут начинаются субъективные метрики оправданности или неоправданности, может это что-то сломать или не может. Убираем субъективные, ставя доработонную задачу на полный цикл — прямые безусловные экономические потери, расходы. Добавляем — увеличиваем количество багов, но не безусловно, а вероятностно. При том, что их отстутствие не гарантированно и так, в худшем случае вероятности лишь заметно увеличиваются, а при перезапуске цикла лишь немного уменьшаются.
Основная причина частых багов из-за правок по результатам ревью (равно и как по результатам тестирования) — они, эти правки, не проходят полный цикл работы над задачей как по психологическим, так и по экономическим (тоже психологическим в общем-то) причинам.
Психологически правки не воспринимаются как отдельная задача, требующая столь же серьёзного отношения как и любые другие на всех этапах, как минимум: постановка задачи, планирование решения, собственно решение, тестирование, ревью и доставка. Часто некоторых этапов вообще нет или решение о необходимости такого этапа принимается ситуационно на основании субъективных метрик типа «много ли переписано строк после прошлого ревью/тестирования».
Ну и экономически часто неоправданно перезапускать полный цикл из-за, например, замеченной кем-то грамматической ошибки в комментарии. И тут начинаются субъективные метрики оправданности или неоправданности, может это что-то сломать или не может. Убираем субъективные, ставя доработонную задачу на полный цикл — прямые безусловные экономические потери, расходы. Добавляем — увеличиваем количество багов, но не безусловно, а вероятностно. При том, что их отстутствие не гарантированно и так, в худшем случае вероятности лишь заметно увеличиваются, а при перезапуске цикла лишь немного уменьшаются.
читая Вас, VolCh, я слышу вот что:
— у нас бывает код плохого качества
— ну это изза психологии, да и делать качественно оказывается дорого.
— аа, хорошо, оставим всё как есть
вы говорите, проблема психологического характера, а решения не предлагаете. то что вы делаете — это попытка объяснить происходящее и примириться с ним. вообще это один из способов решения проблем. пусть так.
я же на это смотрю так: вы можете пожертовать качеством, чтобы сделать быстрее/дешевле, пойти не полному процессу производства софта, а срезать углы. бородатая тактика управления содержанием проекта.
может быть те дефекты, которые появляются изза доработок, не влияют на бизнес, бизнес модель их демпфирует без последствий для продаж и клиентов. ну так и вообще всё замечательно в этом случае — не нужны тяжеловесные процессы, можно больше сфокусироваться на фичах и меньше на тест-активностях.
— у нас бывает код плохого качества
— ну это изза психологии, да и делать качественно оказывается дорого.
— аа, хорошо, оставим всё как есть
вы говорите, проблема психологического характера, а решения не предлагаете. то что вы делаете — это попытка объяснить происходящее и примириться с ним. вообще это один из способов решения проблем. пусть так.
я же на это смотрю так: вы можете пожертовать качеством, чтобы сделать быстрее/дешевле, пойти не полному процессу производства софта, а срезать углы. бородатая тактика управления содержанием проекта.
может быть те дефекты, которые появляются изза доработок, не влияют на бизнес, бизнес модель их демпфирует без последствий для продаж и клиентов. ну так и вообще всё замечательно в этом случае — не нужны тяжеловесные процессы, можно больше сфокусироваться на фичах и меньше на тест-активностях.
Я именно что объясняю свое видение причин появления подобных проблем, но не предалагаю ни мириться, ни бороться. Хотя и предлагаю один из способов борьбы, если считаете, что бороться надо и у вас проблемы именного того характера, которые этот способ устраняет. Мало кто со стороны сможет оценить целесообразность борьбы и, тем более, эффективность конкретного способа в конкретных условиях.
Но давайте начистоту: не поздновато ли? Не будет ли более правильным рассказать новичку об этих правилах до допуска его к коду? Ведь цена ошибки очень высока — редко новички сразу работают так, как вам нужно. А значит, задача будет возвращаться и возвращаться на доработку. А значит, продукт станет доступен пользователю позже, чем мог бы.
Это как 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#). Причина была: сэкономить память. Фактически экономии там несколько байт и у нас не хай-лоад.
К сожалению, слишком часто ревью кода из инструмента помощи автору кода сделать хорошо превращается в инструмент реализации комплекса вахтёра.
С другой стороны, а чем-то был обусловлен выбор именно .ToList()? Я, например, если не вижу обоснований использованного варианта реализации, но даже без глубокого погружения в задачу вижу другие альтернативы, более экономные по ресурсам, то прошу обосновать. Если обоснование вида «да какая разница по списку итерироваться или по массиву?», то настаиваю на более экономном варианте, если его читаемость не ниже.
Однообразием. И здравомыслием. Экономить пару байт на этой операции нет никакого смысла, рядом тяжелые апи-вызовы бегают туда-сюда. Как раз для вас в начале статьи цитата: «Самое сложное для меня в процессе ревью — пойти на компромисс и пропустить готовую и корректно работающую задачу дальше, даже если я знаю другой способ её решения и даже если мой способ мне нравится больше».
Однообразие — важный и объективный момент. Это реальное обоснование, если в «146%» случаев используются списки, а не массивы. Здравомыслие — субъективно.
Ох, не понимаю я этого стремления ввернуть ToList. Скажем, в древние времена, когда ToArray пользовался древним ArrayBuffer'ом и вызывал аллокации чуть ли не на каждый элемент, в отличие от List<>, который это делал логарифм раз, я еще понимаю. Но в нашем современном мире ToArray делает то же самое, что ToList, только действительно тратит меньше памяти и вдобавок ограничивает в действиях с коллекцией — я бы тоже на ревью спросил «а почему тут используется лист? ты собираешься добавлять туда еще что-то или убирать?» Потому что как правило эти ToList'ы просто нужны для неотложенного итерирования.
Я согласен, что эта разница — попытка сэкономить на спичках и в формулировке «меньше памяти» может вызвать только улыбку, но с точки зрения чистоты кода и ограничения на изменение коллекции — я бы спросил с автора.
Опять же, зависит от обстоятельств: может быть это метод типа
тогда по сути все равно — что ToList, что ToArray.
Я согласен, что эта разница — попытка сэкономить на спичках и в формулировке «меньше памяти» может вызвать только улыбку, но с точки зрения чистоты кода и ограничения на изменение коллекции — я бы спросил с автора.
Опять же, зависит от обстоятельств: может быть это метод типа
public IReadonlyList<MyClass> Filter(IQueryable<MyClass> someData)
{
return someData.Where(x => x.IsValid).ToList();
}тогда по сути все равно — что ToList, что ToArray.
Ревью кода настолько обобщенная вещь, что его единственная цель — это решать ту задачу, ради которой оно было введено.
Если оно введено для галочки и отлично эту галочку закрывает — то так тому и быть.
Если введено для того, чтобы никто не мог реализовать свои индивидуальные порывы по улучшению продакшена, и ты теперь знаешь, что твои «улучшения» дальше твоего рабочего места не уйдут — то быть такому ревью.
Чтобы перекрыть кислород расторопным менеджерам, умело подбивающих разработчиков пропихнуть в рамках других задачи их личные хотелки — почему нет?
Если в команде два с половиной человека, и ревью — это отличная возможность для них обмениваться знаниями и опытом в подходах к решению поставленных перед ними задач — что в этом плохого?
Плохим ревью становится только тогда, когда превращается в необходимый ритуал (понажимать на кнопки, написать дежурные фразы), и те, кто этим занимаются, не могут объяснить, зачем лично им оно нужно.
Да, для чего бы ни использовался какой-угодно инструмент, ретроспективно всегда можно указать, что это не самое лучшее применение. Но если это что-то живое (а не зацементированный ритуал), то оно будет со временем эволюционировать вместе с задачами, которые необходимо будет решать. И пока есть осознание, что если мы этот инструмент уберем, то старая проблема вернется, — мы все делаем правильно.
Если оно введено для галочки и отлично эту галочку закрывает — то так тому и быть.
Если введено для того, чтобы никто не мог реализовать свои индивидуальные порывы по улучшению продакшена, и ты теперь знаешь, что твои «улучшения» дальше твоего рабочего места не уйдут — то быть такому ревью.
Чтобы перекрыть кислород расторопным менеджерам, умело подбивающих разработчиков пропихнуть в рамках других задачи их личные хотелки — почему нет?
Если в команде два с половиной человека, и ревью — это отличная возможность для них обмениваться знаниями и опытом в подходах к решению поставленных перед ними задач — что в этом плохого?
Плохим ревью становится только тогда, когда превращается в необходимый ритуал (понажимать на кнопки, написать дежурные фразы), и те, кто этим занимаются, не могут объяснить, зачем лично им оно нужно.
Да, для чего бы ни использовался какой-угодно инструмент, ретроспективно всегда можно указать, что это не самое лучшее применение. Но если это что-то живое (а не зацементированный ритуал), то оно будет со временем эволюционировать вместе с задачами, которые необходимо будет решать. И пока есть осознание, что если мы этот инструмент уберем, то старая проблема вернется, — мы все делаем правильно.
В целом согласен, но итоговый вывод не факт. Если есть осознание, что ревью — это далеко не самый лучший инструмент для решения старой проблемы, а может и новой какой-то, которой не было, когда его вводили (и проект живой, и процесс ревью живой) или вводили не думая, что он решит и эту проблему, то, как минимум, стоит подумать о том, чтобы снять с ревью часть его задач, доверив их решение более подходящим инструментам типа линтеров, автоматически снимаемых метрик кода или тестов.
Грубо говоря, микроскопом и гвозди забивать, и возбудителей болезней искать в крови — это вряд ли правильно, за исключением каких-то особых случаев. При том не просто гвозди забивать было бы эффективней молотком или строительным пистолетом, а и качество поиска микробов страдает от того, что гвозди забивают.
Грубо говоря, микроскопом и гвозди забивать, и возбудителей болезней искать в крови — это вряд ли правильно, за исключением каких-то особых случаев. При том не просто гвозди забивать было бы эффективней молотком или строительным пистолетом, а и качество поиска микробов страдает от того, что гвозди забивают.
Большая просьба автору писать чуть покороче. Времени и так нет. Это не претензия, просто пожелание для будущих статей.
Доля правды есть, но слишком категорично утверждается, что ревью должно решать единственную задачу как практически последний рубеж обороны от разработчика. Провокация?
Ревью, как часть процесса разработки и доставки, может решать множество задач в рамках конкретной компании/проекта/продукта. И это не значит, что это неправильно. Может для каких-то задач есть более эффективные в теории инструменты, но, например, стартовые, постоянные и(или) переменные издержки в них неприемлемы здесь и сейчас.
Начинать решать правильно мы проводим ревью иили неправильно, множно ли что-то улучшить надо с составления формального описания актуального состояния дел, с опроса участников и бизнес-заказчика, формализованного списка бизнес-целей, которых ревью должно помочь достигать, бизнес-задач, которые оно должно решать. Бизнес-требования к ревью и описание актуального бизнес-процесса ревью. А потом уже анализ их соотвествия и возможности оптимизации, и лишь возможно, но не обязательно эти оптимизации будут связаны с качественными изменениями, типа снятия с ревью задач, которые либо решаются уже параллельно, либо куда эффективней по всем метрикам решаются иными способами. В общем обычный подход к оптимизации бизнес-процессов.
Если вы, владельцы бизнес-процесса ревью и бизнес-процессов вообще в вашей компании, считают, что у ревью единственная бизнес-цель, которую оно эффективнее всего решает, а другие цели должны достигаться с помощью более эффективных для них инструментов, то, да, неправильно использовать ревью для других целей. Но если столь редкого единодушия нет, то неправильно говорить «вы делаете это неправильно». Навскидку, например, у компании может не быть возможности внедрять более эффективные инструменты обучения и коучинга (каждый раз когда думаю, что уже знаю, что такое коучинг, узнаю его новые значения) или внедрить автоматические инструменты проверки подавляющего большинства правил форматирования кода.
Неправильно, разве что, иметь процесс код ревью, но не иметь представления каким бизнес-целям он служит в теории, и что производит на практике. Нужны сформулированные цели и нужны метрики для проверки соотвествия им актуального процесса.
Ревью, как часть процесса разработки и доставки, может решать множество задач в рамках конкретной компании/проекта/продукта. И это не значит, что это неправильно. Может для каких-то задач есть более эффективные в теории инструменты, но, например, стартовые, постоянные и(или) переменные издержки в них неприемлемы здесь и сейчас.
Начинать решать правильно мы проводим ревью иили неправильно, множно ли что-то улучшить надо с составления формального описания актуального состояния дел, с опроса участников и бизнес-заказчика, формализованного списка бизнес-целей, которых ревью должно помочь достигать, бизнес-задач, которые оно должно решать. Бизнес-требования к ревью и описание актуального бизнес-процесса ревью. А потом уже анализ их соотвествия и возможности оптимизации, и лишь возможно, но не обязательно эти оптимизации будут связаны с качественными изменениями, типа снятия с ревью задач, которые либо решаются уже параллельно, либо куда эффективней по всем метрикам решаются иными способами. В общем обычный подход к оптимизации бизнес-процессов.
Если вы, владельцы бизнес-процесса ревью и бизнес-процессов вообще в вашей компании, считают, что у ревью единственная бизнес-цель, которую оно эффективнее всего решает, а другие цели должны достигаться с помощью более эффективных для них инструментов, то, да, неправильно использовать ревью для других целей. Но если столь редкого единодушия нет, то неправильно говорить «вы делаете это неправильно». Навскидку, например, у компании может не быть возможности внедрять более эффективные инструменты обучения и коучинга (каждый раз когда думаю, что уже знаю, что такое коучинг, узнаю его новые значения) или внедрить автоматические инструменты проверки подавляющего большинства правил форматирования кода.
Неправильно, разве что, иметь процесс код ревью, но не иметь представления каким бизнес-целям он служит в теории, и что производит на практике. Нужны сформулированные цели и нужны метрики для проверки соотвествия им актуального процесса.
Спасибо, VolCh, вы все верно понимаете. Судя по вашим комментариям, знаете о проблеме изнутри.
У меня и моих супервизоров всю жизнь в роли программиста ревью как-то был инструментом именно оценки решения задачи. Так как до попадания кода (я писал на js) в репу посредством гит хуков была введена цепочка eslint (в конфиге airbnb) → prettier → юнит тесты → commitlint, а при попытке открыть пул реквест в основную dev-ветку из feature-веток прогонялись е2е тесты в селениуме, то код, попадающий на ревью был уже сравнительно чист, отформатирован и нормально работал — и по моим ощущениям в основном проверялся на некое "качество исполнения" — отметались разнообразные велосипеды, грязные хаки, недокументированные / непонятные куски кода, и прочее подобное гадство (обычно с комментариями, но тут уже как повезёт — попадались и оригиналы с синдромом вахтёра, заворачивающие код с ужасающими пометками а-ля "ЧТО ЭТО ВООБЩЕ ТАКОЕ?" или просто "deny").
Не скажу что всё вышеописанное было вот прямо как-то очень круто или правильно, но удобно было явно. Нет, были конечно неприятные случая когда люди уставали смотреть на ошибки линтера и коммитили с --force, но потом народ перешёл на гитлаб и поставил pre-recieve-хук.
Как-то так.
Итого вам нужно быть уверенным что:
решение описанное в коде на ревью, оптимальное
полностью протестировано
новички с ним ознакомились
свежие идеи пошарены в команде
демо новой фичи
и т.д и т.п…
Не слишком ли много для ревью? Процесс итак занимает кучу времени, скучен и однообразен.
С другой стороны Pair-programming, mob programming помогают справиться с практически всеми вопросами которые люди пытаются взвалить на код ревью. Тут вам и нахождение оптимального решения, шаринг, обучение, тест покрытие… В итоге ревью превращается в обычную формальность которая может быть автоматизирована.
решение описанное в коде на ревью, оптимальное
полностью протестировано
новички с ним ознакомились
свежие идеи пошарены в команде
демо новой фичи
и т.д и т.п…
Не слишком ли много для ревью? Процесс итак занимает кучу времени, скучен и однообразен.
С другой стороны Pair-programming, mob programming помогают справиться с практически всеми вопросами которые люди пытаются взвалить на код ревью. Тут вам и нахождение оптимального решения, шаринг, обучение, тест покрытие… В итоге ревью превращается в обычную формальность которая может быть автоматизирована.
Не совсем понятно что за «Эти четыре момента — это именно те...» имеются ввиду в заключении. Выше дюжина пунктов, какие из них — те самые 4?
Дополнительное перечисление их в заключении было бы полезно для понимания.
Дополнительное перечисление их в заключении было бы полезно для понимания.
Не очень понял, о чем статья, сорри:), но при код-ревью я лично руководствуюсь следующими принципами:
Да, и крупных переделок у нас не бывает после code review, только косметика, потому что все детали реализации всех крупных изменений обсуждаются заранее, а мелочь и так люди обычно делают правильно.
- реализуется ли полностью фича Y в данном код ревью X
- нет ли ошибок? проверены ли граничные условия? есть ли юнит-тесты? покрывают ли они задачу?
- не будет ли проседания по скорости, если одобрить X?
- если ответы «да», «да», «да», то про некоторые места спрашиваю почему здесь поменялось то-то и то-то (хотя я сам понял почему поменялось или думаю, что понял)
- если автор бодро и резво отвечает, значит он знает, что делает, и я апрувлю pull request
Да, и крупных переделок у нас не бывает после code review, только косметика, потому что все детали реализации всех крупных изменений обсуждаются заранее, а мелочь и так люди обычно делают правильно.
На самом деле надо признать, что в code review есть два класса задач, каждая из которых может решаться, а может и игнорироваться.
1. Проверка качества кода. «А зачем ты тут так странно делаешь, когда есть strange.do_it()?» У тебя тут сортировка вручную написана. Вот этот код будет работать, но его совершенно невозможно понять. Раздели в этом месте код не несколько независимых функций. Переменная tmp3 недостаточно информативная. Etc. Фактически, это эквивалент редакторской правки для статьи или книги.
2. Проверка на соответствие кода архитектуре проекта. Это может делаться только тем, кто эту архитектуру знает. Предпочительно — PTL, или самый старший из программистов. Он не спрашивает про сортировку пузырьком, он спрашивает «зачем ты тут используешь старый паттерн, который мы усиленно выпиливаем?», «не трогай объект Foo, т.к. он под сильным рефакторингом прямо сейчас, лучше используй вот это и это».
Совершенно разные задачи, совершенно разные комментарии. Первое может делать любой более-менее освоившийся программист, второе — только человек глубоко в проекте.
1. Проверка качества кода. «А зачем ты тут так странно делаешь, когда есть strange.do_it()?» У тебя тут сортировка вручную написана. Вот этот код будет работать, но его совершенно невозможно понять. Раздели в этом месте код не несколько независимых функций. Переменная tmp3 недостаточно информативная. Etc. Фактически, это эквивалент редакторской правки для статьи или книги.
2. Проверка на соответствие кода архитектуре проекта. Это может делаться только тем, кто эту архитектуру знает. Предпочительно — PTL, или самый старший из программистов. Он не спрашивает про сортировку пузырьком, он спрашивает «зачем ты тут используешь старый паттерн, который мы усиленно выпиливаем?», «не трогай объект Foo, т.к. он под сильным рефакторингом прямо сейчас, лучше используй вот это и это».
Совершенно разные задачи, совершенно разные комментарии. Первое может делать любой более-менее освоившийся программист, второе — только человек глубоко в проекте.
UFO just landed and posted this here
Огромное спасибо за статью! Планируете ли перевод на английский?
Спасибо за ваш интерес. Да, на английский переводим. Как опубликуем, я дам тут ссылку.
peterdemin перевод на английский готов: badootech.badoo.com/the-secret-to-great-code-reviews-4896639afc0e
Sign up to leave a comment.
Code review: вы делаете это неправильно