Ответы на общие вопросы
Проверка кода кажется очень простой задачей, но, как и во всем остальном в ИТ, здесь есть нечто большее, чем может видеть глаз. Поскольку я провожу большую часть своего дня в вавилоне, просматривая чужой код, я подумал, что напишу некоторые мысли по вопросам, которые беспокоили меня в прошлом. Итак, вы идете, краткий обзор обзоров кода, основанных на накопленном опыте и процессах, используемых здесь, в babylon.
Определенно. Отличная возможность учиться здесь. Даже если у вас нет комментариев к обзору, это все равно означает, что вы читаете массу кода других людей и улучшаете свои навыки. Вы также увидите, как более старшие люди подходят к отзывам, а также к разногласиям!
Должен ли я, как старший разработчик, просматривать запросы на вытягивание?
Абсолютно. На это есть несколько причин:
- Всегда есть чему поучиться, даже у более молодых разработчиков.
- Вы можете понять проблемы, с которыми сталкиваются конкретные разработчики, и помочь им.
- Опытным разработчикам легче добиться согласованности в проекте. Через некоторое время вы просто запоминаете все, что связано с руководящими принципами проекта.
- Выявление повторяющихся проблем позволяет старшим разработчикам начать думать о том, чтобы поднимать эти проблемы на собраниях группы, создавать дополнительные руководящие принципы для них, проводить презентации или писать статьи или, возможно, писать собственные правила статического анализа для выявления этих проблем до проверки кода.
Здесь есть лишняя пустая строка, мне добавить к ней комментарий?
В целом я бы этого избегал. Обычно есть более важные вещи, которые можно прокомментировать в запросе на вытягивание. Нет необходимости увеличивать количество комментариев только для пустого места, которое всегда можно исправить позже.
На что следует обращать внимание при просмотре запроса на вытягивание?
Обычные вещи:
- Убедитесь, что решение спроектировано правильно и в соответствии с рекомендациями команды. 2 принципа, которые я всегда имею в виду при рецензировании, - это YAGNI и KISS.
- Убедитесь, что все проверено. Это означает фактическую проверку того, что для вновь представленных классов не пропущены тесты. Выполнение тестов обычно является задачей CI-сервера, а не вашей. Кроме того, покрытие тестами не обязательно гарантирует, что код протестирован (т. Е. В тестах действительно есть утверждения).
- Убедитесь, что тесты читабельны. Разработчики, как правило, придерживаются двойных стандартов в отношении производственного и тестового кода.
- Делегируйте простые вещи автоматизированным инструментам статического анализа. Вы не должны выполнять работу, которую может выполнять машина. Потратьте немного времени заранее, чтобы настроить эти инструменты с правилами, которые имеют смысл для вашей команды. Еще лучше, если вы можете заставить их запускаться при каждом запросе на перенос на сервере CI, таком как Jenkins. Наиболее распространенные инструменты статического анализа: Checkstyle, PMD, Findbugs, Lint и ErrorProne.
Здесь, в babylon, мы используем Checkstyle и Lint. Думаю о возможном добавлении PMD в ближайшем будущем. Для получения некоторого руководства по их настройке вы можете взглянуть на мой маленький любимый проект, который содержит конфигурацию для PMD, Findbugs, Checkstyle и Lint.
Реже:
- Убедитесь, что экранная навигация имеет смысл.
- В некоторых случаях может потребоваться обновление документации.
- Убедитесь, что изменения ориентации обрабатываются правильно.
- Убедитесь, что никакие правила Proguard не нужно обновлять в рамках этого запроса на перенос.
- Если были добавлены какие-либо библиотеки, убедитесь, что команда согласна с добавлением.
- Для определенных функций производительность может быть проблемой.
- Убедитесь, что добавленный код не влияет на безопасность.
Какие инструменты помогут с отзывами?
Как долго должен длиться обзор?
Это зависит от размера и сложности запроса на вытягивание. Для меня обзоры в наши дни, как правило, длятся от получаса до пары часов. Если это длится всего 5 минут, значит, вы не просматриваете код, вы просто прокручиваете его.
Есть ли рекомендации по количеству комментариев?
Опять же, это зависит от размера запроса на вытягивание. Надеюсь, подавляющее большинство запросов на вытягивание небольшие и содержат менее 10 комментариев. Если их больше, я могу выбрать наиболее важные и удалить некоторые менее важные.
Стоит ли протестировать решение?
Это зависит. Если у вас есть отдельная команда QA, которая рассматривает заявки, вы можете протестировать только очень конкретные крайние случаи, которые приходят в голову, и позволить им выполнять большую часть работы. Если вы работаете в небольшой команде, возможно, вы захотите проверить решение самостоятельно более подробно.
Сколько рецензентов на запрос на вытягивание?
Зависит от размера команды. В целом, поскольку идеального кода не существует, чем больше людей просматривают пулреквест, тем больше комментариев вы получите. У нас, в babylon, есть минимальное требование - один рецензент для каждого запроса на вытягивание, и мы рассматриваем возможность увеличения этого количества как минимум до двух рецензентов на запрос на вытягивание.
Как выглядит временная шкала запроса на включение в babylon?
- Кто-то открывает пул реквест.
- (Разработчик обычно ничего не делает в какой-либо группе по поводу нового запроса на вытягивание, разработчики несут ответственность за отслеживание открытых запросов на вытягивание).
- Рецензент (или двое) обычно начинает просмотр в течение нескольких часов.
- Рецензенты заканчивают проверку и либо одобряют пул-реквест, либо запрашивают изменения, обычно в тот же день.
- Рецензент (и) дает понять автору пул реквеста, что они завершили рецензию, и просит их просмотреть комментарии и начать применять все изменения, с которыми они согласны.
- Автор запроса на вытягивание делает это. Когда они заканчивают, если есть что-то, с чем они не согласны, они обычно пишут рецензентам, чтобы они быстро обсудили это.
- Автор запроса на вытягивание и рецензенты приходят к соглашению об ожидающих комментариях, окончательных изменениях, переданных в ветку автором запроса на вытягивание.
- Запрос на вытягивание объединен!
Максимальное время, в течение которого пул-реквест остается открытым?
Чем быстрее он закроется, тем лучше для всех. Сохранение их открытыми в течение долгого времени вызывает разочарование у автора, поскольку в конечном итоге будут возникать конфликты слияния (которые все ненавидят). Здесь, в babylon, большинство запросов на вытягивание объединяются в течение дня или двух.
Что произойдет, если разработчики в чем-то не согласятся?
Вот незадача! Это должно происходить только в очень редких случаях. Мы все здесь взрослые и должны суметь договориться о чем-то. Лично для меня http://justflipacoin.com/ работает отлично. Это всего лишь код. Если оба решения одинаково действительны, и я не согласен с другим разработчиком, тогда достаточно просто подбросить монетку, и мы можем продолжить. С другой стороны, если один из разработчиков считает, что есть серьезные проблемы, связанные с реализацией, мы всегда можем пригласить третьего / четвертого / пятого разработчика, чтобы они посмотрели и обсудили.
Могут ли младшие разработчики утверждать запрос на вытягивание?
Ну .. скорее всего нет. Лучше, чтобы кто-то с большим опытом мог одобрить пулреквест. Популярной стратегией является стратегия Gerrit, при которой разработчики могут дать +1 или +2 запросу на вытягивание и минимум один +2 требуется для его объединения.
P.S: То же самое касается людей старшего поколения, которые только что присоединились к проекту. Было бы неплохо немного ознакомиться с кодовой базой, прежде чем иметь возможность утверждать запросы на вытягивание.
Есть еще какие-нибудь подсказки?
- Убедитесь, что вы проверили рассматриваемую ветку, это действительно помогает понять код. Инструмент Upsource может помочь выполнить полную проверку кода в Android Studio.
- Если комментариев слишком много, вы можете рассмотреть возможность применения комментариев к обзору вместе с разработчиком, который сделал запрос на вытягивание, парный стиль программирования. Немного ускоряет процесс.
- Вежливость всегда помогает. Будьте профессиональны и уважительны. Все делают ошибки, и обычно нет единого способа кодирования решения.
- Если вы считаете, что с кодом, который вы просматриваете, что-то не так, убедитесь, что вы предоставили много объяснений того, почему он неправильный, а также предложения по устранению проблемы (возможно, даже несколько предложений).
Есть полезные ссылки?
Вопросы и правила проверки кодекса
