--- title: Code review и конфликт в динамике команды date: 2017-11-03T10:00 tags: [управление] --- Команды программистов из 3-7 человек это идеальная машина по быстрому созданию качественного продукта. Слишком много - и все погрязнут в бесконечных обсуждениях, слишком мало - будет сбиваться ритм и снижаться продуктивность и качество. Я мало что понимаю в менеджменте, поэтому меня больше интересуют вопросы **конфликтов** и **улучшение инспекции кода** для улучшения продукта и сплочения команды. **Code review**, **pull request review** и **peer review** это практически одно и то же, различие лишь в фокусе внимания - на систематичность просмотра существующего кода, на принятие чужого кода или на массовость читателей. Я буду использовать термин **ревью** как процесса и **инспектора** для принимающей стороны. ![](../img/william_frederick_yeames_-_and_when_did_you_last_see_your_father__-_google_art_project.jpg) ### Автоматизация скучного Самое очевидное что надо сделать для команды - ****убрать дебаты о стиле**** переменных, кавычек, количестве пробелов и границе сложности функций. Это всё должны делать автоматические инструменты до коммита - Style guide - надо иметь свои настройки для phpstorm + так же настроенный phpcs, [codeship](https://codeship.com/) - Linters - jslint, phpmd - Complexity - [sonar](https://www.sonarqube.org/) - Test coverage ### Хороший pull-request Если вы предлагаете код для внесения в репозиторий своей команды, имеет смысл описать его контекст, что-бы сразу было наглядно видно - ​Номер тикета в Jira - Требования (скопированные из тикета) - Скриншоты / [gif анимация](https://giphy.com/apps/giphycapture) наглядных UI-изменений - Зависимости от других pull-request'ов и порядок их слияния - Заметки на конкретных строках где вы не уверены или хотите обратить внимание команды Размер изменений должен быть достаточно мал что-бы ревью не превышало часа и [500 строк кода](https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/). При разработке фичи, размер рефакторинга побочных функций не должно занимать более ~ 20% от всех изменений. ### Хороший ревью Процесс принятия кода в репозиторий должен начинаться ****сразу**** после готовности pull-request'а. Это не даст автору и коду остыть и впасть в конфликт с новыми запросами. Ревью проходит в плане абстракций ****сверху вниз**** и по порядку проверяет - Выполнены ли требования, обрабатываются ли ошибки - Архитектурную согласованность — API, DB - Нефункциональные требования - Читаемость - именование переменных, порядок аргументов - Сложность - принцип single responsibility, иерархия вызова функций, повторное использование - Безопасность - инъекции, передача личных данных - Performance - количество запросов к сети, БД, узкие места от количества данных, асинхронные race conditions #### Критика и язык Пожалуй **самое сложное в ревью** это не обидеть автора, не снизить его самооценку и не вызвать личную неприязнь - Используйте пассивную форму общения (вопросы и предложения), а не повелительное наклонение (приказы и требования) - Избегайте обобщений (всегда, никогда, все) - Критикуйте код, а не автора - избегайте ярлыков, осуждения и перехода на личности - Не перекладывайте на разработчика слишком много вопросов (а что будет, если..) где вы подозреваете Edge case. Проверьте сами и поставьте его перед фактом - Чётко указывайте степень своей уверенности (факт - вероятность - мнение) и степень внимания (критичность) комментария - Проявляйте заботу при критике - предлагайте псевдокод, ссылки на строки кода или обучение.  - Знайте когда перестать устраивать чат в гитхабе/битбакете - Если всё тлен - подходите физически и ****программируйте парно**** ### Типы комментариев Поскольку я постоянно занимаюсь ревью, а на один pull-request у меня обычно уходит 5-20 комментариев, то я специально вначале пишу тип комментария, что-бы автору сразу было понятно на что стоит обратить внимание сразу | | | |---|---| |Major|дефект, нечто явно не работающее, вызывающее ошибку| |Medium|какой-то случай не обрабатывается или не по-стандарту, загрязнение объекта переменными, загрязнение функции низкоуровневыми функциями, нарушение state из-за race conditions| |Edge case|довольно редкий не обрабатуемый случай, надо чётко высказывать обязательно ли его исправлять, или решать автору| |Naming|субъективное мнение о читаемости переменных, функций, исключений| |Out of scope|возможное улучшение, которое явно не стоит делать в рамках этой задачи (например UI handling под mobile)| |Optional|альтернативное решение (другая функция, другой порядок и тп.) о которое не обязательно обсуждать| |Minor|мелкая проблема со стилем кода, опечатка, ненужная переменная или вызов функции, отсутсвующий jsdoc, покрытие тестами| |Debateable|спорный момент в котором инспектор не уверен и хотелось бы поднять обсуждение команды| |Security vulnerability|потенциальная уязвимость, утечка или повреждение данных, DoS| |Note|обращение внимания, не требует изменений|| |Performance|неоптимальные запросы, алгоритмическая сложность| |Improvement|небольшое улучшение которое можно было бы сделать в этой задаче| |Redundant|лишний код| |Like|похвала. Хорошее решение, радость от рефакторинга или использование доселе неизвестной мне функции| |I donno|я не понимаю что этот код делает / незнаю синтаксиса| |Why|стандартный вопрос когда не понимаю зачем это надо| ### Спорные моменты **Должен ли инспектор проводить поиск дефектов, т.е. проводить QA?** Я склоняюсь к тому что да, инспектор должен локально воспроизвести состояние нового продукта и полностью его **протестировать**. Однако если инспекторов несколько и они делают проверку одновременно, а при этом коммитятся изменения несколько раз подряд, это сильно растягивает процесс и вносит неразбериху **Сколько человек должно проводить инспекцию и кто это должен быть?** Самое быстрое и эффективное решение - один человек кто сейчас может. Но я склоняюсь к тому что **ревью должна делать вся команда**. Pull request обязательн получить минимум 2 одобрения, после чего полноценную установку с поиском дефектов делать не обязательно ### Стратегии выбора инспектора В разных компаниях и командах выбор смотрящего происходит по-разному, зависит от ситуации: - всегда выбирать ближайшего **старшего** разработчика (тим лид) — подходит если много новичков - всегда выбирать более компетентного — подходит если вы коммитите в чужой репозиторий - по специализации (бэкэнд, знанию фреймворка, БД) - по доменной области / коду (по частоте работы с конкретным файлом) - broadcast, оповещение всех, кто свободен тот и глянет — подходит если нет жёсткого контроля, но могут быть проблемы с провисанием ревью из-за низкой мотивации - контрольный центр — старший делегирует ревью конкретному человеку (диктатура). Получится быстро, но сильно зависит от логики центра - контракт (пара) на спринт — может вызвать губительное привыкание - эксперт со стороны — полезно для редких ситуаций ### Конфликт Конфликты в команде это благо. Это показатель здоровья и конкуренции мнений. Конфликты возникают из-за **ценностей** или из-за **процесса**. Например, если мы спорим из-за **названия переменных** и функций, то это нефункциональная ценность **читаемости**, т.е. поддерживаемости проекта. Если этот спор длится слишком долго, возникает лагерь для которых важна **скорость разработки**. То же может происходить из-за конфликтов **безопасности и UX**, **масштабируемости и консистентности**. Это конфликты ценностей. Если у нас scrum и PM описывает задачу используя обобщения ("сделать чат как в фейсбуке") вместо детального описания, то разработчику прийдётся **самому** исследовать и формализовать требования **вместо** PM, что ему может не понравиться. Аналогично, разработчики могут не делать глубокого поиска дефектов, считая это **ответственностью** автора, или не заниматься поддержкой live среды, считая это **работой** operation engineers. Это конфликты процессов. Лучший способ разрешения конфликта это выговориться. В Agile для этого есть **ретро-митинг** за неделю, где можно письменнно и аргументированно выплеснуть свои эмоции. Это даёт возможность начальству отслеживать состояние команды, скажем на протяжении года. Худший вариант это **подавление эмоций** и удаление от конфликта и безразличие — «ну хорошо раз Петя хочет так, то пусть так, а если Вася хочет по-другому, то пускай сами разберутся, а я сделаю как они решат» #### Личный опыт Поскольку я конфликтный сам по себе, поднятие проблем достаточно простое дело. Но всё-таки приятно когда конфликт успешно разрешается так что обе стороны довольны #### Конфликты ценностей - ​Шумности в офисе (концентрация vs общение) - Наличие bus factor в команде (специализация и глубина vs масштабируемость) - Фокус на одном типе тестов (отслеживаемость и простота) vs разнотипные баги (эффективность) - Однородные микросервисы (надёжность) vs изучение новых, более эффективных фреймворков (развитие) - Централизация данных для разработки (управление) vs локализация и усложнение обновлений (надёжность) #### Конфликты процессов - Глубинное планирование vs решения на ходу - Внесение изменений в ходе спринта vs гибкость разработки - Напряг с поиском инспектора кода vs свобода разработчиков в планировании своего времени - Зависимость задач между собой vs возможная параллельность - Медленной реакции / общении между командами vs фокус на своих продуктах - Рост технического долга vs быстрая ранняя скорость разработки - Ломающийся лок зависимостей на час vs долгий и болезненный апгрейд всех сервисов - Стандартизация UI компонентов vs трата времени и денег при редком использовании #### Личностные особенности Катализатором конфликтов могут быть межличностные отношения. Люди по-разному относятся к общению. Я больше **формалист** и поскольку **память** у меня никакая, приходится много сводить к записям и им я доверяю больше.  Другие люди имеют свои недостатки — кто-то наобещает всего и забудет. Кто-то медленно продвигается, но всех опрашивает, кто-то двигается быстро, набивает шишки и матерится из-за этого, а кто-то сидит тихо и не любит критику, кто-то двигается медленно потому что не любит спрашивать помощи.. > Но именно готовность к открытому конфликту — важное условие, чтобы стать полноценной рабочей группой. Команда сначала должна подраться. Иначе к решению быстрых, сложных задач она не способна. > > [Павел Вейник](https://dev.by/lenta/main/komanda-snachala-dolzhna-podratsya-trudnosti-razrabotchikov-kotorye-podalis-v-timlidy) ### Отличие от парного программирования Парное программирование это ревью кода в реальном времени. Но есть нюансы: - После парного программирования не остаётся **письменной истории**. Остальная команда получает уже хороший код и поэтому может медленней учиться на чужих ошибках - Парное программирование физически синхронное. Это значит что ревью нельзя делать в другом пространстве-времени. Я частенько просматриваю код из дому, повышая свою эффективность Несомненно от него огромные плюсы по глубине охвата, скорости дебага и качеству кода. ### Польза Ревью необходимы команде, потому что оно: - Улучшает и украшает код - Находит альтернативные, элегантные способы решения - Передаёт знания о продукте - Увеличивает владение и повторное использование кода - Информирование команду об изменениях и поднимает дискуссии - Разрешает конфликты через консенсус, снижая негатив при этом - Обучает разработчиков - Снижает число дефектов - Даёт быструю обратную связь для ранних прототипов - Создаёт историю причин и следствий - Позволяет оценивать работу команды через сбор метрик См. также - [Code Review Etiquette](https://css-tricks.com/code-review-etiquette/) - [How to Do Code Reviews Like a Human](https://mtlynch.io/human-code-reviews-1/) - [May the Code Review be with you](https://habrahabr.ru/company/avito/blog/330846/) - [Конфликты команды разработчиков](http://artamonov.ru/2009/11/30/conflicti-komandi-razrabotchikov/) - [Top 10 obstacles to faster code review](https://hackernoon.com/top-10-obstacles-to-faster-code-reviews-and-better-code-quality-5b185df21577) - [Gitlab code review guidelines](https://docs.gitlab.com/ce/development/code_review.html) - [Refactoring guru](https://refactoring.guru/refactoring/what-is-refactoring) ## Related - [Система прав в инфосистемах](/ru/blog/управление/sistema-prav-v-infosistemah/) - [Управление pivotaltracker задачами в PHPStorm](/ru/blog/управление/upravlenie-pivotaltracker-zadachami-v-phpstorm/)