Не так давно сидел я делал ревью кода одного из коллег. Это было не первое мое ревью, но в этот раз я задался вопросом как все таки формализовать подход и на что конкретно стоит обращать внимание и как аргументировать и формулировать предложения и замечания. Сформулировал я для себя вот такие пункты:
Соблюдение принципов 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 принципы конечно же не для всех случаев актуальны, да и с некоторыми запахами в определенных ситуациях можно не бороться. Пункты выше относятся к проектам, где они имеют смысл.
Комментарии (16)
lizergil
08.09.2021 12:45-2За многие годы я пришёл к очень простому пониманию: этот автомобиль способен производить среднее middle предприятие и понять как он устроен(при условии что это вообще применимо)? Если да, то этого достаточно. Не никакого смысла тратить кучу времени и создавать напряжённости в команде полируя одноразовый автомобиль. Нет никакой проблемы вообще. Если автомобиль окажется плохим, вы это увидите потом, сделаете новый. Более того, практику поголовынх ревью и повышение качества просто потому что кому-то не нравится, можно выразить перефразированной очень популярной цитатой:
Делать людям хорошо — себе дороже
Vasily_T
08.09.2021 13:49+1За прошедшие годы я осознал достаточно простую вещь: способны ли большинство разработчиков в команде прочесть код и осознать что он делает? Если да, то этого вполне достаточно. Такая полировка кода бессмысленна и вообще может создать нежелательное напряжение в команде. Проблемы нет. Если код окажется некачественным, это обнаружится потом и будет отрефакторено. Поголовное ревью и рефакторинг, только потому что кому-то код не нравится, можно выразить популярной фразой как про преждевременную оптимизацию, только про преждевременный рефакторинг.
tzlom
08.09.2021 16:50+4# Good d = {i: char for i, char in enumerate(chars)}# Very Good d = dict(enumerate(chars))
sshikov
08.09.2021 17:55+1Соблюдение принципов SOLID
На всякий случай напомню, SOLID — это аббревиатура для 5 основных принципов ООП:
Во-первых, принципы SOLID не ограничены ООП. Они практически все применимы и к другим парадигмам. Но это ерунда. Не ерунда, то, ради чего все это затевается. Требовать соблюдения принципов — зачем?
А вот зачем (вики):Для чего нужны принципы SOLID
При создании программных систем использование принципов SOLID способствует созданию такой системы, которую будет легко поддерживать и расширять в течение долгого времени[3].
Ну т.е. принципы эти призваны создать такой код, который будет потом просто расширять и поддерживать. Прежде чем требовать этого на ревью (ну или проверять соблюдение), хорошо бы понимать, что для проекта это реально важно. И не только вообще, а вот в этом конкретном месте.
А если это не нужно, то требовать применения принципов — все равно что требовать использования паттернов проектирования (которые являются способами решения типовых задач, и в отсутствии самих задач не имеют смысла). И многие очевидно видели, к чему приводит такое бездумное применение.
Aquahawk
За многие годы я пришёл к очень простому пониманию: этот код способен прочесть средний middle разработчик и понять что он делает(при условии что это вообще применимо)? Если да, то этого достаточно. Не никакого смысла тратить кучу времени и создавать напряжённости в команде полируя одноразовый код. Нет никакой проблемы вообще. Если код окажется плохим, вы это увидите потом, отрефакторите. Более того, практику поголовынх ревью и рефакторинг просто потому что кому-то не нравится, можно выразить перефразированной очень популярной цитатой:
mjr27
Могу разве что добавить, что код ревью несут в том числе воспитательную цель. От "тыканья носом" в код с не сильно плохим запахом разработчик чему-то научится? Нет? Ну и фиг с ним тогда.
vyatsek
Полностью согласен, на ревью я прежде всего смотрю, а не поломается ли что-то, если замержат этот код, нет ли каких-то дефектов.
SOLID обычно проверяется на design review, когда раскидывают объекты.
Anamelash
Ох уж это "потом отрефакторите". Обычно это означает "и так сойдет".
Aquahawk
Да. Ровно также как и с оптимизациями. Можно оптимизировать миллион мест и это принесёт но 0 прибыли. Всегда должен быть допустимый уровень погрешности. Нет смысла точить колёся поезда с допусками поршня. Нет смысла пилить одноразовый код с требованиями ядра хай перфоманс системы.
Anamelash
Если можно написать хорошо, то зачем писать плохо?
Aquahawk
Правки на ревью это не про написать. Это про переписать работающий код. Это про вложить ещё денег, сил, сроков в чьи то хотелки по оформлению кода вместо того чтобы вложить их в продукт. И принятие этого решения должно быть очень взвешенным и ориентированным на текущие бизнес потребности проекта.
Anamelash
Одна из потребностей бизнеса это создание понятного и поддерживаемого кода. Ибо как ни крути, но будут приходить новые разработчики которым придется поддерживать кодовую базу.
Даже старым разработчикам будет гораздо проще поддерживать код, который написан правильно.
Aquahawk
Вы очень верно употребили термин "одна из потребностей". Есть и другие. И их нужно взвешивать. Я ни в коем случае не говорю что ревью и рефакторинги это плохо. Это инструмент. И иногда он уместен, а иногда нет. Часто можно услышать позицию возведения "качества кода" в абсолют в угоду другим параметрам продукта. И именно гиперконцентрация на этом часто вылазит боком. Очень часто скорость проверки гипотезы важнее последующей сопровождаемости, потому что 9 попыток из 10 уйдут в урну. Еще раз. Качеством кода можно и нужно управлять. Это хорошо. Сказать что соблюдение неких кодстайлов или паттернов есть критерий качества - хуже. Возвести это в абсолют - ужасно. Как пример могу предложить рассмотреть стандариты кодирования НАСА. Очень надёжный код получается. Но никому на рынке не выжить с таким подходом.
SadOcean
Я стал смотреть больше не на сами компоненты, а на то, как они встраиваются и понятна ли общая канва.
Раз за разом ситуация, что классики еще нормально полируют (проще искать ключи под фонарем), модули и системы - похуже, а на уровне проекта треш, угар и непонятные механизмы, управляющие центральными состояниями системы.
Хотя должно быть наоборот - напиши аккуратное встраивание, и что будет в конкретной реализации - почти пофиг, при переписывании ее выкинут и воткнут новую.
Aquahawk
А вот это верно. Архитектура важнее мелочей по оформлению.