Вопросы код-ревью меня интересуют очень давно. Много раз возникали те или иные проблемы то с качеством кода, то с климатом в коллективе. И действительно, code review — это если не единственное, то одно из самых главных мест для возникновения конфликтов в коллективе разработчиков.
И вот недавно при подготовке к очередному выпуску подкаста "Цинковый прод" я узнаю, что Google опубликовал свод правил по проведению Code Review, битком набитый ценными мыслями. Весь материал довольно объемный и не влезет в одну статью, поэтому я постараюсь выделить наиболее интересные (мне) мысли.
Итак, поехали
Термины, используемые в оригинальной статье:
CL — changelist. Но обычно это называют Merge Request или Pull Request, поэтому в статье я буду использовать сокращение MR
LGTM — Looks Good to Me. Короче, когда жмут кнопку "approve". Я буду использовать термин "апрув", как более понятный населению.
Идеальность MR
На практике можно встретить различные крайности при рассмотрении MR. Кто-то всей командой задалбывает замечаниями, пока всё до мелочей не будет исправлено, кто-то смотрит примерно логику и жмёт "апрув".
В Гугловском документе написана интересная мысль. MR не должен быть идеальным, но он должен улучшать кодовую базу. Т.е с каждым привносимым изменением код должен становиться лучше и лучше. И если MR добавляет много хорошего, то не надо придираться к мелочам; выгоднее, чтобы это улучшение попало в код побыстрее.
Никакой MR не должен делать код хуже. Единственное исключение — это если MR является срочным фиксом чего-нибудь.
Свобода делать замечания
Несмотря на то, что нельзя задалбывать мелочами, тем не менее можно свободно эти мелочи писать хоть к каждой строчке. Противоречие с предыдущим пунктом решается просто: мелочи и придирки ревьювер помечает префиксом "nit:" от англ. nitpick (придирка). Такие замечания фиксить необязательно, однако автор MR может захотеть что-то исправить или, даже если нет, учесть на будущее некоторые моменты.
Факты важнее персональных предпочтений
Почти всегда стандартные принципы построения ПО (software design), базирующиеся на лучших практиках, лучше, чем хитровымученные велосипеды. Поэтому предпочтение нужно отдавать им.
Если можно применить несколько стандартных подходов, то это на усмотрение автора.
Прямая цитата для лучшего понимания:
Aspects of software design are almost never a pure style issue or just a personal preference. They are based on underlying principles and should be weighed on those principles, not simply by personal opinion. Sometimes there are a few valid options. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the author. Otherwise the choice is dictated by standard principles of software design.
Если ревьювер и автор MR не согласны друг с другом
Сначала идет попытка достигнуть консенсуса в комментариях к MR. Если это не получается, то личное обсуждение. Если все равно не пришли к единому мнению, то привлекать членов команды. Но главное, MR не должен застревать надолго из-за несогласия двух человек.
Обсудили в коментах -> Обсудили лично -> Обсудили в команде -> Двигаемся дальше
Скорость проверки MR
Скорость экстремально важна. Если раз в несколько дней давать по одному комменту, то автор MR будет жаловаться, что к нему придираются и не дают работать.
Если MR проверяется очень быстро, то любые замечания не будут являться большой проблемой — ведь их фиксы тут проверят, и таск пройдет дальше.
В Гугле советуют не отвлекаться от фокусировки над своей задачи, но если уж отвлеклись, то просмотреть, нет ли чего-то поревьювить. Например, вернулся с обеда — поревьювил. Пришел на работу — поревьювил. И т.д.
Порядок просмотра MR
Сначала надо просмотреть MR в целом. Надо ли это вообще, в правильном ли месте код (или это должно быть в отдельной библиотеке), есть ли какие-то глобальные проблемы.
Т.е. нет смысла смотреть какие-то детали реализации, если код вообще не туда и не для того.
Вообще, порядок просмотра важен, чтобы как можно быстрее выдавать автору фидбек (смотри предыдущий пункт).
Когда посмотрели MR вцелом, надо пройтись по основным файлам, т.е. проверить самое главное (если непонятно, что самое главное, можно спросить у разработчика).
Опять же, о любых проблемах нужно сообщать сразу, даже если вы еще не досмотрели MR и решили вообще досмотреть его позже.
Далее надо выбрать логичную последовательность просмотра оставшихся файлов. Ни один файл не должен быть пропущен.
На что обращать внимание при просмотре
- Код хорошо спроектирован
- Функциональность хорошо сделана с точки зрения пользователей этого кода, кем бы они ни были.
- Внешний вид (если есть) должен быть хорошим
- Учтены все нюансы параллельного программирования (если есть).
- Код не переусложнен
- Разработчик не оверинженирит: не нужно писать код, который может понадобиться, а может не понадобиться
- У кода есть тесты
- Тесты хорошо спроектированы
- Наименования (для всего) выбраны хорошо
- Комментарии к коду понятны и необходимы. Они должны объяснять, почему так сделано, а не как это сделано.
- Добавлена документация.
- Код соответствует стайл гайдам.
Слишком большие MR
Слишком большие MR нужно просить разбивать на части. Почти всегда это возможно.
Как писать комментарии на код ревью
- Нужно быть вежливым, не переходить на личности. Обсуждать код, а не кодера.
- Не просто выдавать директивы к исправлениям, а объяснять, почему нужно исправить.
- Соблюдать баланс: обозначить проблему и подтолкнуть разработчика, чтобы он сам понял, как лучше ее решить; или же сразу выдать готовое решение. Первое развивает разработчика (стратегическая выгода), второе улучшает и ускоряет MR (тактическая выгода).
- Если ревьювер не понял какой-то момент в коде и просит автора объяснить, что к чему, то лучшим ответом будет изменение кода. Так, чтобы было по коду все было понятно без вопросов.
Если с вашим мнением не согласны
Нужно разобраться детально. Возможно, вы чего-то не понимаете, запросите больше информации. Возможно наоборот. В любом случае, зачастую понимание приходит только после пары раундов объяснений с обеих сторон.
Боязнь расстроить автора MR
Бывает, что разработчик расстраивается, если ревьювер настаивает на каких-то изменениях. Однако, чаще всего разработчики расстраиваются из-за формы замечания, а не из-за содержания. Будьте вежливы, объясняйте аргументами, и скорее всего всё будет ок.
"Я исправлю это потом"
Если разработчик согласен с тем, что в коде есть проблема, но просит заапрувить MR, обещая, что исправит это в другой раз, то, по мнению ребят из Гугла, это "потом" чаще всего никогда не наступает. Поэтому если MR — не какой-то срочный багфикс прода, то нужно настаивать на исправлении.
Выводы
Мне очень понравился этот документ от Google. Особенно лайфхак со словом "nit", акцент на скорости code review, а также то, что MR не должен быть идеальным. Вроде бы просто, но пока это явно не обдумаешь, до дела не доходит.
Если эта статья покажется интересной читателям и наберет какое-то количество плюсов, то я напишу следующую часть: процесс code review со стороны автора MR.
Update: вторая часть статьи здесь
Больше полезного можно найти на telegram-канале о разработке "Cross Join", где мы обсуждаем базы данных, языки программирования и всё на свете!
