Паттерны проектирования в review — Code Review Pro

Паттерны проектирования в Code Review

Паттерн — это ответ. В ревью первым делом проверяется, был ли задан вопрос.

#Результат урока

Вы научитесь отличать паттерн, снимающий реальную сложность, от паттерна, добавляющего её; узнавать преждевременную абстракцию по признакам в диффе; и — самое трудное — формулировать замечание «здесь не нужен паттерн» так, чтобы оно не звучало как «мне не нравится».

#1. Единственный критерий: что стало дешевле

Паттерн всегда добавляет косвенность: вместо одного места, где что-то происходит, появляются интерфейс, реализации и точка сборки. Это плата. Она оправдана, только если что-то стало дешевле — обычно одно из трёх:

добавление варианта без правки существующего кода;
тестирование без поднятия инфраструктуры;
чтение — сложное ветвление заменено явными именами.

Если ни одно из трёх не улучшилось, паттерн — чистый расход.

# ❌ Strategy на два случая, которые не растут
class DiscountStrategy(ABC):
    @abstractmethod
    def apply(self, amount: Decimal) -> Decimal: ...

class NoDiscount(DiscountStrategy):
    def apply(self, amount): return amount

class TenPercent(DiscountStrategy):
    def apply(self, amount): return amount * Decimal("0.9")

# ✅ то же поведение, читается сразу
def apply_discount(amount: Decimal, is_premium: bool) -> Decimal:
    return amount * Decimal("0.9") if is_premium else amount

Три файла и абстрактный класс против одной строки. Обратный случай — когда вариантов действительно много и они приходят извне:

# ✅ здесь Strategy оправдан: правила скидок задаются маркетингом,
#    новые появляются каждый месяц, каждое тестируется отдельно
RULES: dict[str, DiscountRule] = {
    "loyalty": LoyaltyDiscount(),
    "first_order": FirstOrderDiscount(),
    "promo_code": PromoCodeDiscount(),
    "bulk": BulkDiscount(),
}

Разница не в размере кода, а в ответе на вопрос «что произойдёт при добавлении пятого правила». В первом примере — ничего, потому что пятого не будет. Во втором — новый класс и новая строка в словаре, без правки существующих.

Проверьте себя. Для каждой абстракции в открытом PR назовите, что именно стало дешевле. Если не получается — это кандидат на замечание.

Частая ошибка. Считать наличие паттерна признаком зрелого кода. Признак зрелости — соответствие сложности решения сложности задачи.

#2. Преждевременная абстракция

Самая частая находка в ревью senior-уровня — интерфейс с одной реализацией, введённый «на будущее».

# ❌ абстракция «на случай, если понадобится XML»
class Serializer(ABC):
    @abstractmethod
    def serialize(self, data: dict) -> str: ...

class JsonSerializer(Serializer):
    def serialize(self, data): return json.dumps(data)

# в проекте только JSON, XML не планируется

Проблема не в лишних строках, а в том, что абстракция, выведенная из одного примера, почти всегда неверна. Когда действительно появится второй формат, окажется, что ему нужен ещё и content_type, и потоковая запись, и интерфейс придётся переделывать — вместе со всеми вызовами. Абстракция, выведенная из двух реальных случаев, лучше абстракции, придуманной из нуля.

Признаки в диффе:

интерфейс с единственной реализацией и без теста, подменяющего её;
параметр конфигурации, у которого всегда одно значение;
фабрика, создающая один и тот же класс;
обобщённый тип, инстанцируемый одним типом;
название реализации повторяет название интерфейса (Serializer / JsonSerializer).

Исключение — абстракция на границе с внешним миром. Интерфейс PaymentGateway с одной реализацией оправдан не «на будущее», а прямо сейчас: он позволяет тестировать без обращения к платёжной системе. Здесь выгода мгновенная, а не гипотетическая.

Проверьте себя. Найдите в проекте интерфейс с одной реализацией и проверьте, подменяется ли он в тестах. Если нет — он не окупается.

Частая ошибка. Требовать удаления абстракции, которая обеспечивает тестируемость. Одна реализация плюс подмена в тестах — это фактически две.

#3. Паттерны, которые чаще всего обсуждаются

Repository. Оправдан, когда прячет способ хранения от прикладного слоя и позволяет тестировать логику на списке в памяти. Вырождается, когда становится тонкой обёрткой над ORM: методы get, save, filter, повторяющие интерфейс Session, добавляют слой, ничего не скрывая.

# ❌ обёртка ради обёртки: ORM просто переименован
class UserRepo:
    def filter(self, **kwargs): return session.query(User).filter_by(**kwargs)

# ✅ репозиторий говорит на языке домена, а не на языке SQL
class UserRepo:
    def find_active_with_expiring_subscription(self, days: int) -> list[User]: ...

Второй вариант скрывает запрос. Первый — просит его написать снаружи, то есть протекает.

Service Layer. Оправдан как место для сценариев, координирующих несколько сущностей. Вырождается в «сервис на каждую модель» с методами create_user, update_user, delete_user — это тот же CRUD, только через два вызова.

Factory. Оправдана, когда создание объекта требует нетривиальных решений: выбор реализации по конфигурации, сборка из нескольких источников, валидация инварианта. Не оправдана, когда просто вызывает конструктор.

Наблюдатель / события. Мощный способ развязать модули и одновременно лучший способ сделать поток управления невидимым. Уместное замечание в ревью: «после публикации этого события что произойдёт? Обработчиков сейчас три, порядок между ними важен?» Событийная связь не видна в стеке вызовов, поэтому её нужно проговаривать в описании PR.

Проверьте себя. Откройте репозиторий вашего проекта и посмотрите, говорят ли его методы на языке домена или на языке запросов.

Частая ошибка. Оценивать паттерн по его каноническому описанию, а не по тому, что он делает в этом коде.

#4. Антипаттерны, которые видно в диффе

God object. Класс, в который добавляют всё новое, потому что «он и так про заказы». Признак в диффе: файл на 1500 строк, и PR добавляет к нему ещё один несвязанный метод. Замечание работает лучше, если оно про границу: «этот метод не про заказ, а про доставку — он не должен знать про расчёт срока».

Анемичная модель. Классы только с данными, вся логика — в сервисах-процедурах.

# ❌ инвариант можно нарушить снаружи
order.status = "shipped"          # хотя заказ не оплачен
order.total = -100                # хотя так не бывает

# ✅ модель защищает свои инварианты
order.mark_shipped()              # внутри проверка, что оплачен

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

Синглтон как способ получить зависимость. Database.instance() внутри бизнес-логики — скрытая зависимость: сигнатура функции о ней не сообщает, тест не может её подменить, порядок инициализации становится неявным.

Наследование ради переиспользования. class ReportService(DatabaseHelper) — способ получить методы, а не выразить «является». Признак: наследник не может быть подставлен вместо предка нигде.

Проверьте себя. Найдите в проекте самый большой класс и определите, сколько разных тем он охватывает.

Частая ошибка. Называть антипаттерн по имени вместо описания последствия. «Это god object» непроверяемо; «правило про статусы будет продублировано» — проверяемо.

#5. Как формулировать «здесь не нужен паттерн»

Это самый конфликтный вид замечания: автор потратил время, спроектировал, и его просят упростить. Помогает три приёма.

Считать стоимость, а не эстетику. «Здесь четыре файла и абстрактный класс на два варианта, которые не растут. Пока вариантов два, if читается быстрее и меняется дешевле».

Оставить путь назад. «Давай начнём с функции, а когда появится третий вариант, вынесем — это будет правка на десять минут, зато абстракция получится из реальных случаев, а не из предположений».

Признать сценарий, если он реален. Иногда автор знает о дорожной карте больше вас. Вопрос «сколько вариантов ожидается в этом квартале?» разумнее утверждения.

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

Проверьте себя. Вспомните случай, когда вы ввели абстракцию, которая не понадобилась. Что было сигналом заранее?

Частая ошибка. Просить упрощения в PR, где абстракция уже написана и покрыта тестами, ради выигрыша в двадцать строк. Цена переделки может превысить выгоду — иногда правильный ответ «согласовано, оставляем».

#6. Разбор: PR «Экспорт в разные форматы»

Задача из тикета: «добавить кнопку „скачать CSV“ в отчёт по продажам». Автор принёс 140 строк в пяти файлах.

+# exporters/base.py
+class BaseExporter(ABC):
+    @abstractmethod
+    def export(self, rows: list[dict]) -> bytes: ...
+
+    @abstractmethod
+    def content_type(self) -> str: ...
+
+    @abstractmethod
+    def file_extension(self) -> str: ...
+
+
+# exporters/csv_exporter.py
+class CsvExporter(BaseExporter):
+    def export(self, rows): ...
+    def content_type(self): return "text/csv"
+    def file_extension(self): return "csv"
+
+
+# exporters/factory.py
+class ExporterFactory:
+    @staticmethod
+    def create(fmt: str) -> BaseExporter:
+        if fmt == "csv":
+            return CsvExporter()
+        raise ValueError(f"unknown format: {fmt}")
+
+
+# exporters/registry.py
+EXPORTER_REGISTRY: dict[str, type[BaseExporter]] = {"csv": CsvExporter}
+
+
+# api/reports.py
+@app.get("/reports/sales/export")
+def export_sales(fmt: str = "csv"):
+    exporter = ExporterFactory.create(fmt)
+    rows = get_sales_rows()
+    return Response(
+        exporter.export(rows),
+        media_type=exporter.content_type(),
+        headers={"Content-Disposition": f'attachment; filename=sales.{exporter.file_extension()}'},
+    )

Что видит ревьюер. Задача — один формат. В коде: абстрактный класс с тремя методами, одна реализация, фабрика, реестр, параметр fmt, который может принимать одно значение, и ValueError для случая, который невозможен.

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

Отдельно — параметр fmt в публичном API. Он уже стал частью контракта, хотя обозначает выбор из одного варианта. Когда появится Excel, окажется, что для него нужен ещё и стиль ячеек, и потоковая запись — а трёхметодный интерфейс придётся ломать вместе с фабрикой.

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

Комментарии в PR:

exporters/registry.py:1 · major Реестр и фабрика решают одну задачу, и реестр нигде не используется. Один из двух механизмов нужно убрать — иначе следующий человек добавит формат в один из них и будет полчаса искать, почему не работает.

exporters/base.py:1 · major Три абстрактных метода на одну реализацию. Предлагаю пока обойтись функцией export_to_csv(rows) -> bytes и явными заголовками в эндпоинте. Когда появится второй формат — а по дорожной карте это Excel в четвёртом квартале, — вынесем интерфейс из двух настоящих случаев. Сейчас мы угадываем: для Excel почти наверняка понадобится потоковая запись, и текущий export(rows) -> bytes для него не подойдёт, придётся переделывать интерфейс вместе с фабрикой.

api/reports.py:2 · major fmt — уже часть публичного контракта, но принимает единственное значение. Если формат один, параметр лучше не вводить: добавить его потом легко, убрать — ломающее изменение.

exporters/factory.py:6 · minor При таком количестве вариантов фабрика — это dict.get. Даже если оставлять, класс со staticmethod здесь не нужен.

exporters/csv_exporter.py:3 · question Как экранируются значения? Если сборка строки руками — в названиях товаров бывают запятые и кавычки, файл поедет. csv.writer закрывает это сам.

Чем закончилось. Автор согласился с тремя major и оставил 18 строк вместо 140:

# reports/export.py
def sales_to_csv(rows: list[SalesRow]) -> bytes:
    buffer = io.StringIO()
    writer = csv.writer(buffer)
    writer.writerow(["Дата", "Товар", "Количество", "Сумма"])
    for row in rows:
        writer.writerow([row.date, row.product, row.qty, row.total])
    return buffer.getvalue().encode("utf-8-sig")


# api/reports.py
@app.get("/reports/sales/export.csv")
def export_sales_csv():
    return Response(
        sales_to_csv(get_sales_rows()),
        media_type="text/csv",
        headers={"Content-Disposition": 'attachment; filename="sales.csv"'},
    )

Вопрос про экранирование оказался настоящим багом: значения собирались через join(","), и любой товар с запятой в названии ломал файл. csv.writer закрыл это, а utf-8-sig заодно решил проблему с кириллицей в Excel — о ней вспомнили в том же треде.

Через четыре месяца, когда действительно понадобился Excel, интерфейс вынесли — и он получился другим: с потоковой записью в переданный буфер, а не с возвратом bytes. То есть первоначальная абстракция была бы неверной.

#7. Чек-лист

ВЫГОДА       названо, что стало дешевле: добавление варианта, тест или чтение
СЧЁТ         абстракция выведена минимум из двух реальных случаев
ГРАНИЦА      интерфейс с одной реализацией оправдан только на границе с внешним миром
ДУБЛИ        нет двух механизмов, решающих одну задачу
КОНТРАКТ     параметры «на будущее» не попали в публичный API
ИНВАРИАНТЫ   правила живут в модели, а не размазаны по сервисам
ЗАВИСИМОСТИ  явные в сигнатуре, без синглтонов внутри логики
СОРАЗМЕРНОСТЬ сложность решения соответствует сложности задачи

#Что дальше

Направление зависимостей и границы модулей — Архитектурный анализ. Когда упрощение стоит отдельного тикета — Возможности рефакторинга. Тренировка: Code Review Python → Антипаттерны ООП.

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