Код-ревью частенько порождают споры. При подготовке лекции «Отучаемся от токсичного поведения на код-ревью» на конференции 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: понять, что вы можете быть частью проблемы
Чтобы создать более благоприятную среду, важно быть честным с собой и размышлять над любым неэффективным поведением.
Будучи джуниором, я несколько раз замечала ошибку в чьём-то коде и радовалась, ведь я могла оставить комментарий! Теперь я понимаю, что использовала такую возможность, чтобы хвастаться, а не помогать. Нужно внимательно оценивать свои действия.
Думаю, что каждому полезно уделить время этому трудному самоанализу.
И последнее…
Мы говорим не о содержании отзывов, а только о тоне
Знаю, что отзывы важны, и мы тут не воюем против них. Я не прошу ставить под угрозу качество кода. Доброжелательная культура код-ревью и высокое качество кода не должны быть взаимоисключающими.
Просто надеюсь, что люди предпримут шаги, чтобы обеспечить конструктивную, действенную обратную связь и создать более благоприятные условия, в которой разработчики будут чувствовать себя комфортно, будут учиться, расти и не бояться совершать ошибки. Все мы можем совершенствоваться вместе.
Комментарии (73)
aamonster
29.05.2019 12:56На КДПВ всё наоборот. Заставлять человека читать кучу воды вместо того, чтобы просто указать ошибку – неуважение (исключение – когда ты сам влез к этому человеку с информацией об ошибке, никто тебя не просил о code review).
Pand5461
29.05.2019 14:03+1Воды много, но лучше написать "гайдлайн X не выполняется, перепроверить", чем тыкать в каждое место отдельно. Хотя бы во избежание "мне написали поправить А, Б и В — их я и поправил, а Г и Д тупой злобный ревьюер не заметил".
aamonster
29.05.2019 14:31Согласен, указывать одно и то же много раз — тоже плохо. Хуже было бы только на каждую строчку с ошибкой написать по две строчки оправданий за то, что на неё указываешь.
yatagarasu
29.05.2019 18:54+2чем тыкать в каждое место отдельно
Практика показывает что приходится тыкать, иначе люди делают удивленное лицо и говорят, а что вроде всё по гайдлану.
Вообще всё зависит от ревьювера, кому-то не лень откомментить каждый пробел, кому-то лень.
Другое дело это личные проблемы восприятия — надо быть закомплексованным человеком, чтобы воспрнимать коммент под каждым пробелом как давление на своё ЧСВ.
Вообще воспринимать всё в рамках ЧСВ и токсчиности — вредно. Ты не знаешь с кем этот человек общался до этого, и может ему уже надоело весь день отвечать на глупые вопросы а хочется уже поработать.
Всегда надо оценивать компетентность ответа который вам дали и не бояятся задавать вопросы, а по ответам можно будет уже оценивать и свою компетентность, и профессионально расти.Pand5461
29.05.2019 20:57Я как-то предпочитаю "явное лучше неявного" — т.е. лучше сказать, что есть нарушения каких-то практик, в частности, в точках А, Б и В, но я не гарантирую, что это исчерпывающий список.
Вообще воспринимать всё в рамках ЧСВ и токсчиности — вредно.
Да ладно, учитывая пол и национальность автора оригинала — у неё ещё до фига взвешенная точка зрения.
Всегда надо оценивать компетентность ответа который вам дали и не бояться задавать вопросы, а по ответам можно будет уже оценивать и свою компетентность, и профессионально расти.
Мега-труЪ. Особенно про "не бояться задавать вопросы".
plin2s
30.05.2019 10:52Спорный момент про дублирование комментариев к однотипным ошибкам. Бывают ситуации, когда автор совершенно некомпетентен и корректнее сразу указать на конкретные ошибки, нежели давать расплывчатый комментарий "где-то там, что-то там неверно" и потом еще десяток раз разворачивать, потому что либо не было исправлено вообще, либо не целиком.
Правило неплохое, но требует определенного контекста и уровня подготовкиkinall
30.05.2019 11:24Тут как раз просто. В первый раз даём общее замечание, если пришло второй раз — ставим метки везде, где эта ошибка есть, пришло в третий — издеваемся «бип»-ами. Если ничего не помогло, идём к руководству с вопросом о несответствии автора занимаемой должности.
pav5000
29.05.2019 15:47Такие вещи, как на КДПВ вообще по-хорошему должны проверяться линтером на пре-коммит хуке и не давать класть такое в репозиторий.
kinall
29.05.2019 14:01+1Категорически согласен по поводу примера про «бип» — встречал подобное, хтелось просто убивать за такие «бипы». Ощущение самоутвеждения за мой счёт. Скажи один раз — мол, дружище, у тебя есть систематическая проблема, пробегись по коду. Не-е-ет, надо каждый раз носом натыкать, ЧСВ своё почесать!.. Извините, наболело)
А вот по поводу того, что вопрос «Почему вы не использовали ___?» некорректен — ак же категорически не согласен. Этот вопрос означает, что ревьюер НЕ считает себя всезнающим, признаёт, что его мнение не бсолютно, и интересуется чужим. Может быть, он просто чего-то не понял, и применять его любимый ___ здесь просто нельзя?
andreysmind
30.05.2019 12:23-1А вот по поводу того, что вопрос «Почему вы не использовали ___?» некорректен — ак же категорически не согласен. Этот вопрос означает, что ревьюер НЕ считает себя всезнающим, признаёт, что его мнение не бсолютно, и интересуется чужим. Может быть, он просто чего-то не понял, и применять его любимый ___ здесь просто нельзя?
В момем опыте чаще были примеры, когда ревьюер задает такой вопрос по причинам, которые не относятся к коду. Например если других вопросов нет, но хочется докопаться, чтобы человек не подумал, что у него все хорошо, а тут целый простор для дискуссии и морального доминирования.
Если человеку интересно чужое мнение, то он может его и в личной беседе спросить, не засоряя ПР.
mafia8
30.05.2019 15:21Здесь вопрос может подразумевать, что программист знает и обязан (?) был сделать так.
… может быть, ревьюер просто не понял, что иногда решения бывают малоосознанными, человек не может так сказать, почему он так сделал, особенно почему он что-то НЕ сделал, например, не перенес что-то в отдельный файл, если на это не было команды.
Интервьюер ожидает не того, что программист только ответит на вопрос, а перенесет код в отдельный файл.
Дурость.
Спросили — ответил. Сказали сделать — сделал.
Не смешивать.
solver
29.05.2019 14:16Статья большей частью выглядит как «Передача мнения как факта» )
Далеко не всегда надо все это соблюдать. Много частных случаев.
В моей практике были люди которые просто не понимали нормального отношения.
Им бесполезно было говорить «у тебя системная проблема».
Поэтому им и бикали и пикали и чего только не делали. А уволить их было невозможно…Danik-ik
30.05.2019 10:56-1Ваше мнение о статье не совпадает с моим.
Во-первых, любая статья — это всегда мнение. За фактами — это к криминалистами, да и там не сто процентов.
Во-вторых автор нигде не говорит о том, что его мнение — единственно верное. Более того, цитирую:
Просто надеюсь, что люди предпримут шаги, чтобы обеспечить конструктивную, действенную обратную связь и создать более благоприятные условия, в которой разработчики будут чувствовать себя комфортно, будут учиться, расти и не бояться совершать ошибки. Все мы можем совершенствоваться вместе.
Я здесь вижу: "люди, старайтесь быть людьми к всеобщей пользе". При этом нигде в тексте не нашёл "Кто делает не так, тот редиска" или "Предложенные методы являются универсальной таблеткой и применимы без условий и ограничений". Поэтому лично у меня заявление о "Передаче мнения как факта" вызывает лёгкое недоумение.
P.s. Не являюсь экспертом. Не претендую на истину в последней инстанции. Вы имеете право на мнение, отличное от моего.
vladbarcelo
29.05.2019 15:02Не просите разработчика исправлять проблемы «пока они здесь». Если код делает что требуется в тикете и не вводит новых проблем в кодовую базу, одобрите его, а затем создайте билет для очистки кодовой базы.
Плохая идея. Такие todo откладываются в долгий ящик в 99% процентах случаев, а призывы "ну давайте уже почистим наш код" отвергаются как начальством ("у вас это в спринт не входит, сделаете эту таску потом, когда завершите ещё n спринтов с мега-срочными фичами"), так и другими программистами ("у меня и так в жире куча незакрытых тасок висит, у меня нет времени").
abbath0767
29.05.2019 17:05Почему бы подразумевать по умолчанию исправление некоторого техдолга/рефакторинг в стандартный план? Точно так же как и тесты, кодревью и все остальное?
Я понимаю, что не везде это возможно по тем или иным причинам, но на мой взгляд стремится к этому уже будет хорошо. Хотя бы для себяvladbarcelo
29.05.2019 17:36Потому что на любом митинге идея встроить в план исправление техдолга с удовольствием примется и отложится в долгий ящик по всё тем же причинам. Стремиться к этому, в свою очередь, можно бесконечно.
Peter03
29.05.2019 17:41У нас например два-три раза в года делается тех-спринт. т.е. целый спринт выделяется на реализацию того что команда решила сделать для поддержания проекта в правильной форме и тикеты бизнеса в это время не принимаются.
balexa
30.05.2019 11:45С одной стороны да. С другой стороны, меня всегда очень бесили комментарии на ревью, когда фикся конкретную очень проблему в файле с говнокодом, тебе предлагают взять и отрефакторить чуть ли не целиком весь этот файл (и полпроекта заодно).
Должна быть золотая середина. Простой фикс можно и сделать «заодно».
Но если этот todo очень простой — ревьюверу тоже ничего не стоит пофиксить этот туду прямо во время просмотра. Странно что у него есть время на изучение проблемы, написание комментария и нет, не было и не будет времени на фикс.
Если же код сложный, на туду всем в 99% случаев плевать, времени на рефакторинг нет, начальству все равно на ваш техдолг, другим программистам тем более, то это конечно проблема. Но эта проблема явно не должна решаться за счет человека (возможно новичка в команде, или человека который занимается другими частями кода), которому не повезло влезть в этот код и пригласить на вас ревью
Хотя бы потому, что у него на этот фикс уйдет время. А как вы заметили, в таске висят мега-срочные фичи, и начальство не поймет, почему этот фикс делается так долго (по вашим же словам). А значит в следующий раз человек возможно не будет исправлять проблему в корне. А навернет сверху костыль из спагетти. Код станет еще хуже, зато мега-важный тикет с фичей закроется и этого ревьювера из платформы уже система не предложит.murad_mms
01.06.2019 11:02Например, меняет человек 1 строчку в инициализации класса — новый параметр константный.
А параметров там штук 5 и инициализируется он в 5 методах по всему файлу (и везде константы на месте объявлены). Я прошу исправить это, по факту ему придется весь файл переделать и + тесты.
testopatolog
29.05.2019 16:26Автор, спасибо, а как Вам такое Предложение:
Ревью кода — это отдельная задача, которую ставит Руководитель своему подчинённому, затем контролирует её исполнение.
По завершению задачи Руководитель просматривает выполненное review, сглаживает «острые углы» в его комментах (поправляет, если компетенции хватает), затем, передаёт его на доработку создателю кода.
Согласитесь, что Review-эксперту без отладчика и так сложно глянуть и найти баги синхронизации, утечки памяти… а без погружения в прикладную/бизнесовую часть задачи сложно выявить ошибки логики или определения инфоструктур, тем более, проблем производительности, безопасности… Если он это находит и едко прокомментирует, — Руководитель, скажите ему только спасибо, скорректируйте сами этот комментарий, (он Вам в ответ тоже скажет спасибо, а то, возможно, уже нервов на нерадивого коллегу не хватает).
Главное в этом подходе:
1. Открытые эмоции коллег — это выпуск «пара», искренние отношения полезные для дела, а для Руководителя — маркеры для формирования совместимых команд и оценки профессионального уровня отдельных специалистов, выполнение своей задачи обеспечения благоприятной рабочей среды для достижения результата.
2. Далее Review-эксперт не будет видеть смысла писать токсичные тексты, понимая, что его адресат не прочитает колких обращений, благодаря их корректировке Руководителем.
Примечание: извините, что из-за этого Предложения Ваш список правил становится не очень актуальным.
mapron
30.05.2019 10:10Предложение замечательное, окромя одного — в реальности (чаще всего) никаких Руководителей на ревью не напасешься. У нас есть тех дир, его конечно можно добавлять к ревью, но в реале он хорошо если в 1% случаев его просмотрит (просто тупо нет времени). Что там руководителей, у меня вообще просто в принципе ревьюера найти сейчас проблема (нужно править код по другой технологии, а ребята из другого отдела забиты работой по самое не могу, в итоге задержки по 1-2 недели на этом).
Я не встречал еще фирмы где есть такой избыток времени руководителей, если у вас оно есть — я безмерно рад за вас)
Dolios
29.05.2019 16:47Если вы объединяете код, не ответив на все замечания, то это вызывает удивление у человека
Нет, это вызывает волшебный пендель, т.к. в нормальной ситуации код нельзя мержить, пока ревьювер не отметил все замечания, как разрешенные.
Избегайте задавать осуждающие вопросы вроде такого:
Это не осуждающий вопрос. Если мне правда интересно, почему сделано А вместо Б, мне что делать?Spunreal
30.05.2019 22:32Когда мне задают вопрос "почему вы не использовали ___?", у меня возникает только одна ассоциация — лошадь в ванне с огурцами.
Потому что я просто не вспомнил о ? Не знал о ? Не знаю почему не использовал ___?
Это не сколько осуждающий вопрос, сколько бесполезный, на который не понятно как отвечать.
Dolios
30.05.2019 22:43+1Ну так и отвечайте, что не знали, не вспомнили и т.д. А если знали, но не использовали по какой-то известной вам, но неизвестной мне причине, назовите причину.
Зачем все усложнять?
Rukis
31.05.2019 18:31Часто задаю такого рода вопросы. Исполнитель заведомо имеет более глубокое погружение в задачу, возможно он всё учел и осознано не использовал наиболее, как мне кажется, очевидное или правильное решение.
Таким образом, если исполнитель не знал или что-то забыл, то теперь он узнал/вспомнил, а если у него есть аргументированное объяснение, то, возможно, что-то полезное узнаю я.
Peter03
29.05.2019 16:53Мне кажется code review система должна поддерживать некие формальные оценки в дополнение к комментарию.
Ревьюер назначает приоритет комментарию:
1. Critical
2. High
3. Medium
4. Low
Автор кода для каждого комментария устанавливает предполагаемое действие
1. Create ticket in the next release
2. Add TODO comment
3. Reject (with comment to a reviewer)
4. Request arbitrament
5. Ignore
И при оценке более чем одним ревьюером, можно ставить плюс или минус чужому комментарию.
В противном случает иногда трудно понять что на самом деле имел ревьюер в виду.
Я заметила, что рецензенты иногда указывают каждый из случаев вместо того, чтобы оставлять одну подробную заметку со ссылками на полезные ресурсы.
Надо чтобы система поддерживала подобную функциональность, т.е. ссылку на один общий комментарий. Иначе автор изменений может воспринять результаты ревью дословно и сделать изменения только там где указано. Плюс иметь возможность сослаться на произвольный комментарий из предыдущих ревью.Peter03
29.05.2019 17:02И что меня особенно бесит — это когда одни и те же изменения в коде отправляются на повторное ревью несколько раз. т.е. приходится побираться через уже просмотренные изменения заново. Убивает всё желание делать ревью.
Надо чтобы инструмент поддерживал подобные сценарии. Т.е. некий diff с предыдущего ревью (может новые изменения другим цветом?) и возможность импортировать предыдущие комментарии там где ничего не было сделано.
testopatolog
29.05.2019 17:58code review система должна поддерживать некие формальные оценки в дополнение к комментарию
Согласен, также предлагаю к комментарию Review-эксперту добавлять оценку времени, необходимого, по его мнению, для устранения замечания из комментария.
APXEOLOG
29.05.2019 19:49+1Имхо все проблемы решает вежливость. Достаточно написать "Please change X" или "Please consider X instead" и на этом все вопросы токсичности можно закрывать
monah_tuk
30.05.2019 05:41По КДПВ: есть коллеги, с которыми второй вариант не работает, они реагируют только на прямые указания, потому как в ревью борде это дефекты и их нужно закрыть. А просто просмотреть самостоятельно, хотя бы тот же файл, на предмет аналогичных ошибок… никак.
warranty_voider
30.05.2019 13:02ну вещи типа висящих пробелов и расстановки скобок вообще не должны всплывать на ревью, т.к. это работа линтера, а не разработчика. Настройте один раз подходящий инструмент и живите спокойно
Dolios
30.05.2019 22:45Видимо, есть очень богатые компании, которые постоянно тратят время дорогих разработчиков впустую, вместо того, чтобы один раз настроить гитхук.
Avitale
30.05.2019 10:45-1Происходит сбой при вводе отрицательного числа. Не могли бы вы этим заняться, если не сложно?
Меня вот это зацепило. Если в коде происходит сбой, то о каком «если не сложно» может идти речь? Это должно быть исправлено.balexa
30.05.2019 12:19+2Это обычная вежливость, не более. Примерно как «извините, вы не могли бы подвинуть свою машину от проезда». Если машина загораживает проезд — ее очевидно нужно подвинуть. Но очевидно, на фразу «машину с проезда убрал, быстро» вы среагируете скорее всего по другому. Хотя смысл один и тот же.
Avitale
30.05.2019 12:29Да, но можно же сказать: «Поправь этот баг, пожалуйста». Я к тому, что постановка фразы подразумевает, будто у человека есть возможность сказать «нет». Зачем это все, если мы оба знаем, что баг придется поправить вне зависимости от того, сложно человеку или нет?
balexa
30.05.2019 12:32Ну вот привычка такая у человека, стиль речи. «Не могли бы вы переставить машину, если не сложно, а то мы выехать не можем.». Да, это не самая лучшая формулировка.
Но это опять же, обычная вежливость, цепляться за это странно, как по мне.
fshp
31.05.2019 12:53Это дословный перевод с английского. В англоязычных странах повелительное наклонение считается чем-то оскорбительным. Никто не может указывать свободному человеку что ему делать.
Поэтому не могли бы вы вытащить этот нож из моего черепа, пожалуйста? Это доставляет мне дискомфорт, т.к. кровь заливает мне глаза. Как у вас будет свободное время конечно же.
o5a
31.05.2019 15:00Это уже особенности перевода и особенности их речи.
В оригиналеThis breaks when you enter a negative number. Can you please address this case?
В нашем переводе как раз и получается «Исправьте ошибку, пожалуйста». Кстати автор перевода этот момент с «пожалуйста» уже исправил.
То, что использовано «Can you please address this case?» вместо прямого «Address this case.», то это как уже упомянули, особенность их речи, не принято в повелительном наклонении выражаться.Avitale
31.05.2019 15:53Все понятно, спасибо за разъяснение. В оригинале действительно нет этого «если не сложно», которое меня и зацепило. К отсутствию повелительного наклонения у меня претензий не было, в английском оно действительно предпочтительней.
pop70
30.05.2019 11:50ИМХО, есть одно простое правило на все случаи жизни:
"Поступай с другими так, как хочешь, чтобы поступали с тобой".
Все токсичные практики (и не только в it) — всегда пример обратного.
nsinreal
30.05.2019 12:57Это правило не так-то хорошо работает, когда на одном конце провода незакомплексованный человек, а на другом конце провода — человек, который находится в состоянии стресса (или просто не имел конструктивного опыта взаимодействия с критикой).
Много проблем происходит из-за того, что люди разные и не понимают, что их поведение вызывает негативные ощущения у других.
Dolios
31.05.2019 07:47+1Ну, я вот считаю, что, якобы, «токсичный» вариант из КДПВ, это коротко и по делу, а «вежливый» — это бессмысленная трата времени обоих сотрудников.
И, получив подобное ревью, даже не подумал бы, что оно, оказывается, «токсичное».
vintage
30.05.2019 13:26
И хоть бы слово о том, что не надо вообще обращать внимание на такие мелочи.
ivanych
30.05.2019 15:43Надо. Потому что следующий разработчик поправит в этом файле одну строку, прогонит файл через стандартный форматировщик и привет — в файле изменится сто строк. Разбирайся потом, какие изменения относятся к реально сделанной задаче, а какие просто зацепило форматировщиком.
vintage
30.05.2019 15:55+1поправит в этом файле одну строку, прогонит файл через стандартный форматировщик
Вот за это и стоит бить по рукам, а не докапываться до каждого пробела.
ivanych
30.05.2019 18:57За что "это"? За форматирование кода в соответствии со стандартами?
balexa
30.05.2019 19:47За то, что он не разделяет коммиты и валит все в одну кучу.
В вашем кейзе разработчик должен сделать два коммита — первый это «поправил одну строку», второй — «прогнал через стандартный форматировщик».ivanych
30.05.2019 19:51Разделение на комиты не играет тут никакой роли. В итоговом мерж-реквесте (который и будет рассматриваться на код-ревью) все изменения будут отражаться одновременно.
balexa
30.05.2019 20:59Разделение на коммиты здесь принципиально, ибо эти коммиты возможно придётся читать потом кому-то через несколько лет, разбирая что там было сделано. В целом каждый коммит должен быть атомарен, не надо в одну кучу валить фиксы багов, новые фичи и автозамену форматирования.
Если ваш сервис пулреквестов не позволяет разделять коммиты, то замените его либо делайте разные пулреквесты, не надо оправдывать плохие практики тем, что ваш труд якобы не позволяет делать нормально.
ivanych
30.05.2019 22:12Вы зачем-то свалили в кучу кодревью (на котором смотрят конечное состояние кода, независимо от того, какие там были промежуточные коммиты) и просмотр отдельного коммита (в некотором будщем).
Не помешает разделить непосредственно исправление кода и форматирование на разные коммиты — это может пригодиться в будущем, как Вы верно заметили.
Однако на код-ревью таковое разделение не повлияет вообще никак. На код-ревью смотрят весь конечный дифф и в нём будут отображены одновременно и исправление кода и форматирование.
vintage
30.05.2019 19:53-1Не просто коммиты. Это должны быть разные пулреквесты. Причём второй нужен лишь в совсем запущенных случаях типа "все стили в одну строку", а не по любому чиху.
ads83
30.05.2019 21:24Если я правильно вас понял, то вы предлагаете
- Oставить trailing spaces без внимания, т.к. это мелочь которую не надо замечать. Пусть она и нарушает code style
- Запретить этому и любому другому разработчику исправлять пробелы в PR с полезной нагрузкой. И бить по рукам тем, кто так сделает.
- Приводить оформление к оговоренному стилю только когда в файле ужас-ужас, но не раньше, когда просто ужас.
Я правильно вас понял?
Я с таким подходом никогда не сталкивался, и (теоретически) это провоцирует к отказу разработчиков применения авто-форматирования в IDE, ведь из-за автоформата придется проверять не сбились ли пробелы с "как было" на "как надо". Это, в свою очередь, приведет к постепенному нарастанию кома кода вне оговоренного стиля.
Если у вас есть опыт работы команды с таким подходом, поделитесь пожалуйста, насколько эти проблемы реальны? И какого уровня разработчики были в команде?vintage
31.05.2019 12:42Какой ужас. Код с лишними пробелами ведь просто невозможно читать. Развели тут карго-культ на тему код-стайла.
Есть опыт работы без линтеров и форматтеров — ничего страшного с кодом не происходит. Если кто-то написал непонятный код — обсуждаем как написать его понятней. И расположение пробелов — самое последнее, что влияет на ясность кода.
ads83
31.05.2019 14:09Дело не в пробелах. Это малая часть code style. И дело не в карго-культе. Из ваших ответов я делаю вывод, что вы не считаете важным причины, по которым возникло понятие единого стиля кодирования для команды.
Возможно, вам не встречались коллеги, которые пишут цикл в одну строку,else ifразносят на 3 строки и ленятся настроить форматтер в IDE, и тем более его применять. Возможно, вы не понимаете, насколько замедляют чтение и понимание такие нестандартные куски кода.
И вероятно, вы не получали по шапке от тимлида за смешивание в одном PR новый код и форматирование, когда приводите такие куски к удобочитаемому формату. Вы из тех, кто ругает за улучшение общей кодовой базы.vintage
31.05.2019 14:31-1Возможно, вам не встречались коллеги, которые
Вот, вы сами пишите, что проблема в коллегах, а не "единстве стиля". Если коллега пишет говнокод, то надо либо объяснить ему что не так (или он объяснит вам что всё так), либо перестать с ним работать, если он не понимает разумных тезисов.
вы не получали по шапке от тимлида
Я никогда не даю по шапке за такие глупости. Го к нам работать.)
ads83
31.05.2019 15:57Я все меньше понимаю вас.
Вот за это и стоит бить по рукам, а не докапываться до каждого пробела
совсем не вяжется с
Я никогда не даю по шапке за такие глупости
Не бьете, потому что не дают? Использовали сильную фигуру речи, хотя так не считаете?
Это должны быть разные пулреквесты. Причём второй нужен лишь в совсем запущенных случаях типа "все стили в одну строку"
Так все-таки это глупости, на которые вы не обращаете внимания, или вещи, достойные отдельного пулл-реквеста?
vintage
31.05.2019 16:04Для изменения одной строчки не надо изменять весь файл, какой бы благородный мотив ни был. Исключение — легаси-говнокод — от него надо избавляться как можно быстрее.
А вот эту строчку форматировать можно по разному. И как именно — не особо принципиально в разумных пределах.
babylon
31.05.2019 14:12Чем выше уровень разработчика тем меньше волнует пробельная тема если конечно пробелы не ломают xml ))). Бывает экран маленький и наличие лишнего пробела не заметно.
ads83
31.05.2019 15:50Именно поэтому я спрашивал, какого уровня была команда. Потому что опыт работы в all senjor команде неправильно распространять на другие, более распространенные случаи.
И да, меня лично не напрягают ни пробелы в конце строки, ни табуляции. Но я понимаю, что в разноуровневой (и по мастерству, и по желанию считаться с другими) команде хорошо бы иметь правила, которым следуют все.
ivanych
30.05.2019 22:29Нет.
Когда разработчик пишет свой код, он должен сразу привести его к стандартному форматированию, чтобы не увеличивать хаос. Для этого он должен прогнать файл через форматировщик. Но форматировщик при этом внесет исправления в чужой код, который ранее не был отформатирован.
Если же разработчик не будет прогонять файл через форматировщик, чтобы не менять чужой код, то это означает, что он и свой код не отформатирует.
Надежда на то, что форматирование и своего и чужого кода будет исправлено в другом мерж-реквесте, не обоснована. Потому что пока готовится это исправление, дугие разработчики уже внесут новые правки (а мы помним, да, что при этом они не форматируют код как надо, тоже планируя отформатировать его потом?). Конечный-исправляющий-все-косяки мерж-реквест не будет сделан никогда.
ads83
30.05.2019 21:52+1Статья оставляет двойственное ощущение. С одной стороны, написаны правильные и интересные вещи. С другой, я вижу некие внутренние противоречия, и это снижает доверие к статье в целом:
- С одной стороны, "стиль и синтаксис не имеют ничего общего с проблемой, которую изначально решает разработчик". С другой, обсуждается комментарий с пробелами — что является следование стилю. Так соблюдение стиля в коде PR — это допустимо или нет?
- Несколько раз озвучивается тезис, что главный критерий — решает ли PR задачу, поставленную перед исполнителем. С другой стороны, "Полезное поведение № 5: Используйте возможности для обучения" говорит, что даже если задача решена, стоит комментировать для обучения другого человека. Когда он этого не просил. Так, чтобы не вызывать у него негатива. Я не говорю, что так сделать невозможно. Но все-таки, автор, в чем цель код-ревью?
- Может, я делаю неправильные выводы из "Полезное поведение № 10: установите стандарт, пока команда мала и растёт". Но прям слышу продолжение "… и не поощряйте изменения стандарта, когда команда выросла". Что вы будете делать, если проекту 3-5-10 лет, а актуальные стандарты уже другие?
- Писать новый код по новым стандартам, старый оставлять и мучиться с двоестандартием?
- Убеждать собственников потратить неопределенное количество сил на переписывание старого работающего по новому?
- Требовать писать новый код по-старому, и решать проблемы с недовольством команды?
bm13kk
Все примеры должны быть автоматизированы и частью статического анализа