Чек-лист код-ревью

Я считаю, что каждый разработчик, независимо от своей позиции должен делать code review других разработчиков, чтобы учиться. Благодаря этому он сможет учиться отличать плохой код от хорошего, перенимать хорошие практики более опытных разработчиков и разбираться во всех частях проекта, а не только в тех, где он сам пишет код.

Но когда прилетает Pull Request (Merger Request) я всегда начинаю судорожно вспоминать, что же надо проверять в рамках ревью, чтобы что-то не упустить. Чек-листы сильно упрощают этот процесс. Поэтому я подготовил детальный чек-лист, который можно будет скачать, распечатать и иметь перед глазами. Но сначала разберём его.

Перед ревью

Если PR большой, лучше всего скачать проект, установить зависимости, перейти в нужную ветку и начать смотреть код в привычной вам IDE. Если он небольшой - тогда не страшно, можно его смотреть хоть в браузере. Так же ознакомьтесь с задачей. Без понимания самой задачи дальнейшее ревью будет усложнено, так как вы не сможете понять зачем тут тот или иной код. Потратьте время, прочитайте техническое задание и после этого можно приступать к ревью, проверяя, каждый кусок на соответствие требованиям.

Стиль кода

Этот пункт должен сводиться к минимуму, так как за это должны отвечать инструменты линтинга. Перед ревью нужно, чтобы lint был успешно пройден и правки по стилю кода могли сводиться только к тому, что не может быть покрыто правилами.

Название переменных, функций, классов

Убедитесь, что все переменные названы по стилю, который у вас оговорен в компании и соответствует содержащимся там данным. Нет бесполезных x, a, p, которые не несут смысла. Все названия функций соответствуют тому, что они делают, а классы отражают реальные бизнес объекты.

Читаемость кода

Код должен читаться легко. Если в коде много сложных map/reduce, от которых можно было избавиться - надо исправлять. Если функция сложная и занимает 100 строк - надо разбивать на небольшие понятные функции. Если логика усложнена и можно было реализовать проще - надо переписывать.

Соответствие требованиям

Тут нужно убедиться, что реализованный функционал соответствует требованиям задачи. Возможно исполнитель мог упустить какой-то важный момент или наоборот добавить что-то не нужное, чего в требованиях нет.

Комментарии в коде

Комментарии должны быть не избыточны и пояснять предметную область или неочевидные вещи, которые без комментариев понять невозможно. Так же во всех контрактах и бизнес объектах должен быть комментарий с соответствием бизнес термину.

enum PayApiStatus {
    /** @summary если платёж принят, но не выполнен */
    Accepted = 1
}

Повторное использование

Всегда нужно придерживаться принципа DRY (don't repeat yourself). Не должно быть повторяющихся кусков кода. Так же функции, которые могут в дальнейшем использоваться повторно должны быть спроектированы соответствующим образом. Если такие функции уже были ранее в коде, необходимо проверить, что в PR не изобретает велосипед, а использует то, что уже наработано.

Поддержка разных окружений

Конфигурации окружений не должны быть в коде. Код должен уметь работать не только на разных окружениях (develop / production), но и иметь несколько instance, так как код может быть горизонтально масштабироваться.

Производительность

Код выносит все сложные вычисления в отдельные процессы и не блокирует выполнение приложения. В коде нет узких мест, которые при росте данных или нагрузки буду давать просадку в производительности.

Тесты

Тесты должны соответствовать вашей договорённости о покрытии. Если какой-то функционал содержит сложную логику, следует иметь для него unit тесты.

Скачать в виде чек-листа можно по ссылке.