10 советов, как ревьюить код, который вам не нравится

Автор оригинала: David Lloyd
  • Перевод
  • Tutorial
Я постоянно делаю коммиты в проекты open source (Red Hat и др.). И заметил, что больше всего времени отнимают негативные код-ревью, субъективные по сути. Чаще всего такое происходит с коммитами, где мейнтейнеру по какой-то причине не нравится ваше изменение. В лучшем случае такая стратегия код-ревью приводит к потере времени в бессмысленных спорах; в худшем случае он активно препятствует коммиту, создавая враждебную и элитарную среду.

Код-ревью должен быть объективным, кратким и, по возможности, содержать только определённые факты. Это не политический или эмоциональный спор, а технический. Его цель — продвижение вперёд, развитие проекта и всех участников. Любой коммит должен оцениваться только по существу, а не по субъективному мнению.

Стратегии код-ревью


Вот несколько стратегий, которые следует иметь в виду при рассмотрении кода, которые вам (как мейнтейнеру) по какой-то причине не нравится:

1. Перефразируйте возражение в виде вопроса


  • Плохо: «Это изменение сделает XXX невозможным» (Это преувеличение; неужели на самом деле это невозможно?)
  • Хорошо: «Как мы будем делать XXX после вашего изменения?»

2. Избегайте преувеличений


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

  • Плохо: «Это изменение уничтожит производительность».
  • Хорошо: «Похоже, что X может быть медленнее, чем существующий Y; вы проводили измерения/собрали данные, чтобы показать, что это не так?»
  • Лучше (если у вас есть время): «Я пока собираю данные. Попробую проверить, что X не медленнее Y».
  • Тоже хорошо: «Это изменение изменяет одиночный цикл O(n) на дважды вложенный цикл O(n²); не повлияет ли это на производительность?»

3. Оставьте ехидные комментарии при себе


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

  • Плохо: «Думаю, что это плохое изменение, которое всё порушит».
  • Плохо: «Вы уверены, что разработка программного обеспечения для вас правильный карьерный выбор?»

4. Действуйте позитивно


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

  • Плохо: «Это изменение отстой, моя версия лучше».
  • Хорошо: «У меня тоже есть изменение для этого места: возможно, мы можем сравнить и/или объединить идеи».
  • Тоже хорошо «У меня в работе аналогичное изменение, но я решил сделать X, потому что ZZZ; почему вы выбрали Y?»

5. Помните, что у всех разный опыт


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

  • Плохо: «Разве вы не видите, что это явно неправильно?»
  • Хорошо: «Это неверно, потому что вызывает исключение нулевого указателя, когда X равен Y».

6. Не преуменьшайте сложность того, что не очевидно


Помните, что очевидные для вас вещи могут быть очевидны не для всех. Предлагая альтернативные подходы и указывая на полезные примеры, вы поможете всем синхронизироваться.

  • Плохо: «Почему бы просто не фробнуть гноззл?»
  • Хорошо: «Если фробнуть гноззл, то данная часть может стать проще (см. XXX для примера)».

7. Относитесь с уважением


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

  • Плохо: «Это глупый код, написанный глупым человеком».
  • Хорошо: «Спасибо за ваш вклад. Однако он не может быть принят в нынешнем виде; существует множество проблем (как указано выше)».
  • Тоже хорошо: «Как указано выше, с этим коммитом есть несколько проблем. Может, отступить на шаг и поговорить о сценариях использования? Это поможет нащупать путь».

8. Управляйте ожиданиями (и своим временем)


Если коммит слишком большой и его нельзя быстро рассмотреть, то нормально сразу об этом сказать. Затем ищите вариант решения.

  • Плохо: «Я не принимаю его, он слишком большой».
  • Тоже плохо: игнорировать коммит, пока он не исчезнет.
  • Хорошо: «Не могли бы вы разбить его на более мелкие коммиты? У меня не слишком много времени для код-ревью, а это просто слишком большой/сложный коммит для одного ревью».

9. Скажите «пожалуйста» (проявите вежливость)


Просто сказав «пожалуйста», вы показываете, что уважаете время отправителя, особенно когда хотите изменить форматирование или стиль, что может показаться незначительным изменением. Примеры:

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

10. Начните разговор


Если после всего этого вам всё ещё что-то не нравится, но вы не уверены, почему, возможно, придётся просто смириться. Или сказать: «Мне это не нравится, но я не уверен, почему, можем поговорить об этом?» Это разумный вопрос, и даже хотя это может занять немного времени, но часто стоит того, потому что теперь у вас два человека, и оба учатся (объяснять и слушать), а не два человека, которые противостоят друг другу.

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

Резюме


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

Комментарии 35

    0
    Было бы интересно если бы были приведены в качестве примера (как хорошие, так и плохие) некоторые code review для open source проектов которые всем доступны для просмотра.

    Как говорится: «одна картинка стоит тысячи слов», то «один реальный пример code review стоит десяти статей про то что делать и что не делать»
      0

      open source ревью обычно всем доступны для просмотра, поэтому они и open по определению.
      Смотрите ревью в каких либо больших проектах, часто они на гитхабе, там обычно профессиональнее чем в маленьких проектах.

      0
      Смысл пункта 9 полностью потерялся при переводе :( Да и вообще говорить «пожалуйста» в русском языке на каждый чих не очень принято
        0

        Возможно имелось в виду использовать could вместо can. Хотя для меня разница не принципиальная.

          0
          В оригинале как раз про «please», а не про «could — can».
          В русском не принято в каждой просьбе использовать слово «пожалуйста», в английском принято, у них частота его использования сильно выше. Это, кстати, одна из причин по которой русских считают не очень вежливыми.
        0
        «почему бы вам просто...».
        Я вот, например, не знаю почему человек сделал именно так как сделал, и хочу получить ответ на мое более простое решение. И вполне логично, что мой вопрос будет сформулирован например как «почему бы вам просто не убрать копирование?». И что в нем не так?

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

        Хорошо: «Как мы будем делать XXX после вашего изменения?»
        Подойдет к тебе в реале человек и скажет такое с надрывом — и ты идиот перед всеми остальными. Хорошо? Палка о двух концах.
          0

          "просто" — это оценочное слово. Можно, например, сформулировать как "Почему тут нельзя сделать вот так?" и, если это не очевидно, перечислить преимущества и недостатки предложенного варианта. Это способ сильно повысить вероятность того, что обсуждение будет конструктивным, а не свалится в перекидывание казашками между двумя д'Артаньянами.

            0

            Предложите свой вариант и скажите почему он лучше. Это заметно снизит вероятность понимания "не так" если вы пишете по делу, а не раздаете оценки.

            0

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

              0

              Культурные особенности они такие, да и мир — не самое дружелюбное место)

                0
                Интернет тем более
                +2

                Статья о том как избежать претензий и перебранок если вам что-то субъективно не нравиться в просмотренном материале. И первые пять комментаторов правилам точно не последовали :)))

                  +2

                  Добавлю ссылки на статью о двух частях, которая мне лично очень помогла с проведением code review: How to Do Code Reviews Like a Human (part one, part two).

                  –3
                  Если я знаю, что будут придираться, то посылаю очень сырой код, — пусть за одно и проверят. А я пока отдохну.
                    0
                    Если я такое чуствую, то скорее вы получите отказ в мерже. Очень много времени забирает ревью кода, но без контрибюторов, опен сорс это сизифов труд. Вы пользуетесь продуктом, вам что-то надо, мы можем сказать насколько это сложно и указать места и как это делать, но явный тяпляп это перебор.

                    Не раз себя ловил на мысли: блин, я бы уже за час это написал, но нет надо держать себя в руках, контрибюторы должны подучиться.
                      0
                      А кто ревьюит ревьюера? почему он истина?
                        0
                        Ревьювер это не судья, а помощник, который может подсказать как сделать код лучше.
                        И его стоит слушать, потому что обычно у него больше опыта в том коде, которым он владеет.
                          0
                          Если говорить о тотальном контроле качества на предприятии, то получается ревьюера никто не контролирует. Зачастую получается, кто первых халат одел, тот и доктор.
                            0
                            Ревьювер не обязательно один. У нас принято правило, что ПР прошел, если более 60% ревьюверов дало добро.
                            Ну и стоит следить за ситуацией, когда ПР проходит только потому что у всех не было до него дела и поставили аппрув чтобы не мешался.
                              0

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

                            0
                            Все кому это доступно. Если ты на опенсорс проекте несколько лет, то твое мнение что-то да значит. И как правило, я пишу почему конкретный кусок кода не подходит и как это исправить.
                        0
                        Иными словами, комментировать нужно позитивно, конструктивно и вежливо!
                          +1
                          Как-то стремно звучит.
                          Не знаю как остальным, но лично мне не нравится вся эта псевдокультурность. Я люблю когда говорят прямо и по делу. Если код говно, то код говно и это факт. Не имеет зачения я сказал или ко мне претензия — это просто факт (примечание: я говорю про аргументированные моменты).

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

                          Сори, че-то накипело.
                            0
                            Если код говно, то код говно и это факт.


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

                            Восприятие размытых формулировок обычно требует инсайтов, а перед инсайтом многие животные (включая людей) становятся возбужденными и агрессивными. Вдобавок размытые формулировки снимают ответственность с инструктирующего, поскольку если бы он предложил решение — то оно могло бы оказаться тоже неправильным. По сути, он перекладывает риск с себя на ученика.
                            Разумеется, размытые формулировки при таких раскладах будут чаще встречать отторжение, чем чёткие и недвусмысленные инструкции.
                              0
                              Гм… А с чего вдруг ревьювер — это обязательно учитель/ментор?
                              С обучением джунов подход совсем другой. Я свою мысль несу про ревью полноценный равновесных коллег.
                              0
                              А если с вами будут говорить точно также? Готовы ли вы принять точно такое отношение и к себе?
                                0
                                Легко. Я люблю когда люди говорят честно и прямо — не надо ничего додумывать.
                                  0
                                  Могу только восхититься!
                                    0
                                    Как по мне, то это должно быть нормальным адекватным поведением.
                                    Я часто встречаюсь с ситуацией, что люди не могут отделить код от себя и все, что касается кода они принимают на душу. Вот именно это и есть проблема. Человек может быть сколь угодно хорошим, но если результат его работы попахивает, то комментарии будут именно к результату работы, а не к человеку.

                                    Как мне кажется, первое, чему нужно учить разработчиков — отделяться от кода и не воспринимать его как часть себя. Конечный результат — пожалуйста. Способ его достижения — будь готов спокойно отправить в /dev/null, иначе не будет никакого развития и роста.

                                    P.S. В особых случаях программисты относятся к программе как… «оно как живое, нужно аккуратно вносить изменения» и все в таком духе. Тут каждому должно быть понятно, что архитектурно программа написана настолько их рук вон плохо, что прямо слов нет, только адовые комментарии к коду. Хотя скорее всего тут уже и к кодописателю будут совсем нелестные комментарии.
                            +2
                            Ох, не знаю. У меня был коллега, который на подобные вопросы (Как мы будем делать XXX после вашего изменения?) реагировал обидой, воспринимая их подколом (на самом деле здесь должно было быть более экспрессивное слово, но оно нецензурное). Более того, задавая подобные вопросы, вы явно знаете, что вы имеете в виду, но вы об этом не говорите. К чему это приводит? К тому, что если коллега вас не понял, то вы еще долго будете играть в вопрос-ответ. А выглядеть это будет издевательством над тем самым коллегой.
                              +5
                              Это изменение изменяет одиночный цикл O(n) на дважды вложенный цикл O(n²); не повлияет ли это на производительность?

                              «Послушай, разве ты не видишь, что твоему товарищу за шиворот падают капли расплавленного олова?»

                                0
                                Вот из-за таких людей лиинукс теперь без Линукса…
                                  +2
                                  Во-первых, его зовут Линус. Во-вторых, линукс с ним после месячного отпуска.
                                  0
                                  В целом ревью это же по сути критика, а там давным давно уже написано как это делать правильно. Занимайтесь критикой а не критиканством и научитесь ненасисльственному общению

                                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                  Самое читаемое