Здесь описаны мои исследования, как сделать ревизию кода в команде более приятным занятием, которое может дать новый опыт всем участникам. У нас полностью географически распределённая команда, все коммуникации выполняются через интернет, и зачастую асинхронно. Мы используем Trello для описания возможностей продуктов, поодиночке создаём код, отправляем в GitHub пулл-реквесты, а также пользуемся встроенной в GitHub функцией их ревью. Это отличается от просмотра кода лицом к лицу в офисе и даже по видеочату.
Если не подходить к делу всерьёз, то асинхронная и письменная ревизия кода может стать причиной катастрофы в команде, приведя к ухудшению взаимодействия и сотрудничества. Но если все участники будут стараться делать всё хорошо, то такой подход может работать очень эффективно.
Для чего нужно ревью кода?
Прежде чем начать рассуждать о том, как делать и как не делать ревью, полезно подумать о том, для кого в компании оно нужно, и в соответствии с этим вырабатывать требования. И хорошо бы не забывать, что ревью — это не просто поиск багов и проблем в структуре кода.
Ревью нужно для улучшения качества кода и морального духа в команде.
Анализ кода друг друга — один из главных путей взаимодействия между разработчиками во время обычного рабочего дня. Это должен быть вдохновляющий процесс, чтобы все участники ждали его и активно участвовали в нём.
Вот основные задачи, решаемые с помощью ревизий:
- Создание внутренней культуры команды. Ревью — это один из основных процессов, который подразумевает наличие командной культуры. И если участникам неприятно участвовать в ревью, значит внятной командной культуры нет. Ревизии должны создавать позитивную атмосферу, терпимость и дружественность.
- Улучшение рабочих взаимоотношений благодаря возможности чаще разговаривать с коллегами.
- Снятие напряжённости в отношениях с помощью положительных отзывов о чужом коде.
- Обучение. Проводящий ревью может узнать для себя новые вещи из чужого кода, а автор — из отзыва.
- Выход за границы своей зоны ответственности, позволяющий всем участникам команды анализировать разные части кода, ознакомляясь со всей кодовой базой.
- Наставничество и образование. Ревью — это возможность для более опытных участников стать наставниками и обучать других. Но имейте в виду, что, поскольку время уже прошло и код написан, ревью — не всегда лучшее время для обучения. Разработчики должны совместно работать над техническим проектом как до, так и во время создания кода.
- Качество кода. И наконец, качество (включая баги, удобство в сопровождении, документацию, организацию, архитектуру, удобство использования продукта…).
Что не надо делать при ревью
Возможно, эта часть покажется вам довольно негативной, но зато после неё идёт часть с приятными вещами!
Есть много разных способов сделать ревью кода неудачным. Воинственные, злые и непредсказуемые комментарии способны лишить команду удовольствия от работы и превратить ревизию в эмоционально тяжёлую обязанность.
Понимание, чего следует избегать при ревью, как раз и отличает ценную часть работы команды от жестокого и неприятного наказания. Эрик Дитрих, «Чего нужно избегать при ревью кода»
Не проводите ревью формально и с минимальными комментариями
(aka узнать, насколько плохо думают о вас коллеги)
Не нужно всего лишь указывать на ошибки, ничего к этому не добавляя.
Разработчики тратят много времени на код и гордятся им, и ревью — их шанс продемонстрировать свою работу. А при формально-минималистичном подходе всё, на что вы можете надеяться, — это комментарии в стиле «Выглядит неплохо, смерджил». Такое ревью достойно названия «Узнай, насколько плохо о тебе думают».
Невнятное и непродуманное общение может привести к долговременным неприятным последствиям. Командная культура испортится, что снизит продуктивность. При этом разработчики часто получают отзывы вроде «похоже, эта порка придумана специально для того, чтобы выбить из них дух», а сами ревизии превращаются в «ментальные соревнования, в которых люди стреляют по цели… а цель — это разработчик, написавший код».
Не думайте, что достаточно лишь «критиковать код, а не кодера»
Это один из самых популярных советов: критикуйте код, а не кодера. Он хорош (если вы критикуете коллегу, а не его код, то у вас проблемы). Но этого недостаточно. Написание кода — творческий процесс, в который разработчик вкладывает сердце и душу. И если вы жестоко и тупо критикуете чей-то код, то тем самым вы критикуете самого человека. Нужно уметь критиковать. Найдите более позитивные способы анализа и вносите свои предложения, как улучшить код.
Следите за тоном
Не переходите на личности. Фразы вроде «Почему ты сделал именно так?» воспринимаются как нападки. Плохой вариант: «То, как ты написал эту функцию, затрудняет её чтение, добавь больше комментариев» (личное обращение подразумевает, что человек что-то сделал не так). Лучше скажите: «Тебе не кажется, что стоит добавить в код дополнительные комментарии, это облегчит чтение функции?» (звучит гораздо корректнее и позволяет сосредоточиться на улучшении кода).
Обойдитесь без требовательных или вызывающих выражений. Избегайте фраз, смысл которых заключается в «ты неправ». Не говорите людям, что они неправы или что они сказали/сделали что-то не то, им будет неприятно. В таких случаях люди могут уходить в защиту, перестают долго и творчески работать и учиться. Избегайте фраз наподобие:
- «Так не сработает»
- «Совершенно неправильно»
- «Почему ты просто не … ?»
Не сгущайте краски. Вы же не хотите, чтобы на критику обижались за необоснованность или преувеличенность? Не говорите «всегда», «никогда», «бесконечно», «ничего».
Не надо оскорблений. Избегайте ругательств вроде «дурак» или «идиот», даже если лично вы не считаете их обидными. У подобных слов есть определённый спектр значений, и он точно не улучшит настроение автора кода.
Не используйте нетерпеливых или пассивно-агрессивных оборотов. Всегда будьте сдержанны и дружелюбны, избегайте неприятных фраз, демонстрирующих раздражение и заставляющих людей чувствовать, что они плохо справляются:
- «Ещё раз повторяю: здесь нужно…»
- «Как я уже говорил…»
Не подливайте масла в огонь. Человеку может быть неприятно получить от кого-то комментарий с критикой, а под ним ещё несколько с «+1» или нравоучениями по тому же поводу. К тому же многочисленная критика способна просто запутать.
Не будьте непрошеным советчиком
Сдерживайте себя и не предлагайте внести в код все изменения, которые вы хотели бы внести. Вы можете деморализовать автора кода, предложив поменять столько, что придётся всё переписать. Так что выбирайте только самое важное и лучшее.
Одна из задач ревью — найти и исправить баги и проблемы в структуре кода. А вовсе не заставить автора написать код так, как это сделали бы вы. Не забывайте, что в программном обеспечении многие вещи можно решить по-разному, в зависимости от вкусов разработчика, и у каждого подхода есть свои плюсы и минусы. Собираясь предложить очередную правку, спросите себя: может быть, это просто моё мнение? Если автор выбрал другие способы, обосновав свой выбор, то лучше поддержите его. Уважайте право автора на собственные решения.
Даже если кто-то написал код не так, как это сделали бы вы, то это ещё не означает, что код написан неправильно. Главное — качественный, удобный в сопровождении код. Если он удовлетворяет этим критериям и принятым в команде правилам, то этого достаточно. Роберт Бог, «Эффективные ревью без боли»
Вы никогда не сможете заставить других написать именно такой код, какой написали бы вы, не надо и пытаться (если вам это так важно, то лучше сразу напишите код сами). Эрик Дитрих, «Чего нужно избегать при ревью кода»
Не нарушайте правило «Никаких сюрпризов»
Избегайте неожиданностей. Создайте для команды общее задокументированное соглашение о pull request’ах и стандартах кода и во время ревью опирайтесь на него, а не на своё мнение.
Не говорите только для того, чтобы услышать свой голос
Прежде чем написать отзыв, подумайте, чего вы хотите этим достичь. Все ли ваши комментарии необходимы? Помните, что вы пытаетесь конструктивно помочь, а не просто высказать свою точку зрения. Прежде чем опубликовать отзыв, перечитайте свои комментарии и спросите себя, все ли они действительно полезны и важны? Лишние удалите. Лучше анализировать свои заметки после просмотра кода, постфактум. Пока вы разбираетесь с кодом, пишите сколько угодно комментариев, а потом спокойно просмотрите их и решите, что оставить.
Оставлять комментарии только для того, чтобы подчеркнуть свою правоту, не всегда полезно (или разумно). Иногда лучше держать своё мнение при себе, чтобы сохранить длительные хорошие отношения с человеком. Кэтрин Дэниелс, «Как давать и получать отзывы»
Вы не обязаны находить проблемы в ходе каждого ревью
Если код хорош и может быть влит в кодовую базу без изменений, то всё прекрасно!
Что надо делать при ревью
Ревью можно улучшить. Как именно? Ниже вы прочтёте предложения, собранные в интернете.
Хвалите хороший код
Не жалейте времени на то, чтобы похвалить сильные стороны кода, прежде чем указывать на недостатки и вносить предложения.
Человеческая природа такова, что нам нужно знать о своих успехах, а не только о неудачах. Разработка — это творческий процесс, в который люди вкладывают душу, они часто принимают многое близко к сердцу. Поэтому похвалы для них ещё более важны. Роберт Бог, «Эффективные ревью без боли»
Вы же хотите, чтобы участники команды воспринимали ревью как приятный и полезный опыт, а не как чистое критиканство. Положительные отзывы снижают напряжённость в отношениях и помогают людям лучше реагировать на критику.
Важно писать позитивные комментарии так, чтобы это не выглядело слащаво или фальшиво. Не должно создаваться впечатление, что вы просто стараетесь подсластить пилюлю.
Избегайте сомнительных комплиментов. Фразы вроде «Хорошая работа, но…» (дальше — список изменений, которые вы предлагаете сделать) выглядят наигранными.
Как сделать похвалы более честными?
- Оцените большинство хороших частей кода и другие аспекты и постарайтесь сказать, почему они вам понравились.
- Вникая в чужой код, оставляйте в комментариях свой «монолог на ходу». «Ага, понял, что это делает… Хорошо, подключается сюда и вызывает это, замечательно… а эта часть зависит от тех двух, отлично». Ход мыслей другого программиста, который разбирается в твоём коде, — ещё одна форма проверки работы. А когда тебе попадаются части кода, которые ты не понимаешь, можно попросить объяснить.
Сначала — положительные моменты
Сразу задайте настроение, в первую очередь рассказав о том, что вам понравилось. Теперь это легко можно сделать благодаря новым возможностям ревью кода на GitHub, создавая многострочные комментарии и публикуя их скопом (вместо публикации по мере сохранения), а также делая краткое резюме (отображается в начале отзыва) до внесения комментариев:
Избегайте неизбежных обвинений
Спрашивайте, а не говорите. Лучше задавать вопросы, а не делать заявления.
Заявление — это обвинение. Фраза «Здесь ты не соблюдал стандарты» — это обвинение, намеренное или нет. Задайте вопрос: «Почему ты использовал здесь такой подход?» Роберт Бог, «Эффективные ревью без боли»
Если задавать вопросы, то это улучшит общий настрой, восприятие ревью изменится благодаря приглашению к диалогу и обучению, а автор кода с удовольствием объяснит причину или поищет более удачный способ решения.
В идеале в ответ на корректный вопрос автор сам найдёт баг или предложит внести в код улучшения.
Если вы видите какие-то моменты, которые могут быть ошибками, не надо сразу говорить людям, что они неправы. Обычно достаточно спросить: «Что будет, если я передам этому методу ноль?», и человек наверняка сам скажет: «У меня были сомнения, исправлю». Позволить ему самому решить проблему и предложить улучшить код — гораздо лучше, чем давать указание по исправлению несовершенств. Эрик Дитрих, «Используйте ревью кода для казни»
Плохо:
- «Здесь ты не соблюдал стандарты»
- «А — неправильно, надо использовать В»
- «Запутанный код»
- «Ты не инициализировал эти переменные»
Хорошо:
- «Почему ты выбрал здесь такое решение?»
- «Почему ты использовал A вместо B?»
- «Эта часть мне непонятна. Можешь объяснить?»
- «Я не нашёл, где инициализируются эти переменные»
Избегайте обвинительного «почему». Как и утверждения, вопросы «почему» тоже могут звучать обвинительно, так что без них будет лучше.
Плохо:
- «Почему здесь ты не следовал стандартам?»
- «Почему ты просто не …?»
Хорошо:
- «По какой причине ты решил отойти здесь от стандартов?»
- «Чем ты руководствовался, когда …?»
Будьте скромнее
Задавайте вопросы, а не требуйте. Вместо того чтобы сказать автору, какие нужны изменения, задайте ему вопрос о коде или внесите предложение и спросите автора, что он думает по этому поводу. Так вы передаёте мяч на его сторону поля и выказываете уважение его свободе воли, давая автору шанс объяснить своё решение или высказаться, считает ли ваше предложение полезным для кода.
Вместо «Давай назовём эту переменную username, потому что иначе непонятно» лучше перефразировать предложение в виде вопроса: «Может быть, назвать её username? Текущее имя выглядит не слишком понятно для меня, потому что оно также используется в другом контексте в someotherfile.js.». Дэниел Бадер, «7 способов избежать ухудшения отношений при ревью кода»
Когда вы что-то предлагаете, обязательно объясняйте причину, по которой это изменение может улучшить код.
Используйте личные примеры. Покажите автору кода, что не только он совершал такую ошибку. Это отличный способ смягчить критику: «Мне самому это тяжело далось…», «Я делал то же самое».
Не на каждый вопрос нужно отвечать. Примите для себя сразу, что не на каждый ваш вопрос автор кода должен дать ответ. Так вы сможете задавать в своём отзыве вопросы, заставляющие задуматься. На них необязательно отвечать, чтобы влить код в кодовую базу, но зато они подстёгивают мысль разработчика и в долгосрочной перспективе повышают качество его работы.
Не нарушайте правило «Никаких сюрпризов»
Воспользуйтесь чек-листом:
Наверняка каждый участник вашей команды раз за разом совершает одни и те же десять ошибок. Причём труднее всего обнаружить пропуски каких-то элементов. Так что чек-лист — самый простой способ избежать наиболее распространённых ошибок и снизить вероятность пропусков. SmartBear, «Лучшие методики ревью кода»
Конечно, чек-лист не может покрыть все возможные случаи. Но в хороший список достаточно включить все требования, которым должен удовлетворять pull request, чтобы быть слитым с кодовой базой. Это полезный инструмент и для кодера, и для рецензента. Чек-лист может помочь улучшить код, сделать его более последовательным, избежать нарушения правил и стандартов. Новым участникам будет очень полезно законспектировать требования к коду, прежде чем отправлять свой первый pull request и проводить первую ревизию.
Пример чек-листа, с которого можно начать:
- В вашей ветке должна содержаться одна логически отдельная операция и никаких не связанных с ней изменений.
- У вас должны быть качественные commit-сообщения. См.
<ссылка на руководство по commit-сообщениям>. - Ваша ветка должна содержать новые или изменённые тесты для всего нового или изменённого кода. Все тесты должны выполняться в ветке. См.
<ссылка на руководство по написанию тестов>. - Ваша ветка должна содержать новую или обновлённую документацию для всего нового или обновленного кода. См.
<ссылка на руководство по написанию документации>. - Ваша ветка должна быть актуальна мастер-ветке и сливаться без конфликтов, поэтому сделайте ребейз до pull request’а.
- Весь новый код должен удовлетворять принятым у нас архитектурным требованиям и руководствам по Python, JavaScript, HTML и CSS-стилям. См.
<ссылка на руководства по архитектурам и стилям>. - Если новый код содержит изменения в схеме базы данных, то он должен включать и миграцию базы данных. См.
<ссылка на руководство по миграции базы данных>. - Если код содержит изменения, нарушающие обратную совместимость с плагинами, API-клиентами или темами, то насколько необходимы такие изменения? Эффект от таких изменений достаточно велик для отказа от совместимости? Внесён ли в список изменений отказ от обратной совместимости?
- Добавляет ли новый код какие-то зависимости (например, импортированы новые сторонние модули)? Если да, то проводилась ли оценка новых зависимостей, были ли они добавлены в соответствии с правильной процедурой? См.
<ссылка на руководство по обновлению зависимостей>. - Тестировался ли код с рабочими данными? См.
<ссылка на получение рабочих данных в руководстве разработчика>. - Если изменился пользовательский интерфейс, то проводилось ли тестирование на экранах разного размера и в разных браузерах? См.
<ссылка на руководство по адаптивному дизайну>. - Если изменился пользовательский интерфейс, то удовлетворяет ли он требованиям по доступности? См.
<ссылка на руководство по доступности>. - Если появились новые строковые переменные, видные пользователям, то были ли они интернационализированы? См.
<ссылка на руководство по интернационализации>.
Используйте исчерпывающе задокументированные стандарты программирования:
Стандарты программирования — это общее соглашение, заключаемое между разработчиками. Набор обязательных для всех рекомендаций по написанию качественного и удобного в сопровождении кода. Стандарты — фундамент ревью, потому что во время ревью мы проверяем код на соответствие стандартам. Вместо того чтобы предъявлять к коду требования, не входящие в стандарты, сделайте запрос на их включение в список рекомендаций.
Без доступного всей команде эталонного проекта стандартов разработчики могут даже не понимать, где обнаружатся проблемы при следующем ревью. А это не добавляет эффективности их работе.
Стандарты должны быть исчерпывающими. Можно опираться на стиль форматирования кода PEP 8, но это недостаточно полный стандарт для использования во время ревью.
Анализируйте то, что нужно, а остальным займутся инструменты
Во время ревью практически никогда не должны возникать проблемы с форматированием кода. Ревизии должны провоцировать появление продуктивных мыслей и дискуссий, а траты времени на стиль и форматирование в этом не помогут. Для подобных вещей используйте инструменты, например линтеры и средства автоматического форматирования.
Заключение
Это статья о том, как давать положительные заключения и правильно критиковать, как успешно вносить предложения во время ревью кода. Я предлагаю вам сделать сжатую версию этого руководства с выделенными ключевыми моментами и раздать её каждому участнику команды. Положите инструкцию в общий репозиторий, чтобы любой мог начать её разбор и предложить свои изменения, и тогда вся команда будет вовлечена в обсуждение.
Также рекомендую подумать о том, как у вас создаётся код, который потом попадает на ревью. Вероятно, вы хотите, чтобы кодер и рецензент имели похожие представления о техническом дизайне до того, как код окажется на ревью. Заранее решите, кто будет писать код для конкретной фичи, а кто будет рецензировать, и поощряйте их работать вместе, чтобы эти двое обсуждали структуру кода и его промежуточные варианты, прежде чем проверять финальную версию. Нам же нужна качественная архитектура (две головы лучше), но также нужно избегать излишних дебатов во время анализа, когда оба разработчика предлагают совершенно разные решения (одному из них придётся полностью переписать уже готовый код).
Если ещё до ревью между автором и рецензентом установится крепкое сотрудничество, то во время ревью, скорее всего, всплывёт лишь несколько мелких проблем.
Комментарии (60)
socketpair
05.01.2017 22:02-26За очевидные баги, найденные на этапе ревью, нужно унижать, как я считаю (кроме ревью в статусе WIP)
s-kozlov
08.01.2017 17:33Ищу программиста, никогда не допускающего «очевидных» багов. Пока безуспешно. Видимо, бога таки нет.
socketpair
08.01.2017 21:59-1вопрос в степени очевидности при ревью. Попадаются такие — что прям берёшь из коммита — запускаешь, а оно сваливается на чемн-ть типа синтакс еррор. или при простых кейсах неопределённая переменная или ещё что в таком роде. ну и тестов, разумеется, на новый код нет.
Впрочем, как и подтверждается из других статей про хабр, главное — не высказывать свое мнение, которое не соответствует серой биомассе, а просто поддакивать, мол, какая классная статья.s-kozlov
09.01.2017 13:48+2Со всеми бывает, все мы люди. Другое дело, если человек так регулярно косячит. И даже в этом случае лучше не опускаться до унижений, а избавиться от нерадивого сотрудника. Это же не средняя школа, где всех надо «тянуть».
Marsikus
05.01.2017 22:09Вот еще хорошая статья про кодревью: https://blog.alphasmanifesto.com/2016/11/17/how-to-perform-a-good-code-review/
Кстати, а Gerrit для ревью использовать вы пробовали?
socketpair
05.01.2017 22:11+4А вот о гораздо бОльшей проблеме с ревью, которая называется «Сделаю по-быстрому, все баги ревьюеры найдут» — ни слова.
Marsikus
05.01.2017 22:16+2Есть такое. Иногда можно делать «пост-ревью» уже замёрженного кода, что-то да найдется.
Johanga
10.01.2017 14:48Очевидно, что код ревью не позволяет найти все баги. Получит завелнные тесты или билд после коммита, начнет думать.
Если у таких разработчиков убрать код ревью, они «сделают по-быстрому, все баги тестировщики найдут». То есть проблема не в код ревью, а в самих людях.s-kozlov
10.01.2017 15:21Если у таких разработчиков убрать код ревью, они «сделают по-быстрому, все баги тестировщики найдут».
Лично сталкивался с явлением «сделаю по-быстрому — все баги найдет ревьюер».
Dreyk
05.01.2017 22:24+13Ой ну прям не программисты, а кисейные барышни на прогулке.
Конечно, нельзя оскорблять коллег, конечно, нельзя показывать свое (возможное) превосходство, но не надо сидеть часами подыскивать слова, только чтобы никого не задеть. Eсли человек, к примеру, из раза в раз делает плохо, сказать ему "Как я уже говорил" или "Мы это уже обсуждали" — абсолютно нормально.
Все также зависит от организации: если это open-source, то да, можно и поаккуратней, чтоб не спугнуть новичка, но если это человек, который получает за свою работу деньги и он лажает — ему надо об этом сказать прямо, а не скакать козликом вокруг да около
Если челоек нарушил принятые гайдлайны оформления кода, то на вопрос "Почему ты так сделал?" он ответит "А что? Что не так?" и все равно вашей следующей репликой будет "Ты написал не по гайдлайнам". Так зачем эта лишняя итерация?
Antelle
05.01.2017 22:50+1Если человек из раза в раз делает плохо, хотя ему об этом пишут, что-то не так. Лучше даже с ним поговорить. Он или не понимает, что от него хотят, или почему-то не хочет (не может) сделать правильно и ему нужна помощь. Взывание к человеческой забывчивости (я же говорил) на кодревью скорее всего не поможет.
Suvitruf
06.01.2017 01:12+1У нас был опыт работы с такими людьми. Это не относится именно к программистам. И геймдизайнеры такие были, и художники.
Проблема в том, что ты до последнего веришь, что человек исправится, признает ошибки. А в итоге через много месяцев (а иногда лет) приходится увольнять.Antelle
06.01.2017 01:18Тоже был такой опыт в одной из прошлых контор, уволили достаточно быстро, после разговора о том, что конкретно надо исправить и ревью результатов через несколько месяцев.
Так я же не говорю, что надо терпеть пока человек сам исправится. Просто это, скажем так, организационный момент, на кодревью который решать не надо.
MacIn
11.01.2017 20:53Eсли человек, к примеру, из раза в раз делает плохо, сказать ему «Как я уже говорил» или «Мы это уже обсуждали» — абсолютно нормально.
… а потом выяснится, что «плохо» — оно не плохо, а просто субъективный взгляд делавшего ревью. Который никто (по данному конкретному узкому вопросу) не принимает в расчет.
Dreyk
05.01.2017 22:26+1Ой ну прям не программисты, а кисейные барышни на прогулке.
Конечно, нельзя оскорблять коллег, конечно, нельзя показывать свое (возможное) превосходство, но не надо сидеть часами подыскивать слова, только чтобы никого не задеть. Eсли человек, к примеру, из раза в раз делает плохо, сказать ему "Как я уже говорил" или "Мы это уже обсуждали" — абсолютно нормально.
Все также зависит от организации: если это open-source, то да, можно и поаккуратней, чтоб не спугнуть новичка, но если это человек, который получает за свою работу деньги и он лажает — ему надо об этом сказать прямо, а не скакать козликом вокруг да около
Если человек нарушил принятые гайдлайны оформления кода, то на вопрос "Почему ты так сделал?" он ответит "А что? Что не так?" и все равно вашей следующей репликой будет "Ты написал не по гайдлайнам". Так зачем эта лишняя итерация?
alemiks
05.01.2017 23:29+5что-то я сломал моск немного
Не переходите на личности. Фразы вроде «Почему ты сделал именно так?» воспринимаются как нападки
Хорошо:
«Почем ты выбрал здесь такое решение?»
Так хорошо или плохо в итоге?s-kozlov
08.01.2017 17:36А всё плохо. Если человеку охота обижаться, он обидится и на «что если назвать переменную username?»
forester11
06.01.2017 00:46Проблема формальных ревью имхо в их крайней неэффективности.
1) по сути они склоняют всех к мультитаскингу — ибо сделал изменения — жди вопросов/предложений, а чтобы время не пропадало займись чем то еще, и так переключайся туда-сюда по кругу. Мультитаскинг надо избегать ибо он жрет эффективность и концентрацию.
2) зажимают все общение в рамки письменного изложения. Письменно прийти к решению/пониманию в разы сложнее/дольше чем вербально. Не все умеют излагать мысли на бумаге, не все умеют понимать абстрактные идеи изложенные письменно (кому то нужна картинка например или какая то жизненная аналогия).
3) люди в большинстве своем не умеют передавать эмоции письменно (не все родились поэтами), соответсвенно могут портится отношения на ровном месте (фокус статьи я так понимаю на этом), ибо нет эмоционального фидбэка и корректировки.
Так что если можете избегать формального ревью — делайте это.
Парное программирование, работа всей команды над одним модулем/компонентом (а не каждый в своем огородике), правило "максимально просто плюс есть тест — ревью пройден заочно", кросфункциональные команды (таск форс), и многое другое позволяет достигнуть всех плюсов формальных ревью не сжигая на это времени/нервов/ресурсов.
superyateam
06.01.2017 00:52а я бы не отказывался. В процессе формального ревью порой отлавливаются баги, фиксание которых может занять больше времени, чем ожидание когда твой пулл-реквест «заапрувуят». Также, ревью помогает не допустить добавление в проект спорных дизайнерских решений, исправление (рефакторинг) которых, может занять очень много времени. Ну и наконец, формальные ревью полезны тем, что помогают команде ознакомиться с тем, что делают остальные члены и подать голос, в случае если остальные с чем-то несогласны. Не получится так, что приходишь в среду на работу, а в master кто-то наговнокодил и после этого полдня команда разбирается с последствиями.
Aingis
06.01.2017 12:39Претензии натянутые и странные. Как будто вы не можете организовать свою работу.
1) Не обязательно мгновенно ревьювить и мгновенно реагировать. Идеальный цикл таков: отправил на ревью — подождал пока все посмотрят (это эффективней делать утром), сделал правки, реквест принят.
2) Ничто не мешает подойти или позвонить (если подойти нельзя в силу удалённости) и поговорить об всех интересующих вопросах, это вполне обычная практика.
3) На ревью и не надо передавать эмоции письменно, надо обращать внимание на конкретные моменты. Про эмоциональный ответ см. п. 2.
Парное программирование — это такая штука, о которой все говорят, но которую я никогда не видел чтобы кто-нибудь применял в обычной рутинной работе. Про «работу всей команды над одним модулем/компонентом (а не каждый в своем огородике),» вообще не понял что это такое, групповое программирование? Как вы это организуете эффективно? Таск форсы и тому подобное вообще из другой оперы и ревью никак не отменяет.forester11
06.01.2017 23:49Подождал да…
Зависит от процессов в которых вы работаете или на какие хотите выйти.
В лине (lean) «подождал» это уже waste, пусто потраченное время.
Возможно для процессов поддержки продукта, багфикса, ревью более-менее рабочее решение как девелоперов хоть как-то мотивировать читать чужой код и делится опытом и наработками.
Но когда команда уже работает над одним и тем же модулем (а не в отдельных огородиках), в тесном контакте, работает инкрементально, ревью становятся 3ей ногой которая мало того что требует ресурсов и времени на поддержку так еще и может захоти пойти в другую сторону. Получается такое ревью ради ревью, которое люди по старой памяти пытаются притянуть куда можно.
При работе удаленно парное последовательное программирование на моем опыте работает просто огненно: один девелопер заканчивает работу над модулем в своем часовом поясе/графике, отписывает что сделал и что получилось что еще нет, второй подхватывает доделывает/исправляет, и передает работу опять первому. Очень эффективно работает.
В том же ревью по умолчанию предполагается что если написали коментарий, то автор должен внести изменения. В случае с удаленной работой/поясами задержка минимум день или два, плюс добавим пару дней если кто-то чего-то не понял. К чему этот цирк и почему коментатору взять и не исправить самому? Опять же на моей практике это еще более эффективно чем писать простыни текста в ревью, взял и исправил.
Далее работа инкрементально… Представьте идет работа над новым продуктом, в идеологии агиле/лин: нет железного дизайна, есть цели которые достигают инкрементально, дизайн строится по ходу понимания проблемы все лутше и лутше, на каждой итерации рефакторя или переделывая первоначальные концепты. И вот, на первой или второй итерации вся команда начинает ревьювить и вылизывать первоначальный пруф-концепт, который наверняка в 4ой или 5ой итерации полностьт перепишут. Какой смысл это ревьювить и вылизывать? Никакого.
Надо сказать что я больше пишу про ревью внутри команды. Команда это 2-5 человек, работающая тесно над одной проблемой, в практически полной свободе. Бывает если надо внести изменения в какую-то изолированную организацию/группу ревью может и понадобится (или пулл реквест, смотря что они используют), но обычно это костыли и команда уже не кросс-функциональная.Aingis
07.01.2017 11:03И опять вы перекладываете с больной головы на здоровую.
Подождал — это, очевидно, значит переключился на другую задачу
один девелопер заканчивает работу над модулем в своем часовом поясе/графике, отписывает что сделал и что получилось что еще нет, второй подхватывает доделывает/исправляет,
Вы — внезапно! — описали процесс ревью. Только исправляет не автор, а разработчик по очереди. К парному программированию это не имеет никакого отношения. Просто работа по сменам.
В том же ревью по умолчанию предполагается что если написали коментарий, то автор должен внести изменения.
То неловкое чувство, когда не читал публикацию…
В случае с удаленной работой/поясами задержка минимум день или два, плюс добавим пару дней если кто-то чего-то не понял.
Вы написали, что удалённая работа менее эффективна, но это не проблема ревью, а процесса работы.
К чему этот цирк и почему коментатору взять и не исправить самому? Опять же на моей практике это еще более эффективно чем писать простыни текста в ревью, взял и исправил.
А если вообще решение неправильно? Причины разные: неправильное понимание задачи, незнание каких-то нюансов и т.п. Если учитель всё время будет исправлять за учеником, то он ничему не научится.
И вот, на первой или второй итерации вся команда начинает ревьювить и вылизывать первоначальный пруф-концепт, который наверняка в 4ой или 5ой итерации полностьт перепишут.
Нет ничего более постоянного чем временное. Послезавтра планы поменялись, появились другие задачи и код, «который конечно перепишут», остаётся на года. Вообще-то, вы забываете, сам процесс предполагает, что код пишется максимального качества.
P. S. Вы так и не ответили про работу все командой над одним модулем. Вот вам на новой итерации надо написать модуль. Как вы это сделаете всей командой? Не в три смены, в одном или близком часовых поясах.forester11
07.01.2017 14:04И опять вы перекладываете с больной головы на здоровую.
Мы просто в разных подходах живем.
Подождал — это, очевидно, значит переключился на другую задачу
Меня такое не устраивает так как приводит к мультитаскингу.
Вы — внезапно! — описали процесс ревью. Только исправляет не автор, а разработчик по очереди.
Так я не против ревью по смыслу. Я против формальных ревью когда людей суют в эти тулзы где можно комментировать и оставлять замечания. Часто начинается бред, статьи которые с ним борятся, хотя прямое общение отфильтровывает многие проблемы. Это так же как старожилы агиле предлагают не использовать онлайн спринт-борды, а использовать обычные физические доски в оффисе и прямое общение — такой формат позволяет избежать множества проблем и общатся эффективнее.
К парному программированию это не имеет никакого отношения. Просто работа по сменам.
Кому как. Для многих и работа «по сменам» это когда у кажлого модуля свой владелец и никто туда не лезет без спроса.
То неловкое чувство, когда не читал публикацию…
Слишком длинно но основные моменты надеюсь уловил.
Вы написали, что удалённая работа менее эффективна, но это не проблема ревью, а процесса работы.
Вообще на своем опыте я постоянно наблюдал ситуации когда люди сидящие в одном оффисе днями спорят в ревью вместо того чтобы встать подойти или какой мит устроить. Удаленная работа всего-лишь обнажает такие вещи и выявляет их неэффективность по сути, так сказать внутренний конфликт идеи.
А если вообще решение неправильно? Причины разные: неправильное понимание задачи, незнание каких-то нюансов и т.п. Если учитель всё время будет исправлять за учеником, то он ничему не научится.
Ну человеку тут же укажут что решение неправильное. По хорошему (агиле-лин процесс) все эти временные изменения должны тут-же собиратся (прямо на коммит), прогонятся юнит тесты и какая-то полуавтоматика, и паралельно роль QA уже тестировать и не ждать от девелоперов «мы закончили». Фидбэк должен быть мгновенным, если что-то сломано об этом человек узнает очень быстро.
Я не думаю что учитель/ученик подходит здесь. В школе ученик сдал задание и забыл о нем навсегда, естественно если учитель исправит это будет в пустоту (фидбэка нет). В случае с рабочей командой же код никуда не пропадает и результат кода видят все (продукт/приложение).
В свою очередь если можно сделать несколькими способами одно и тоже, и кто-то считает что одно правильное другое нет, то тут просто у команды должны быть четкие ориентиры что важно а что нет. Важно закончить в срок и отдать рабочее решение или провалить срок но вылизать кусок кода который имеет минимальный эффект для продукта и клиента? При ясных целях и ориентирах этот бред быстро заканчивается. По существу если ориентирвы выставленны во время итерации правильная команда сначала сделает чтобы все заработало, и уже в оставшееся время в спринте будет тратить на улутшения и фиксы. Если же кто-то кинется писать идеально сразу, или начнется формальное блокирующее ревью до QA, это все просто помножит риски тупо провалить сроки и потратить время в пустую.
Нет ничего более постоянного чем временное. Послезавтра планы поменялись, появились другие задачи и код, «который конечно перепишут», остаётся на года. Вообще-то, вы забываете, сам процесс предполагает, что код пишется максимального качества.
Максимальное качество можно достигнуть разными путями. Можно сразу пытатся все сделать идеально и рисковать сроками и возможностью начать улутшать то что завтра выкинут, а можно улутшать итеративно, минималистично, и то что 100% нужно здесь и сейчас. Я предпочитаю второй подход.
P. S. Вы так и не ответили про работу все командой над одним модулем. Вот вам на новой итерации надо написать модуль. Как вы это сделаете всей командой? Не в три смены, в одном или близком часовых поясах.
Не совсем правильно. Сама постановка вопроса «на новой итерации надо написать модуль» предполагает готовый дизайн, какой-то план по которому идут, т.е. попахивает вотерфолом. Такой постановки вопроса не должно быть вообще. Постановка может быть «нам надо сделать фичу X в продукте Y». Команда делает быстрый ресерч — какие изменения и в каком модуле должны быть сделаны, что протестировано, какие тесты написаны. Соотвественно один может начать писать юнит тесты, QA роль может начать готовить тест кейсы и поднимать инфу на том как воспроизвести окружение, другой девелопер начать писать концепт каких-то изменений в модуле, кто то может начать писать новый модуль (если надо). Часто бывает что надо даже разобраться с каким-то новых low-level API для нового функционала и обьяснить команде его секреты — вот вам и задача на день. Проходит день и команда лутше понимает что надо делать дальше — синхронизация — правим дальше, вместе.
Формальное ревью с блокировкой тут совершенно не в тему, имхо, оно может потянуть команду в старые процессы и весь агиле закончится.
MacIn
11.01.2017 21:14и паралельно роль QA уже тестировать и не ждать от девелоперов «мы закончили»
Замечательная идея… я прямо вижу, как разработчики вносят сегодня изменения в ту часть, которая была протестирована QA вчера. В результате либо QA не отловит проблему, либо будет делать одну работу дважды.
Постановка может быть «нам надо сделать фичу X в продукте Y». Команда делает быстрый ресерч — какие изменения и в каком модуле должны быть сделаны, что протестировано, какие тесты написаны.
Круто, если у вас есть свободная команда под этот проект. У меня на работе, например, в моей команде, которая занимается back-endом, 4 человека, и у каждого своя зона ответственности. Собрать всех вместе, чтобы пилить одну фичу? Не в этой жизни, это тупо расходование ресурсов попусту, потому что 3/4 задач вообще неясны и их постановка непонятна 3/4 команды. И только 1/4 понятна всем.
maniacscientist
06.01.2017 12:42Если «огородик» — это проблема, значит есть проблемы с интерфейсами и API, а то и ваще нездоровый агиле применяется: «мы слоны, мы сделаем все как просили»
garex
07.01.2017 07:38+1Не все умеют излагать мысли на бумаге
Программирование по сути — изложение своих мыслей
на бумагев исходном коде.forester11
07.01.2017 14:44+1Относительно все конечно излагают.
Но много ли Вы видели программистов которые пишут код 100% по делу, без лишней «пены», максимально просто и эффективно имплементят какой-то известный алгоритм? Таких единицы. Обычно же делается куча лишних движений, толи в попытке скопировать мастеров, толи потому что всегда так делали и так удобней, или нехватает IQ для выявления взаимосвязей и повторимостей.
Еще меньше тех кто умеет не только написать код, но и изложить архитектуру, видеть дальше чем тот кусок кода что сейчас меняется.
И вот если человек не может написать код четко и ясно, как он напишет обоснование, да еще обычно и не на родном языке?s-kozlov
08.01.2017 17:42И вот если человек не может написать код четко и ясно, как он напишет обоснование, да еще обычно и не на родном языке?
Пишут и еще как. Эта болезнь называется «обязательные комментарии».
MacIn
11.01.2017 21:00по сути они склоняют всех к мультитаскингу — ибо сделал изменения — жди вопросов/предложений, а чтобы время не пропадало займись чем то еще, и так переключайся туда-сюда по кругу. Мультитаскинг надо избегать ибо он жрет эффективность и концентрацию.
Иногда это неизбежно — команда маленькая, ты не можешь ждать, пока освободится твой коллега, который сделает ревью, а освободится он завтра к вечеру.
2) зажимают все общение в рамки письменного изложения. Письменно прийти к решению/пониманию в разы сложнее/дольше чем вербально. Не все умеют излагать мысли на бумаге, не все умеют понимать абстрактные идеи изложенные письменно (кому то нужна картинка например или какая то жизненная аналогия).
Надо учиться. Это имеет огромный плюс — изменение описано формально так, что сможет понять твой коллега, который не в теме — это значит, что через год, когда ты, или кто-то еще будет работать над этим куском, можно будет открыть комментарии к коммиту и описание проблемы и ее решения в task'е — ticket'е и понять, чтож там было, и почему сделано именно так. Это по сути частичное документирование кода. В противном случае получится так: ты что-то исправил, закоммитил с коротким комментарием вида «исправлениа бага с пустым полем ID», потом объяснил на пальцах коллеге за соседним столом, который будет делать ревью, а через день-два ВСЕ забыли, что это было вообще, не говоря уж о деталях реализации или том, что сам баг вызывался путем манипуляций «два притопа, три прихлопа» и только в полную луну. А так — вот оно — открыл таск — все есть.
vvscode
06.01.2017 00:58+1Может кто-то поделиться опытом, как создать эту самую атмосферу " вдохновляющий процесс, чтобы все участники ждали его и активно участвовали в нём"? Как помочь товарищам войти во вкус и читать чужие пуллреквесты с удовольствием?
ndv79
06.01.2017 21:39+2как создать эту самую атмосферу
Не следовать советам этой статьи. Людей вдохновляет реализация их идей и признание их профессиональных качеств. Людям нельзя говорить, как делать работу, только что делать. Поэтому в ходе review должны проверяться сугубо формальные требования, чтобы не возникали конфликты. Эти требования должен составить архитектор письменно. Например:
1. сложные не-UI методы должны быть покрыты unit-тестами
2. поля таблиц БД должны быть документированы
и т.д.
forester11
07.01.2017 00:17Звучит как свобода — свобода делать как хочешь, плюс минимальные рамки для достижения каких-то вторичных организационных целей. Работает замечательно с замечательными людьми на моем опыте.
Frank59
10.01.2017 17:25«людям нельзя говорить, как делать работу,»
Абсолютно не соглаесен. Если ты читаешь чужой код и видишь, как в каком-либо моменте можно сделать лучше, чем есть, обязательно стоит это предложить. Если автор кода аргументированно с тобой не согласен, он объяснит, почему он сделал именно так. В том числе и в этом есть смысле ревью.ndv79
13.01.2017 17:34Frank59, вопрос был «как сделать вдохновляющую атмосферу», а не «как замучить и перессорить программистов».
s-kozlov
14.01.2017 05:55+2Ну вот правда: не программисты, а кисейные барышни. Малейшее замечание в профессиональном тоне и по делу — и вот уже они замученные и перессоренные.
MacIn
11.01.2017 21:19Снобов и не умеющих общаться корректно по форме — за дверь, можно немножко отпинать. Там само наладится. Когда человек поймет, что коллеги его реально могут научить интересным трюкам, дать дельные советы, не унижая при этом, сам втянется.
salsaly4
06.01.2017 02:47+1На стенку. Рядом с «Настоящий мужчина должен» и «Настоящая женщина обязана» )))
Alexeyco
06.01.2017 03:30+1Но если все участники будут стараться делать всё хорошо, то такой подход может работать очень эффективно.
Я не в претензиях к автору — он наверняка счастливый человек. Будь моя воля, я бы ходил за ним со стальным прутом и бил прутом всех, кто захочет отнять у автора мороженку… ну или разрушит его калабашку в песочнице. Потому как именно эти слова обычно предшествуют БП… будь то крушение авиалайнера, железнодорожного поезда, сервера и т.д. Я когда-то тоже был молодым… и надеялся, надеялся, надеялся.
Ivan_83
06.01.2017 06:14+4Увы, совет имеет ограниченное применение, а именно случай когда люди имеют близкий уровень.
Был случай, когда чел со стороны=новенький=совсем не в теме накатал «документацию» к программно-аппаратному комплексу, годного текста там почти совсем не было: люди в теме от такой «подачи» разбежались бы (мягко говоря), а юзвери не узнали бы ничего нового и заложили в мозгах неверное понимание процесса, плюс туда было втащено всё со всех документаций имевшихся до него, в том числе и то что было уже отклонено и то что не было согласованно.
Те похвалить там можно было разве что за старание, объём и оформление.
И ещё, я бы добавил правило: если ты видишь критику в 3-5 местах то считай что всё остальное отлично и вопросов не вызывает. Но это так сказать с другой стороны взгляд.
Те я редко вижу вещи которые меня восхищают чтобы отдельно писать об этом, намного чаще код который или я бы написал так же или написан +- также как я бы сам написал или хотел чтобы оно было.
Косяки же оставить без внимания просто нельзя.
Ну или видно что нечто можно написать проще/короче/эффективнее, хотя это уже не критика а скорее обмен опытом.
Тут вот критику не любят, хотя ИМХО одно замечание ценнее сотен "+1".
Ivan22
06.01.2017 16:33+5Вобще правила хорошие, я их все в общении с женой и детьми использую. Особенно хорошо в семейной жизни помогают
пункты «Не переходите на личности», «Избегайте фраз наподобие: „Совершенно неправильно“».
Еще годный совет — «Не используйте нетерпеливых или пассивно-агрессивных оборотов».
Ну и просто бомба — «Не говорите только для того, чтобы услышать свой голос».
В общем всё это прекрасные советы для улучшения взаимопонимания в семье. Я вам как отец 2 детей
и женатый уже 11 лет заявляю.
p.s. Да, для код ревью это тоже годится. Да и вообще для повышшения эффективности любых межличностных взаимодействий!
bapcyk
06.01.2017 21:39-3Лично мое опыт ревью показывает его неэффективность в 90%-никогда ревью не находило баги, зато в 99% случаев — это самоутверждение, конфликт, субъективность, ретроградность. Очень часто, хотя и не всегда- это плохая практика, за маркетовыми словами о которой стоит резкое падение производительности команды. Это ревью гарантирует в 100% случаев. И второй негативный эффект ревью — в целом команда становится более серой, яркие решения, хакерский дух, смарт-решения, дающие преимущества в конкурентной борьбе, исчезают из команды, равно как и их носители. В целом это характерно для всей американской ИТ-культуры, которая стоит на фундаменте маркетинга, пиара, зарабатывания денег на процессе внедрения методик и написания книжек. Я рассматриваю это всё в целом — фейл с многими концепциями ООП, UML, agile, паттернами ООП, ревью, ныне супермодный devops, в этом ряду и ревью процесс — всех их объединяет вышеперечисленное. Пи-ар, маркетинг, отсутствие настоящих, объективных критериев, но зато упор на громкие имена и названия их книжек. Проблема с ревью-процессом носит общий характер, корни болезни в западной ИТ-культуре. Контркультура образовалась там же в виде Open source сообществ. И да, во все времена была коллаборация между программистами, сидение за одним монитором и решение какой-то задачки сообща, но это никогда не вело к конфликтам, потому что мотивация была совершенно отличной. Прошу пардона за столь длинный комментарий, но очень хочется услышать единомышленников, уже пришедших к подобному пониманию :)
forester11
07.01.2017 00:05Критерии то очень просты — успех продукта у пользователя, большая эффективность в его разработке (потратить не годы а месяцы), и чтобы девелоперам нравился процесс и результаты.
Проблема обычно в том что люди начитаются какой-то мотивационной части методик (маркетинг и пиар )) ), начинают их употреблять не по существу а формально (дейли стэндап — как это круто! чтобы не филонили на работе), и потом обнаруживают что ускорения нет а наоборот, плюс еще и девелоперы демотивированы, и потом неправильный вывод делается что виновата методика или вообще вся индустрия/страна/вселенский заговор. Тот-же агиле очень легко зафейлить, например используя ревью — засунув в ревьюверы не участников команды а кого-то со стороны. На этом агиле в принципе и заканчивается с перечисленными Вами проблемами (самоутверждение, ретроградство и так далее).
А агиле то подразумевает что вся команда сама-себе менеджер. Практически полная свобода действий. Но так как для менеджмента это часто уж слишком круто, и будет «хаос» в их понимании, агиле начинается с менеджерами и прочими намордниками и заканчивается так и не начавшись.
ДевОпс тот-же пошел из Гугла/Амазона и компания насколько я понимаю, от лидеров рынка. Какие еще могут быть критерии? Другое дело что когда начинают копировать не понимая как это работает (не разбираясь в принципах на которых построены уже практики) настает печаль.
ndv79
14.01.2017 22:56«Вы не умеете его готовить» :)
см. мой коммент как сделать чтобы не было проблем.
Feniksss
06.01.2017 23:43Слишком много и толсто. Со всех сторон, конечно. Но в реальности всё гораздо проще.
Не грубить и не зарываться в своих умных советах, потому как иногда, именно это и рушит команду/взаимоотношения в команде.
За чек-лист плюс. Жаль что это возможно далеко не во всех реальных проектах.
Frank59
10.01.2017 17:22Хорошая статья. Согласен практически со всем написанным.
Еще важный момент — пресекать холивары, которые могут зарождаться вокруг какой-нибудь фигни. Как правило для этого достаточно немного доброй воли. Для других случаев у нас в команде действует следующее правило, если ревьювер и автор кода прицнипиальное не согласны в каком-либо моменте(условно спорят больше 5 минут), зовем 3го и делаем так как он говорит.
izvolov
Я считаю, что профессионалы должны быть готовы открыто обсуждать друг с другом результаты своей работы. В том числе во время обзора кода. Без грубости, спокойно, аргументированно, но открыто и честно.
Если код можно написать более эффективно и более идиоматично, то это нужно сделать. Если есть более эффективный алгоритм, то его нужно использовать. Либо аргументировать, почему в данной конкретной ситуации этого делать не нужно.
Мы технические специалисты, а не "художники с тонкой и хрупкой душевной организацией".
lookid
> Мы технические специалисты, а не «художники с тонкой и хрупкой душевной организацией».
Посмеялся. Это как «я женщина, а не посудомойка».
У нас профессионал начинается с 5+ лет опыта. А до этого нужно еще дожить. Это в США стажер проходит 3-5 часов собеседования, а у нас всё проще. Если ты не гуру проекта, языка, рефакторинга или технологии, то ревьюить тебе бессмысленно. Так что я бы с тобой очень сильно поспорил. У нас до 5+ лет, как раз таки «художники с тонкой и хрупкой душевной организацией». Я лично вообще не видел человека, который может ревьюить с опытом меньше 3-5 лет на фултайме или меньше синьера. Большинство начинающих программистов даже читать про рефакторинг не начинали.
Borz
тем более надо учиться обосновывать свой выбор — пока поясняешь либо сам поймёшь что был прав или не прав, либо же коллеги покажут что можно доработать если в этом есть необходимость.
xzDeveloper
Грустно, но очень правда…
Johanga
Ревью это еще и обучение. начинающий и думающий разработчик может подхватить хорошие практики, которые использовал сеньор. А иногда и сеньору есть чему поучиться у начинающего.
MacIn
По факту — технические специалисты низкой квалификации. Же.
superyateam
Дело не в тонкой душевной организации.
Любой человек может выйти из себя, если его постоянно критиковать, хоть и за дело. Вместо того, чтобы учиться, он будет лишь огрызаться, и в итоге возненавидит вас и уйдет из проекта. Да, если вы все это время были правы, а он — нет, то это еще более менее можно назвать положительным исходом. И это при том, что вы создали токсичную атмосферу и настроили против себя возможно и других членов команды.
А что если вы не всегда были правы? Что если временами он был прав? Что если вы придирались абсолютно по пустякам, просто потому что привыкли придираться к конкретно этому члену команду? Наконец, что если вы рядовой член команды, как и постоянно вами критикуемый, и в общем-то не несете определенной ценности в работе — просто кодер? Мне кажется, в данном случае, вы только ухудшаете атмосферу на работе и в коллективе. И я бы как начальник, как минимум, провел бы воспитательную беседу с обоими.
В конце концов, ничего сложно нет в том, чтобы быть вежливым коллегой и тактично рецензировать работу. Пальцы от этого не отвалятся, а язык не отсохнет. Но при этом, в коллективе будут дружеские отношения, да и вашей же репутации и авторитету это пойдет только на пользу.
Borz
Честно сказать "да, я был не прав. Спасибо за подсказку" разве нельзя?
Если придираешься по мелочам, то это увидит вся команда и вполне нормально будет, если тебе обратят на это внимание, т.к. ты сам мог и не понимать этого
superyateam
во-первых, не все могут сказать «я был не прав». во-вторых, то что я говорю, как раз и избавит от таких неприятных бесед с командой. По-моему, проще изначально быть тактичным.
В общем, в двух словах: ты ставишь reject на пулл-реквесте, и он в мастер уже не попадет, так какой смысл при этом быть еще резким и грубым?
PsyHaSTe
Как-то раз я не мог пройти ревью просто с фразой «зачем ты сделал ПО-СВОЕМУ??? Все равно в итоге будет по-моему или я не позволю залить этот код в апстрим, так зачем тратить время?». Причем самое обидное, что я был готов сделать так, как просят, если бы мне внятно объяснили задачу, а не сказали «сделай так-то так-то», а потом выяснилось, что делать надо совсем иначе. Доходило до смешного — практически любые стили использующие calc приходилось переделывать на JS, потому что «это нечитаемый декларативный стиль, а вот скрипт отладил, и по шагам все понятно, что где происходит».
И да, фразу «я был не прав» от человека не слышал ни разу. Даже когда мне предлагалось делать в REST API запуск сервиса по get-запросу. «Зато в браузере удобно набирать, а твой пост только через утлиту отправить можно»©.
garex
Любой программист — человек. А любой человек может быть демотивирован бездарно-построенным процессом управления или никакой культурой.
Относиться к сложным системам типа "человек" как к простым биомеханизмам типа "бактерия", в которых не надо особо разбираться — сродни забиванию гвоздей микроскопами.
alexBondar
1. Люди разные — не все способны принимать критику, признавать свои ошибки и т.д., не говоря уже о том, что значит слово профессионал и какой процент таких людей в команде :)
2. Мне кажется упущен главный момент «в распределенной команде», где люди зачастую и не видели друг друга в лицо и пулл реквесты — это и есть 90% общения. А если учесть, что многие большие компании имеют сотрудников не то что в разных офисах или городах, а на разных континентах. В таких случаях умение правильно комуницировать проблему — это путь к здоровой атмосфере на проекте.
Не стоит забывать, что если человек (с которым вы лично не знакомы) начинает учить жизни — мы начинаем считатать его
мудакомнехорошим человеком — так работать защитный механизм на подсознании большинства людей.Более того, никто не отменял стереотипы. Как бы вы отнеслись если бы некий Камлеш Кумар Кришна начал бы оставлять каменты типа «все не то и все не так».
Так что обеими руками за корректные каменты описанные автором.
PsyHaSTe
Насчет пункта 2 не совсем согласен: когда критика приходит по адресу, например лично я принимаю её с какой-никакой благодарностью, ибо я обучаюсь таким образом, в худшем случае имеется злость на себя из-за наступания на одни и те же грабли в который раз. Но когда вполне нормальный код комментируется одним WTF и блокированием мерджа, то тогда такие мысли возникают, да. Но тут уже вопрос как мне кажется не в защитных механизмах, а в том, что это вполне нормальная оценка человека, который подобный образом относится к ревью.