Часть вторая. Как проходить code review по версии Google
Возможно вы читали первую часть статьи про код ревью со стороны ревьювера (кстати, мы уже успели ее обсудить в последнем выпуске подкаста «Цинковый прод»).Так как статья набрала много лайков, пишу обещанное продолжение про код ревью с другой стороны — со стороны автора изменений кода
Как обычно, будем говорить MR (Merge Request) вместо CL, потому что термин CL мало кто понимает.
Оригинал инструкции для авторов MR по версии Google можно посмотреть здесь, а я дам краткую выжимку.
Итак, поехали
Описание MR
В Google описание изменений сохраняется в истории системы контроля версий, и с большой вероятностью его будут читать очень много людей в будущем. Поэтому описание MR очень важно.
В первой строчке (в заголовке) должно быть одной фразой описано, что было сделано в MR. Причем по традиции применяется императивный (повелительный) стиль, т.е. Delete blablabla, а не Deleting blablabla
Само описание должно быть информативным, содержать краткое описание решаемой проблемы, ссылки на необходимые документы (если необходимо), задачи таск трекера и другой контекст. Даже в маленьком MR что-то такое должно быть.
Описание типа «Fix bug», понятное дело, считается неадекватным.
MR должен быть как можно меньше
- Маленький MR можно быстро проверить
- Проверка будет более осмысленной
- Меньше вероятность упустить баг
- Не так обидно, если весь MR будет отклонен. Ведь очень плохо, когда сделана большая работа, а потом выясняется, что все это вообще было не нужно
- Проще вливать изменения, меньше конфликтов
- Легче добиться хорошего качества кода
- Чем больше изменений за раз, тем сложнее откатывать код при такой необходимости
Очень редко бывает ситуация, когда нельзя уменьшить размер MR, разбив его на части. Ревьювер имеет право отклонить MR, если он слишком большой
Конечно, не может быть жесткого правила, какой MR будет считаться большим, какой маленьким. 100 строк кода — это уже большой или еще нет? Кто знает.
- MR должен делать что-то одно. Обычно это не вся фича, а ее часть
- Выделяйте рефакторинг в отдельный MR
MR должен быть маленьким, но самодостаточным
- Всё, что необходимо ревьюверу для понимания MR, должно быть в этом MR
- После вливания кода, система должна функционировать нормально.
- Если добавляется метод API, в этом же MR должны быть продемонстрированы и способы его использования. Другими словами, MR не должен быть настолько маленьким, чтобы трудо было понять как он будет использоваться, к каким последствиям приведет.
- Покрывайте код тестами, причем тесты должны быть в этом же MR
Не принимайте близко к сердцу
Иногда ревьюверы ведут себя не очень вежливо, могут написать что-нибудь плохое про ваш код. Это не очень профессионально с их стороны, однако зачастую зерно истины в таких комментариях всё же есть, и это надо учитывать. Не отвечайте слишком резко. Это только усугубит ситуацию.
В случае, если вам не нравится тон разговора в комментариях, лучше найти этого человека и пообщаться с ним лично, объяснить, что вас не устраивает.
Если и это не помогло, Гугл советует обратиться к менеджеру.
Из моего опыта хочу добавить, что зачастую человек пишущий невежливый комментарий, часто не отдает отчет, что это может смотреться как-то агрессивно. Текст скрывает половину эмоций; например, сказанное в шутливо-дружественном тоне в текстовом виде может казаться жестким наездом. Поэтому полностью согласен с гуглом — в случае недопонимания общаться только лично!
Объясняйте кодом
Если вас просят объяснить какой-то момент в коде, подумайте, нельзя ли поправить код так, чтобы он был понятен без объяснений. Потому что если один не понял, то и другие могут не понять.
Реакция на комментарии ревьювера
Зачастую, когда работа сделана и отправлена на код ревью, психологически довольно сложно принять тот факт, что придется еще и что-то исправлять. Поэтому постарайтесь не воспринимать комментарии ревьювера в штыки, подумайте, возможно он прав, и код станет в результате лучше.
Однако если ревьювер все же не прав, смело пишите об этом, снабдив ответ аргументами и фактами.
Выводы
В целом, как я понял из документа от Гугл, автор MR должен сделать всё, чтобы облегчить задачу ревьюверу; чтобы ревьювер понял, для чего был сделан код, как был сделан код, должен быть весь необходимый для понимания контекст и т.д.
А неизбежно возникаемые разногласия решать, приходя к консенсусу в вежливой профессиональной форме.
В следующем выпуске «Цинкового прода» мы обязательно обсудим эту статью (и многое другое), поэтому не забудьте подписаться на наш подкаст, иначе пропустите самое интересное!
