Еще раз о Code Review

Не так давно сидел я делал ревью кода одного из коллег. Это было не первое мое ревью, но в этот раз я задался вопросом как все таки формализовать подход и на что конкретно стоит обращать внимание и как аргументировать и формулировать предложения и замечания. Сформулировал я для себя вот такие пункты:

Соблюдение принципов SOLID

На всякий случай напомню, SOLID - это аббревиатура для 5 основных принципов ООП:

Single Responsibility Principle - принцип единой ответственности. Этот принцип говорит, что для класса должна быть определена единственная ответственность. Или еще его иногда формулируют как “для внесения изменений в класс должна быть только одна причина”

Open Closed Principle - принцип открытости-закрытости. Этот принцип говорит, что программные сущности должны быть открыты для расширения, но закрыты для модификации.

Liskov Substitution Principle - принцип подстановки Барбары Лисков. Роберт Мартин формулирует этот принцип так: “Функции, которые используют базовый тип, должны иметь возможность использовать подтипы базового типа, не зная об этом.”

Interface Segregation Principle - принцип разделения интерфейсов. Этот принцип говорит, что “много интерфейсов специального назначения лучше, чем один интерфейс общего назначения”.

Dependency Inversion Principle - принциц инверсии зависимостей. Этот принцип говорит, что абстракции не должны зависеть от реализаций, наоборот, реализации должны зависеть от абстракций.

Отсутствие дурных запахов

Обычно выделяют около 20 запахов и в идеале бы, конечно, знать и отслеживать их все. Их все я здесь перечислять не буду, но вот наиболее часто встречающиеся и сильнее других бросающиеся в глаза:

• Его величество дублирование кода

• Магические числа

• Операторы типа switch

• Длинный метод

• Длинный список параметров

• Неинформативные имена переменных

Дурные запахи (не все) и принципы SOLID взаимосвязаны и иногда наличие запаха может сигнализировать о нарушении одного из принципов, к примеру дублирование кода может говорить о нарушении принципа единой ответственности. Однако, на мой взгляд все равно стоит оставлять комменты и о запахе и о нарушении.

Правильное использование языка

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

Использовать литеральную форму задания словаря, вместо циклов, когда это возможно:

chars = ['a', 'b', 'c']

# Bad
d = {}
for i in range(len(chars)):
  d[i] = chars[i]

# Good
d = {i: char for i, char in enumerate(chars)}

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

Использовать подходящие методы встроенный типов:

colors_codes = {
  'red': '#FF0000',
  'green': '#008000'
}
white_name = 'white'
white_code = '#FFFFFF'

# Bad
if white_name not in colors_codes:
  colors_codes[white_name] = white_code
print(colors_codes[white_name])

# Good
print(colors_codes.setdefault(white_name, white_code))

Использовать менеджеры контекста:

#Bad
...
fin = open(path, 'rt')
text = fin.read()
print(text)
...

# Good
...
with open(path) as fin:
  text = fin.read()
  print(text)
...

и т.д.

Покрытие кода тестами

В этом пункте проверяю не количество тестов, а их качество: есть ли тесты для граничных случаев и есть ли непокрытые кейсы.

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

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