Отучаемся от токсичных практик на код-ревью

Автор оригинала: Sandya Sankarram
  • Перевод

Код-ревью частенько порождают споры. При подготовке лекции «Отучаемся от токсичного поведения на код-ревью» на конференции AlterConf я была готова услышать кучу возражений и критики. Но совершенно не ожидала, что сообщество настолько поддержит идею. Я предполагала сопротивление, но сообщество очень доброжелательно и с одобрением приняло меня. 

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

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

Непродуктивное поведение № 1: передача мнения как факта


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

Вместо того, чтобы сказать:

Этот компонент следует сделать без изменения состояния (stateless).

…лучше предоставить некоторый контекст:

Поскольку у этого компонента нет методов жизненного цикла или состояния, его можно сделать stateless. Это повысит производительность и читаемость кода. *Вот* некоторая документация.

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

Такие обсуждения лучше вести отдельно и определиться со стилем для всей команды. Внедрите линтер и автоматические исправления кода. Затем будете ссылаться на эти рекомендации по стилю, а не на своё личное мнение.

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

Непродуктивное поведение № 2: лавина комментариев


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

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

Непродуктивно и подавляюще:



Несколько комментариев для одной повторяющейся ошибки (пробелы в конце строки)

Более полезный вариант:


Консолидированный фидбек

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

Непродуктивное поведение № 3: просить инженеров решать чужие проблемы, «пока они здесь»


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

Я не предлагаю лишить разработчиков ответственности за код только потому, что они изначально его не писали. На самом деле, нехорошо говорить что-то вроде «Чужой код — не моя проблема». Просто более целесообразно создать отдельный тикет и пулл-реквест для исправления грязного кода. Таким образом, вы избегаете резкого увеличения объёма чьей-то работы, решая проблему с техническим долгом.

TL;DR: Не просите разработчика исправлять проблемы «пока они здесь». Если код делает что требуется в тикете и не вводит новых проблем в кодовую базу, одобрите его, а затем создайте тикет для очистки кодовой базы.

Непродуктивное поведение № 4: задавать осуждающие вопросы


Избегайте задавать осуждающие вопросы вроде такого:

Почему ты здесь просто не сделал ____ ?

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

Часто эти осуждающие вопросы — просто завуалированные требования. Вместо этого предложите рекомендацию (с документацией и ссылками), опустив резкие слова.

Вы можете сделать ____, что имеет преимущество ____.

Непродуктивное поведение № 5: сарказм


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

Непродуктивно:

Вы хотя бы тестировали этот код перед отправкой?

Полезно:

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

Вот ещё один пример комментария в код-ревью, который ни смешон, ни полезен:

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

В приведённом примере «бип!» — совершенно не полезно и не содержательно. Это педантичный юмор, который не помогает автору.

Непродуктивное поведение № 6: Эмодзи вместо описания проблемы


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


Не используйте эмодзи для указания на проблемы с кодированием

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

Вполне нормально использовать положительные смайлики, такие как «большой палец вверх» или «ура!», но не для указания на проблемы, а как отзыв на хороший код.


Одобряющие смайлики — это здорово

Непродуктивное поведение № 7: не отвечать на все комментарии


Авторы тоже вносят свой вклад в токсичность обсуждения. Если вы объединяете код, не ответив на все замечания, то это вызывает удивление у человека: он старался вам помочь, а вы даёте понять, что некоторые отзывы не имеют значения.

Если комментарий нерелевантен или вы не будете принимать меры, просто кратко объясните, почему.

Не игнорируйте коллег.


Объединение кода без ответа на комментарий

Непродуктивное поведение № 8: игнорирование токсичного поведения, если оно исходит от лучших профессионалов


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

Опыт работы с человеком, который демонстрирует токсичное поведение:

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

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

Я лично испытывала большое беспокойство всякий раз, когда получала электронное письмо с комментариями на мой пулл-реквест от одного бывшего коллеги (известного своими токсичными, малополезными отзывами). Хотя токсичное поведение на всех влияет по-разному, но определённо никто не выигрывает от этой токсичности.

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

Полезные практики код-ревью


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

Полезное поведение № 1: используйте вопросы или рекомендации для налаживания диалога


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

Почему вы просто не объединили эти преобразования в файле с константами?

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

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

…или предложите рекомендацию:

В этом файле у вас много вызовов трансляции для «функции x». Возможно, имеет смысл создать отдельный файл с её константами.

Полезное поведение № 2: сотрудничать, не прятаться


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

«…когда вы хотите помочь или работать вместе, вы должны полностью участвовать, а не просто иногда вмешиваться» — руководство Recurse Center

Полезное поведение № 3: отвечайте на каждый комментарий


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

Например:

Человек A: — Что вы думаете о создании вспомогательной функции для этого вызова API? В остальном всё хорошо.

Человек Б: — Эта строка не входила в мой набор изменений. Я объединю код, создам отдельный тикет на GitHub и запишу в план работ для нашей группы.

Полезное поведение № 4: знать, когда лучше организовать личную встречу


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

Таким образом, группа сможет быстрее принять решение и применить его.

Проблемы, которые занимают часы и тонны комментариев, обычно можно решить за несколько минут продуктивного разговора. — «Аккуратная Java»

Полезное поведение № 5: Используйте возможности для обучения и не хвастайтесь


Прежде чем принять участие в обзоре кода, спросите себя:

Ваш комментарий действительно помогает учиться или вы придираетесь?

Подумайте о своём комментарии. Помните, что цель кода-ревью — научить и помочь другим разработчикам расти. Это не трибуна для выступлений.

Полезное поведение № 6: не показывать удивление


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

Подробнее см. в правиле «Не притворяйтесь удивлённым» из руководства Recurse Center.

Полезное поведение № 7: автоматизируйте всё, что можно


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

Есть также инструменты для автоматического запуска тестов при добавлении нового кода. Они отображают предупреждения, если набор изменений нарушает какой-то из тестов. Например, TeamCity и Jenkins CI.

Кроме того, используйте хуки Git для запуска тестов и линтеров на новом коде. Они перехватят коммит, если обнаружат баги.

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

Полезное поведение № 8: не перенимайте токсичное поведение


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

Найдите коллег, которые вас поддержат.

Если вы заметили непродуктивные код-ревью, попытайтесь сообщить коллеге, говорите прямо и честно (если чувствуете себя безопасно в своей должности/компании).

Полезное поведение № 9: менеджеры, внимательно рассматривайте кандидатов, прислушивайтесь к команде и применяйте свои полномочия


У менеджеров большие возможности для создания позитивной и благоприятной атмосферы в коллективе.

Перефразируем советы из статьи «Вред токсичных разработчиков»:

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

(*) Хотя в статье предлагается изолировать токсичных разработчиков, я считаю важным поощрить токсичного разработчика к сотрудничеству с командой, но более здоровыми способами. Изоляция не решит проблему. Если дать ему отдельную работу, токсичность снизится в краткосрочной перспективе, но разработчик не откажется от своего непродуктивного поведения. Он только потеряет трибуну, чтобы его демонстрировать.

Полезное поведение № 10: установите стандарт, пока команда мала и растёт


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

Полезное поведение № 11: понять, что вы можете быть частью проблемы


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

Будучи джуниором, я несколько раз замечала ошибку в чьём-то коде и радовалась, ведь я могла оставить комментарий! Теперь я понимаю, что использовала такую возможность, чтобы хвастаться, а не помогать. Нужно внимательно оценивать свои действия.

Думаю, что каждому полезно уделить время этому трудному самоанализу.

И последнее…

Мы говорим не о содержании отзывов, а только о тоне


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

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

Подробнее
Реклама

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

    +8
    Все примеры должны быть автоматизированы и частью статического анализа
      +11
      На КДПВ всё наоборот. Заставлять человека читать кучу воды вместо того, чтобы просто указать ошибку – неуважение (исключение – когда ты сам влез к этому человеку с информацией об ошибке, никто тебя не просил о code review).
        +4

        Воды много, но лучше написать "гайдлайн X не выполняется, перепроверить", чем тыкать в каждое место отдельно. Хотя бы во избежание "мне написали поправить А, Б и В — их я и поправил, а Г и Д тупой злобный ревьюер не заметил".

          0
          Согласен, указывать одно и то же много раз — тоже плохо. Хуже было бы только на каждую строчку с ошибкой написать по две строчки оправданий за то, что на неё указываешь.
            +7
            чем тыкать в каждое место отдельно

            Практика показывает что приходится тыкать, иначе люди делают удивленное лицо и говорят, а что вроде всё по гайдлану.

            Вообще всё зависит от ревьювера, кому-то не лень откомментить каждый пробел, кому-то лень.

            Другое дело это личные проблемы восприятия — надо быть закомплексованным человеком, чтобы воспрнимать коммент под каждым пробелом как давление на своё ЧСВ.
            Вообще воспринимать всё в рамках ЧСВ и токсчиности — вредно. Ты не знаешь с кем этот человек общался до этого, и может ему уже надоело весь день отвечать на глупые вопросы а хочется уже поработать.

            Всегда надо оценивать компетентность ответа который вам дали и не бояятся задавать вопросы, а по ответам можно будет уже оценивать и свою компетентность, и профессионально расти.
              +1

              Я как-то предпочитаю "явное лучше неявного" — т.е. лучше сказать, что есть нарушения каких-то практик, в частности, в точках А, Б и В, но я не гарантирую, что это исчерпывающий список.


              Вообще воспринимать всё в рамках ЧСВ и токсчиности — вредно.

              Да ладно, учитывая пол и национальность автора оригинала — у неё ещё до фига взвешенная точка зрения.


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

              Мега-труЪ. Особенно про "не бояться задавать вопросы".

              0

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

                0
                Тут как раз просто. В первый раз даём общее замечание, если пришло второй раз — ставим метки везде, где эта ошибка есть, пришло в третий — издеваемся «бип»-ами. Если ничего не помогло, идём к руководству с вопросом о несответствии автора занимаемой должности.
              +2
              Такие вещи, как на КДПВ вообще по-хорошему должны проверяться линтером на пре-коммит хуке и не давать класть такое в репозиторий.
              +5
              Категорически согласен по поводу примера про «бип» — встречал подобное, хтелось просто убивать за такие «бипы». Ощущение самоутвеждения за мой счёт. Скажи один раз — мол, дружище, у тебя есть систематическая проблема, пробегись по коду. Не-е-ет, надо каждый раз носом натыкать, ЧСВ своё почесать!.. Извините, наболело)
              А вот по поводу того, что вопрос «Почему вы не использовали ___?» некорректен — ак же категорически не согласен. Этот вопрос означает, что ревьюер НЕ считает себя всезнающим, признаёт, что его мнение не бсолютно, и интересуется чужим. Может быть, он просто чего-то не понял, и применять его любимый ___ здесь просто нельзя?
                0
                А вот по поводу того, что вопрос «Почему вы не использовали ___?» некорректен — ак же категорически не согласен. Этот вопрос означает, что ревьюер НЕ считает себя всезнающим, признаёт, что его мнение не бсолютно, и интересуется чужим. Может быть, он просто чего-то не понял, и применять его любимый ___ здесь просто нельзя?


                В момем опыте чаще были примеры, когда ревьюер задает такой вопрос по причинам, которые не относятся к коду. Например если других вопросов нет, но хочется докопаться, чтобы человек не подумал, что у него все хорошо, а тут целый простор для дискуссии и морального доминирования.
                Если человеку интересно чужое мнение, то он может его и в личной беседе спросить, не засоряя ПР.
                  0
                  Здесь вопрос может подразумевать, что программист знает и обязан (?) был сделать так.

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

                  Интервьюер ожидает не того, что программист только ответит на вопрос, а перенесет код в отдельный файл.

                  Дурость.
                  Спросили — ответил. Сказали сделать — сделал.
                  Не смешивать.
                  +7
                  Статья большей частью выглядит как «Передача мнения как факта» )
                  Далеко не всегда надо все это соблюдать. Много частных случаев.
                  В моей практике были люди которые просто не понимали нормального отношения.
                  Им бесполезно было говорить «у тебя системная проблема».
                  Поэтому им и бикали и пикали и чего только не делали. А уволить их было невозможно…
                    0

                    Ваше мнение о статье не совпадает с моим.


                    Во-первых, любая статья — это всегда мнение. За фактами — это к криминалистами, да и там не сто процентов.


                    Во-вторых автор нигде не говорит о том, что его мнение — единственно верное. Более того, цитирую:


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

                    Я здесь вижу: "люди, старайтесь быть людьми к всеобщей пользе". При этом нигде в тексте не нашёл "Кто делает не так, тот редиска" или "Предложенные методы являются универсальной таблеткой и применимы без условий и ограничений". Поэтому лично у меня заявление о "Передаче мнения как факта" вызывает лёгкое недоумение.


                    P.s. Не являюсь экспертом. Не претендую на истину в последней инстанции. Вы имеете право на мнение, отличное от моего.

                    +11
                    Не просите разработчика исправлять проблемы «пока они здесь». Если код делает что требуется в тикете и не вводит новых проблем в кодовую базу, одобрите его, а затем создайте билет для очистки кодовой базы.

                    Плохая идея. Такие todo откладываются в долгий ящик в 99% процентах случаев, а призывы "ну давайте уже почистим наш код" отвергаются как начальством ("у вас это в спринт не входит, сделаете эту таску потом, когда завершите ещё n спринтов с мега-срочными фичами"), так и другими программистами ("у меня и так в жире куча незакрытых тасок висит, у меня нет времени").

                      0
                      Почему бы подразумевать по умолчанию исправление некоторого техдолга/рефакторинг в стандартный план? Точно так же как и тесты, кодревью и все остальное?
                      Я понимаю, что не везде это возможно по тем или иным причинам, но на мой взгляд стремится к этому уже будет хорошо. Хотя бы для себя
                        +3

                        Потому что на любом митинге идея встроить в план исправление техдолга с удовольствием примется и отложится в долгий ящик по всё тем же причинам. Стремиться к этому, в свою очередь, можно бесконечно.

                        • НЛО прилетело и опубликовало эту надпись здесь
                        –1
                        С одной стороны да. С другой стороны, меня всегда очень бесили комментарии на ревью, когда фикся конкретную очень проблему в файле с говнокодом, тебе предлагают взять и отрефакторить чуть ли не целиком весь этот файл (и полпроекта заодно).

                        Должна быть золотая середина. Простой фикс можно и сделать «заодно».

                        Но если этот todo очень простой — ревьюверу тоже ничего не стоит пофиксить этот туду прямо во время просмотра. Странно что у него есть время на изучение проблемы, написание комментария и нет, не было и не будет времени на фикс.

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

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

                          Например, меняет человек 1 строчку в инициализации класса — новый параметр константный.
                          А параметров там штук 5 и инициализируется он в 5 методах по всему файлу (и везде константы на месте объявлены). Я прошу исправить это, по факту ему придется весь файл переделать и + тесты.

                      • НЛО прилетело и опубликовало эту надпись здесь
                          +2
                          Предложение замечательное, окромя одного — в реальности (чаще всего) никаких Руководителей на ревью не напасешься. У нас есть тех дир, его конечно можно добавлять к ревью, но в реале он хорошо если в 1% случаев его просмотрит (просто тупо нет времени). Что там руководителей, у меня вообще просто в принципе ревьюера найти сейчас проблема (нужно править код по другой технологии, а ребята из другого отдела забиты работой по самое не могу, в итоге задержки по 1-2 недели на этом).
                          Я не встречал еще фирмы где есть такой избыток времени руководителей, если у вас оно есть — я безмерно рад за вас)
                          +2
                          Если вы объединяете код, не ответив на все замечания, то это вызывает удивление у человека

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

                          Избегайте задавать осуждающие вопросы вроде такого:

                          Это не осуждающий вопрос. Если мне правда интересно, почему сделано А вместо Б, мне что делать?
                            0

                            Когда мне задают вопрос "почему вы не использовали ___?", у меня возникает только одна ассоциация — лошадь в ванне с огурцами.


                            Потому что я просто не вспомнил о ? Не знал о ? Не знаю почему не использовал ___?


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

                              +1
                              Ну так и отвечайте, что не знали, не вспомнили и т.д. А если знали, но не использовали по какой-то известной вам, но неизвестной мне причине, назовите причину.
                              Зачем все усложнять?
                                0
                                Часто задаю такого рода вопросы. Исполнитель заведомо имеет более глубокое погружение в задачу, возможно он всё учел и осознано не использовал наиболее, как мне кажется, очевидное или правильное решение.

                                Таким образом, если исполнитель не знал или что-то забыл, то теперь он узнал/вспомнил, а если у него есть аргументированное объяснение, то, возможно, что-то полезное узнаю я.
                            • НЛО прилетело и опубликовало эту надпись здесь
                              • НЛО прилетело и опубликовало эту надпись здесь
                                  0
                                  Gerrit так позволяет.
                                • НЛО прилетело и опубликовало эту надпись здесь
                                  +4

                                  Имхо все проблемы решает вежливость. Достаточно написать "Please change X" или "Please consider X instead" и на этом все вопросы токсичности можно закрывать

                                    +1

                                    По КДПВ: есть коллеги, с которыми второй вариант не работает, они реагируют только на прямые указания, потому как в ревью борде это дефекты и их нужно закрыть. А просто просмотреть самостоятельно, хотя бы тот же файл, на предмет аналогичных ошибок… никак.

                                      0
                                      ну вещи типа висящих пробелов и расстановки скобок вообще не должны всплывать на ревью, т.к. это работа линтера, а не разработчика. Настройте один раз подходящий инструмент и живите спокойно
                                        0
                                        Видимо, есть очень богатые компании, которые постоянно тратят время дорогих разработчиков впустую, вместо того, чтобы один раз настроить гитхук.
                                          0
                                          Серверный?
                                            +1
                                            Достаточно клиентского. Тех, кто будет его обходить без весской причины, следует просто уволить. Вроде мы про взрослых людей пишем тут, а не про детский сад.
                                      +3
                                      Происходит сбой при вводе отрицательного числа. Не могли бы вы этим заняться, если не сложно?

                                      Меня вот это зацепило. Если в коде происходит сбой, то о каком «если не сложно» может идти речь? Это должно быть исправлено.
                                        +2
                                        Это обычная вежливость, не более. Примерно как «извините, вы не могли бы подвинуть свою машину от проезда». Если машина загораживает проезд — ее очевидно нужно подвинуть. Но очевидно, на фразу «машину с проезда убрал, быстро» вы среагируете скорее всего по другому. Хотя смысл один и тот же.
                                          +2
                                          Да, но можно же сказать: «Поправь этот баг, пожалуйста». Я к тому, что постановка фразы подразумевает, будто у человека есть возможность сказать «нет». Зачем это все, если мы оба знаем, что баг придется поправить вне зависимости от того, сложно человеку или нет?
                                            0
                                            Ну вот привычка такая у человека, стиль речи. «Не могли бы вы переставить машину, если не сложно, а то мы выехать не можем.». Да, это не самая лучшая формулировка.
                                            Но это опять же, обычная вежливость, цепляться за это странно, как по мне.
                                              0

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


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

                                            0
                                            Это уже особенности перевода и особенности их речи.

                                            В оригинале
                                            This breaks when you enter a negative number. Can you please address this case?


                                            В нашем переводе как раз и получается «Исправьте ошибку, пожалуйста». Кстати автор перевода этот момент с «пожалуйста» уже исправил.

                                            То, что использовано «Can you please address this case?» вместо прямого «Address this case.», то это как уже упомянули, особенность их речи, не принято в повелительном наклонении выражаться.
                                              0
                                              Все понятно, спасибо за разъяснение. В оригинале действительно нет этого «если не сложно», которое меня и зацепило. К отсутствию повелительного наклонения у меня претензий не было, в английском оно действительно предпочтительней.
                                            +2

                                            ИМХО, есть одно простое правило на все случаи жизни:
                                            "Поступай с другими так, как хочешь, чтобы поступали с тобой".
                                            Все токсичные практики (и не только в it) — всегда пример обратного.

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

                                                image


                                                И хоть бы слово о том, что не надо вообще обращать внимание на такие мелочи.

                                                  +3

                                                  Надо. Потому что следующий разработчик поправит в этом файле одну строку, прогонит файл через стандартный форматировщик и привет — в файле изменится сто строк. Разбирайся потом, какие изменения относятся к реально сделанной задаче, а какие просто зацепило форматировщиком.

                                                    –4
                                                    поправит в этом файле одну строку, прогонит файл через стандартный форматировщик

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

                                                      0

                                                      За что "это"? За форматирование кода в соответствии со стандартами?

                                                        –2

                                                        Я вполне однозначно процитировал за что.

                                                          +1

                                                          Нет. В цитате упоминаются два действия, с каким из них Вы не согласны?

                                                            –1

                                                            С их комбинацией.

                                                              0

                                                              Как надо?

                                                          –1
                                                          За то, что он не разделяет коммиты и валит все в одну кучу.
                                                          В вашем кейзе разработчик должен сделать два коммита — первый это «поправил одну строку», второй — «прогнал через стандартный форматировщик».
                                                            +1

                                                            Разделение на комиты не играет тут никакой роли. В итоговом мерж-реквесте (который и будет рассматриваться на код-ревью) все изменения будут отражаться одновременно.

                                                              –1

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


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

                                                                +1
                                                                Вы зачем-то свалили в кучу кодревью (на котором смотрят конечное состояние кода, независимо от того, какие там были промежуточные коммиты) и просмотр отдельного коммита (в некотором будщем).

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

                                                                Однако на код-ревью таковое разделение не повлияет вообще никак. На код-ревью смотрят весь конечный дифф и в нём будут отображены одновременно и исправление кода и форматирование.
                                                              –2

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

                                                                0

                                                                Если я правильно вас понял, то вы предлагаете


                                                                1. Oставить trailing spaces без внимания, т.к. это мелочь которую не надо замечать. Пусть она и нарушает code style
                                                                2. Запретить этому и любому другому разработчику исправлять пробелы в PR с полезной нагрузкой. И бить по рукам тем, кто так сделает.
                                                                3. Приводить оформление к оговоренному стилю только когда в файле ужас-ужас, но не раньше, когда просто ужас.

                                                                Я правильно вас понял?


                                                                Я с таким подходом никогда не сталкивался, и (теоретически) это провоцирует к отказу разработчиков применения авто-форматирования в IDE, ведь из-за автоформата придется проверять не сбились ли пробелы с "как было" на "как надо". Это, в свою очередь, приведет к постепенному нарастанию кома кода вне оговоренного стиля.
                                                                Если у вас есть опыт работы команды с таким подходом, поделитесь пожалуйста, насколько эти проблемы реальны? И какого уровня разработчики были в команде?

                                                                  0

                                                                  Какой ужас. Код с лишними пробелами ведь просто невозможно читать. Развели тут карго-культ на тему код-стайла.


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

                                                                    0

                                                                    Дело не в пробелах. Это малая часть code style. И дело не в карго-культе. Из ваших ответов я делаю вывод, что вы не считаете важным причины, по которым возникло понятие единого стиля кодирования для команды.
                                                                    Возможно, вам не встречались коллеги, которые пишут цикл в одну строку, else if разносят на 3 строки и ленятся настроить форматтер в IDE, и тем более его применять. Возможно, вы не понимаете, насколько замедляют чтение и понимание такие нестандартные куски кода.
                                                                    И вероятно, вы не получали по шапке от тимлида за смешивание в одном PR новый код и форматирование, когда приводите такие куски к удобочитаемому формату. Вы из тех, кто ругает за улучшение общей кодовой базы.

                                                                      –1
                                                                      Возможно, вам не встречались коллеги, которые

                                                                      Вот, вы сами пишите, что проблема в коллегах, а не "единстве стиля". Если коллега пишет говнокод, то надо либо объяснить ему что не так (или он объяснит вам что всё так), либо перестать с ним работать, если он не понимает разумных тезисов.


                                                                      вы не получали по шапке от тимлида

                                                                      Я никогда не даю по шапке за такие глупости. Го к нам работать.)

                                                                        0

                                                                        Я все меньше понимаю вас.


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

                                                                        совсем не вяжется с


                                                                        Я никогда не даю по шапке за такие глупости

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


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

                                                                        Так все-таки это глупости, на которые вы не обращаете внимания, или вещи, достойные отдельного пулл-реквеста?

                                                                          0

                                                                          Для изменения одной строчки не надо изменять весь файл, какой бы благородный мотив ни был. Исключение — легаси-говнокод — от него надо избавляться как можно быстрее.


                                                                          А вот эту строчку форматировать можно по разному. И как именно — не особо принципиально в разумных пределах.

                                                                    0

                                                                    Чем выше уровень разработчика тем меньше волнует пробельная тема если конечно пробелы не ломают xml ))). Бывает экран маленький и наличие лишнего пробела не заметно.

                                                                      0

                                                                      Именно поэтому я спрашивал, какого уровня была команда. Потому что опыт работы в all senjor команде неправильно распространять на другие, более распространенные случаи.
                                                                      И да, меня лично не напрягают ни пробелы в конце строки, ни табуляции. Но я понимаю, что в разноуровневой (и по мастерству, и по желанию считаться с другими) команде хорошо бы иметь правила, которым следуют все.

                                                                    0

                                                                    Вот разные PR — это как раз вредный совет. Одновременно-то их не сделать...

                                                                      0

                                                                      Почему же не сделать?

                                                                      +2
                                                                      Нет.

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

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

                                                                      Надежда на то, что форматирование и своего и чужого кода будет исправлено в другом мерж-реквесте, не обоснована. Потому что пока готовится это исправление, дугие разработчики уже внесут новые правки (а мы помним, да, что при этом они не форматируют код как надо, тоже планируя отформатировать его потом?). Конечный-исправляющий-все-косяки мерж-реквест не будет сделан никогда.
                                                                      • НЛО прилетело и опубликовало эту надпись здесь
                                                                          0

                                                                          Как не вспоминать, если вот же — на код-ревью прислали неотформатированный код. И потом пришлют.

                                                            +2

                                                            Статья оставляет двойственное ощущение. С одной стороны, написаны правильные и интересные вещи. С другой, я вижу некие внутренние противоречия, и это снижает доверие к статье в целом:


                                                            1. С одной стороны, "стиль и синтаксис не имеют ничего общего с проблемой, которую изначально решает разработчик". С другой, обсуждается комментарий с пробелами — что является следование стилю. Так соблюдение стиля в коде PR — это допустимо или нет?
                                                            2. Несколько раз озвучивается тезис, что главный критерий — решает ли PR задачу, поставленную перед исполнителем. С другой стороны, "Полезное поведение № 5: Используйте возможности для обучения" говорит, что даже если задача решена, стоит комментировать для обучения другого человека. Когда он этого не просил. Так, чтобы не вызывать у него негатива. Я не говорю, что так сделать невозможно. Но все-таки, автор, в чем цель код-ревью?
                                                            3. Может, я делаю неправильные выводы из "Полезное поведение № 10: установите стандарт, пока команда мала и растёт". Но прям слышу продолжение "… и не поощряйте изменения стандарта, когда команда выросла". Что вы будете делать, если проекту 3-5-10 лет, а актуальные стандарты уже другие?
                                                              • Писать новый код по новым стандартам, старый оставлять и мучиться с двоестандартием?
                                                              • Убеждать собственников потратить неопределенное количество сил на переписывание старого работающего по новому?
                                                              • Требовать писать новый код по-старому, и решать проблемы с недовольством команды?
                                                              +1
                                                              Ощущения от прочтения: чем дальше в индустрию, тем больше правят массовики-затейники, а не инженеры. Общий смысл в том, что типа не дай бог нарушить зону комфорта нашего васеньки… Не вздумай ничего сказать прямо, пусть он сам догадается, а жёлтую карточку покажет система, человек не может, потому что мы же ж одна команда, зачем всё портить…
                                                              «Будьте осторожны, чтобы не выказать удивление, если человек чего-то не знает.»
                                                              — я однажды натурально язык от удивления проглотил, когда узнал, что коллега js-ник после трёх лет работы на работе не знал конструкции const self = this. Как показала дальнейшая практика взаимодействия, пинка ему надо было выдавать как можно скорее. К сожалению, тогда в компании вот так же носились с сотрудниками в духе «не надо, просто подождём, люди ведь хорошие и учатся», что в итоге просто закончилось бесконечным исправлением косяков и переработками тех людей, которые умеют работать и вынуждены убирать говно за такими васеньками…

                                                              По-моему, достаточно просто базовой рабочей этики, вроде избегания конфликтов, стремления быть конструктивным и так далее. Прирождённых интриганов все эти ваши правила не остановят: они найдут способ ущипнуть. Зато иногда драматически удлиняется путь коммуникации между сотрудниками, люди думают про тонны словесного поноса, который они вынуждены писать, чтобы донести простую мысль.
                                                                0
                                                                Добавлю свои пять копеек. То что встречается в работе, то что вспомнилось.
                                                                • слишком долгий ответ на pull request. Что имеется в виду: один из участников ревью слишком долго не реагирует, не ставит approve и не пишет замечаний. Но код из этого пулл река кому-то срочно нужен. В итоге либо получается простой в работе, либо утверждение запроса происходит не дожидаясь кого-то из участников.
                                                                • Легкомысленное принятие запроса. То есть ставим approve не просмотрев код
                                                                • Игнорирование замечаний. Автор, не смотря на наличие замечаний сам завершает pull request. Или просто сам создает, сам апрувит, не дожидаясь никого

                                                                PS: понимаю что часть проблем решается настройкой политики проведения PR
                                                                  –4
                                                                  Код-ревью еще та гадость. Тот кто это придумал и поддерживает нуднейший человек. Устроили «бюрократию» их хобби программировать, то только отвращение от работы вызывает. Программирование само по себе испепеляющая и нудная вещь, так её еще и корпоративным адом испортили. Причем ревью делают с целью самоутвердиться, так как у кодеров больше всего комплексов, которые сублимируют из всех профессий. И выходит так якровыраженный синдром вахтера, особенно у тим-лидов. Хуже только у рекламщиков и дизайнеров делающих рекламу, там поголовно у всем чувство величия и комплекс бога. Это еще принято запивать алкоголем и заедать жирным от осознания собственной несчастности.

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

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