SOLID, паттерны, слои архитектуры, зависимости, модульность
Архитектурная ошибка отличается от обычной тем, что её нельзя исправить в следующем PR. К моменту, когда она станет очевидна, на ней будет стоять сорок файлов.
Вы научитесь видеть в тридцати строках диффа решение, влияющее на всю систему: неверное направление зависимости, протёкший слой, размытую границу модуля. И формулировать архитектурное возражение так, чтобы оно не выглядело вкусовщиной, — через сценарий будущего изменения, а не через название принципа.
«Это нарушает SRP» — худшая формулировка архитектурного замечания. Она непроверяема: у автора своё представление об ответственности, у вас своё, и спор становится терминологическим.
Работает другая форма — назвать конкретное будущее изменение и показать его цену:
❌ «Здесь нарушение Single Responsibility»
✅ major: класс держит и SQL-запросы, и отправку письма. Когда мы
будем менять шаблонизатор писем — а это в планах на квартал —
придётся править файл, покрытый тестами на работу с БД,
и поднимать базу, чтобы проверить письмо. Предлагаю разделить.Второе можно обсуждать: автор либо согласится, либо скажет «шаблонизатор мы не меняем, а класс живёт две недели до удаления» — и это будет достойный аргумент.
Отсюда общий принцип архитектурного ревью: вы оцениваете не соответствие принципам, а стоимость будущих изменений. Принципы — сокращённая запись накопленного опыта о том, какие изменения дороги, но в замечании должно быть само изменение.
Проверьте себя. Возьмите архитектурное замечание, которое собирались написать, и переформулируйте через конкретный сценарий правки.
Частая ошибка. Требовать соблюдения принципа в коде, который заведомо просуществует месяц. Абстракция тоже стоит денег.
Из всех архитектурных свойств направление зависимостей заметнее всего в диффе: оно видно по одной строке импорта.
Правило: зависимости направлены в сторону стабильного. Доменная логика не зависит ни от чего, инфраструктура зависит от домена, транспорт — от прикладного слоя. Обратные стрелки — самый дорогой вид долга, потому что они делают ядро системы нетестируемым.
# ❌ домен знает про инфраструктуру: расчёт цены нельзя протестировать
# без БД, а логику скидок — без Redis
# domain/pricing.py
from infrastructure.redis_client import redis
from infrastructure.db import session
def calculate_price(order_id: int) -> Decimal:
order = session.query(Order).get(order_id)
rate = redis.get("usd_rate")
...# ✅ домен принимает данные и абстракции, ничего не знает об источниках
# domain/pricing.py
def calculate_price(order: Order, usd_rate: Decimal) -> Decimal:
...
# application/use_cases.py
def get_order_price(order_id: int, orders: OrderRepo, rates: RateProvider) -> Decimal:
return calculate_price(orders.get(order_id), rates.usd())Второй вариант тестируется без единого мока: calculate_price — чистая функция. Это практический признак правильного направления зависимостей — тест на бизнес-правило не требует поднимать инфраструктуру. Если требует, стрелка направлена не туда, независимо от того, как названы папки.
Что искать в диффе: импорт слоя инфраструктуры в файле домена; обращение к request, session, os.environ внутри доменной функции; вызов ORM в модуле с бизнес-правилами.
Проверьте себя. Откройте самый важный доменный модуль проекта и посмотрите на список его импортов. Что там лишнее?
Частая ошибка. Считать вопрос решённым, потому что папки называются domain и infrastructure. Имя папки не создаёт границу — её создаёт отсутствие импортов.
Второе по частоте — обход слоя. В диффе выглядит как безобидное сокращение пути.
# ❌ контроллер обращается к БД напрямую
@app.get("/orders/{order_id}")
def get_order(order_id: int):
row = db.execute("SELECT * FROM orders WHERE id = %s", (order_id,)).fetchone()
return {"id": row.id, "total": row.total}Сам по себе такой код работает и даже короче правильного. Цена возникает потом: правило «отменённые заказы не показываем» появится в сервисном слое, а этот эндпоинт его не применит. Так рождаются баги, которые невозможно объяснить, — одна и та же сущность ведёт себя по-разному в двух местах.
# ✅ путь через прикладной слой, правила применяются один раз
@app.get("/orders/{order_id}")
def get_order(order_id: int, service: OrderService = Depends()):
order = service.get_visible_order(order_id, current_user)
return OrderResponse.from_domain(order)Отдельный вид протечки — когда наружу уезжает внутренняя модель:
# ❌ ORM-модель возвращается как ответ API
return order # все поля таблицы, включая internal_notes и cost_priceЗдесь схема БД становится публичным контрактом: добавили колонку — изменили API, переименовали — сломали клиентов. Явная схема ответа отделяет одно от другого, и это не бюрократия, а единственный способ менять таблицы, не выпуская ломающих изменений.
Проверьте себя. Найдите в проекте эндпоинт, возвращающий ORM-модель напрямую, и перечислите поля, которые вы не собирались публиковать.
Частая ошибка. Разрешать обход слоя «для простого чтения». Простое чтение — ровно то место, где потом обнаруживается несогласованность правил.
Признак размытой границы — одна модель, обслуживающая несколько контекстов.
# ❌ один класс на все случаи жизни
class User:
email: str
password_hash: str # нужно аутентификации
card_token: str # нужно биллингу
delivery_address: str # нужно доставке
warehouse_notes: str # нужно складуКаждый модуль тянет за собой поля, которые ему не нужны, и любое изменение задевает всех. Через год такой класс правят пять команд, и никто не знает, кто читает warehouse_notes.
# ✅ у каждого контекста своё представление, связанное идентификатором
# auth/models.py
class Account: id: UserId; email: str; password_hash: str
# billing/models.py
class Customer: id: UserId; card_token: str; plan: Plan
# shipping/models.py
class Recipient: id: UserId; address: AddressДублирование UserId здесь не проблема, а плата за независимость: биллинг меняет свои поля, не согласуя с доставкой.
Второй признак — циклическая зависимость модулей. Она означает, что граница проведена не там: два модуля на самом деле один, либо между ними не хватает третьего.
Проверьте себя. Постройте граф импортов проекта (pydeps, madge) и найдите циклы.
Частая ошибка. Разрывать цикл отложенным импортом внутри функции. Цикл остаётся, просто перестаёт падать при загрузке.
Принципы полезны не как критерий соответствия, а как готовые вопросы.
| Принцип | Вопрос к диффу | Что настораживает |
|---|---|---|
| SRP | Сколько разных причин заставит менять этот файл? | БД, HTTP и форматирование в одном классе |
| OCP | Что придётся править при добавлении нового типа? | if/elif по типу, растущий с каждой фичей |
| LSP | Все реализации действительно взаимозаменяемы? | Наследник бросает NotImplementedError |
| ISP | Всем ли клиентам нужны все методы интерфейса? | Реализации с пустыми заглушками |
| DIP | На что указывает импорт — на абстракцию или на реализацию? | Домен импортирует драйвер |
Самый практичный из них в ревью — OCP, потому что он проверяется механически:
# ❌ каждый новый тип оплаты — правка этой функции и риск для существующих
def process(payment_type: str, amount: Decimal):
if payment_type == "card":
...
elif payment_type == "sbp":
...
elif payment_type == "crypto":
...Если в диффе к такой цепочке добавляется ещё одна ветка — третья или четвёртая по счёту, — это уместный момент для замечания. Не первая: абстракция, введённая при двух вариантах, обычно оказывается неверной, потому что закономерность ещё не видна.
Проверьте себя. Найдите в проекте самую длинную цепочку elif по типу и посмотрите в истории, сколько PR её касались.
Частая ошибка. Вводить полиморфизм при двух вариантах. Правило «три» существует не случайно: по двум точкам закономерность не определяется.
Задача: подписка на снижение цены товара и рассылка уведомлений. Дифф на 70 строк, три новых файла. CI зелёный, тесты есть.
+# domain/price_watch.py
+import requests
+from infrastructure.db import session
+from infrastructure.mailer import send_email
+
+
+class PriceWatcher:
+ def check_and_notify(self, product_id: int):
+ product = session.query(Product).get(product_id)
+ watches = session.query(PriceWatch).filter_by(product_id=product_id).all()
+
+ rate = requests.get("https://api.rates.example/usd").json()["rate"]
+ price_rub = product.price_usd * rate
+
+ for watch in watches:
+ if price_rub < watch.target_price:
+ send_email(watch.user.email, f"Цена снижена: {price_rub} ₽")
+ watch.notified_at = datetime.now()
+ session.commit()
+
+
+# api/products.py
+@app.post("/products/{product_id}/check-price")
+def check_price(product_id: int):
+ PriceWatcher().check_and_notify(product_id)
+ return {"ok": True}Что видит ревьюер. Код работает. Тест, приложенный к PR, поднимает базу, подменяет requests и send_email тремя моками и проверяет, что письмо отправлено. Само наличие трёх моков в тесте на бизнес-правило — сигнал.
Проследим зависимости. Файл в domain/ импортирует requests, сессию БД и почтовый клиент. То есть доменный модуль зависит от трёх инфраструктурных, и правило «уведомляем, когда цена ниже целевой» невозможно проверить, не подняв три подсистемы. Стрелка направлена наружу вместо внутрь.
Дальше — смешение ответственностей в одном методе: получение данных, обращение к внешнему API за курсом, конвертация валюты, решение об уведомлении, отправка письма, фиксация факта отправки. Шесть причин для будущих правок в одиннадцати строках.
И архитектурный вопрос, который важнее всех замечаний по структуре: почему это HTTP-эндпоинт. Кто его вызывает? Если внешний планировщик — значит, у нас публичный эндпоинт, запускающий рассылку, без аутентификации и без защиты от повторного вызова.
Комментарии в PR:
api/products.py:2· blocker Эндпоинт запускает рассылку писем, не требует аутентификации и не защищён от повторных вызовов. Любой желающий отправит нашим пользователям столько писем, сколько захочет. Кто должен это вызывать? Если планировщик — это задача воркера, а не публичный HTTP.
domain/price_watch.py:1· major Доменный модуль импортируетrequests, сессию БД и почтовый клиент. Из-за этого правило «уведомляем при цене ниже целевой» тестируется только с тремя моками и поднятой базой — что и видно в приложенном тесте.Предлагаю разделить: чистая функция
should_notify(watch, price) -> boolв домене, получение данных и отправка — в прикладном слое. Тест на правило станет тремя строками без моков.
domain/price_watch.py:12· major Курс валюты берётся синхронным запросом без таймаута и без обработки сбоя. Если сервис курсов недоступен, рассылка падает целиком. Курс стоит получать через абстракцию и передавать значением — заодно уйдёт зависимость отrequestsв домене.
domain/price_watch.py:18· majorsession.commit()внутри цикла: если письмо третьему пользователю не отправится, первые двое уже отмечены как уведомлённые, а остальные — нет, и повторный запуск разошлёт письма заново тем, кому не дошло. Нужно решить, что здесь важнее — не потерять или не задублировать, — и написать это явно.
domain/price_watch.py:17· minorwatch.user.emailв цикле — N+1 по пользователям. Загрузите вместе с подписками.
domain/price_watch.py:14· question Что должно произойти, если цена снизилась дважды? Сейчасnotified_atперезаписывается, и письмо уйдёт повторно на каждый запуск, пока цена ниже цели. Это задумано?
Чем закончилось. Обсуждение по последнему вопросу оказалось самым содержательным: выяснилось, что продуктового решения не было вообще, и «уведомить один раз» приняли уже в ходе ревью. Итоговая структура:
# domain/price_watch.py — ни одного инфраструктурного импорта
def should_notify(watch: PriceWatch, price_rub: Decimal) -> bool:
return price_rub < watch.target_price and watch.notified_at is None
# application/notify_price_drops.py
def notify_price_drops(product_id: int, deps: Deps) -> None:
product = deps.products.get(product_id)
watches = deps.watches.for_product(product_id, with_users=True)
price_rub = product.price_usd * deps.rates.usd() # один раз, с таймаутом
to_notify = [w for w in watches if should_notify(w, price_rub)]
for watch in to_notify:
deps.mailer.send(watch.user.email, price_drop_letter(price_rub))
deps.watches.mark_notified([w.id for w in to_notify]) # одной операциейВместо HTTP-эндпоинта — задача воркера. Тест на should_notify — четыре параметризованных случая без единого мока; ровно то, чего не хватало.
Обратите внимание, что дал архитектурный разбор помимо структуры: обнаружились публичный эндпоинт рассылки, отсутствие таймаута, риск двойной отправки и незакрытое продуктовое решение. Все они — следствия того, что слои были перемешаны: когда всё в одном методе, ни один из этих вопросов не задаётся.
СТРЕЛКИ импорты домена не указывают на инфраструктуру
ТЕСТИРУЕМОСТЬ бизнес-правило проверяется без БД, сети и моков
СЛОИ ни один слой не обойдён «для простоты»
МОДЕЛИ наружу уходит явная схема, а не ORM-сущность
ГРАНИЦЫ у каждого контекста своя модель; циклов между модулями нет
OCP новый тип не требует правки существующего ветвления
ЦЕНА для каждого замечания назван будущий сценарий изменения
СРОК абстракция соразмерна ожидаемой жизни кодаКак отличить оправданный паттерн от преждевременной абстракции — Паттерны проектирования. Как измерять то, что обсуждалось качественно, — Метрики качества кода. Тренировка: Code Review React → Архитектура компонентов, Code Review Python → Антипаттерны ООП.
Ключевая мысль: архитектурное замечание проверяется вопросом «какое будущее изменение станет дешевле». Если ответа нет, это вкусовщина, даже если названа принципом.
Далее: Паттерны проектирования в review