Еще раз о 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 принципы конечно же не для всех случаев актуальны, да и с некоторыми запахами в определенных ситуациях можно не бороться. Пункты выше относятся к проектам, где они имеют смысл.