Skip to content

Commit e6ae859

Browse files
MKTorontoMKToronto
committed
updates
1 parent 84c3409 commit e6ae859

2 files changed

Lines changed: 454 additions & 377 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 377 deletions
Original file line numberDiff line numberDiff line change
@@ -74,383 +74,7 @@ The review checks:
7474

7575
Findings are reported by severity (Critical / Important / Suggestions) with file/line references and fix snippets.
7676

77-
**Example output** (from reviewing a test `bookstore-api/` project). See the [color-coded version](https://mktoronto.github.io/python-clean-architecture/) on the landing page.
78-
79-
```text
80-
$ /review-architecture bookstore-api/
81-
82-
Architecture Summary
83-
----------------------
84-
85-
Layers
86-
------
87-
API: routers/books.py, routers/orders.py
88-
Logic: services/book_service.py, services/order_service.py
89-
Database: db/database.py (models + session factory)
90-
Models: models/book.py, models/order.py (Pydantic)
91-
Unclear: utils/helpers.py (formatting + DB queries -- mixed concerns)
92-
93-
Dependency Flows
94-
----------------
95-
routers/books.py -> services/book_service.py -> db/database.py [ok] (no abstraction)
96-
routers/orders.py -> services/order_service.py -> db/database.py [ok] (no abstraction)
97-
routers/orders.py -> db/database.py [!] layer skip
98-
utils/helpers.py -> db/database.py [!] mixed concerns
99-
100-
Layer Violations
101-
----------------
102-
[!] routers/orders.py:2 imports SessionLocal + OrderModel directly
103-
[!] routers/orders.py:21 performs raw DB query inside an endpoint
104-
[!] utils/helpers.py:1 imports DB layer -- utility module should not touch persistence
105-
[!] No DataInterface Protocol exists -- all services hardcode SessionLocal()
106-
107-
What Works Well
108-
----------------
109-
[ok] Good Pydantic model separation -- BookCreate / Book and OrderCreate / Order
110-
properly separate input from output (models/book.py, models/order.py)
111-
[ok] Clean main.py -- simple composition with tagged routers, no business logic
112-
[ok] Consistent router structure -- proper APIRouter() with prefix/tag organization
113-
[ok] Book service functions are individually small -- each under 10 lines
114-
115-
Findings by Severity
116-
----------------------
117-
118-
Critical
119-
120-
1. Layer skip -- router queries DB directly
121-
routers/orders.py:2,19-32
122-
Principles: P3 Depend on Abstractions, P2 Low Coupling
123-
124-
The read_order endpoint imports SessionLocal and OrderModel, then runs
125-
raw SQLAlchemy queries inside the router. Bypasses the service layer.
126-
127-
# Before (routers/orders.py:19-32)
128-
from db.database import SessionLocal, OrderModel
129-
130-
@router.get("/{order_id}")
131-
def read_order(order_id: int):
132-
db = SessionLocal()
133-
order = db.query(OrderModel).filter(OrderModel.id == order_id).first()
134-
db.close()
135-
...
136-
137-
# After -- delegate to service, raise proper HTTP errors
138-
from fastapi import HTTPException
139-
140-
@router.get("/{order_id}", response_model=Order)
141-
def read_order(order_id: int):
142-
order = order_service.get_order(order_id)
143-
if not order:
144-
raise HTTPException(status_code=404, detail="Order not found")
145-
return order
146-
147-
2. No dependency injection -- all services hardcode DB access
148-
services/book_service.py:1, services/order_service.py:3
149-
Principle: P3 Depend on Abstractions
150-
151-
Every function creates SessionLocal() internally. No DataInterface
152-
Protocol exists. Testing requires a real database.
153-
154-
# Before (services/book_service.py:1,5-6)
155-
from db.database import SessionLocal, BookModel
156-
157-
def create_book(data: BookCreate) -> Book:
158-
db = SessionLocal()
159-
book = BookModel(**data.dict())
160-
...
161-
162-
# After -- define a Protocol, inject it
163-
class BookDataInterface(Protocol):
164-
def create(self, data: dict) -> dict: ...
165-
def get_all(self) -> list[dict]: ...
166-
def get_by_id(self, book_id: int) -> dict | None: ...
167-
168-
def create_book(data: BookCreate, data_interface: BookDataInterface) -> Book:
169-
result = data_interface.create(data.model_dump())
170-
return Book(**result)
171-
172-
3. No resource management -- leaked DB connections on exceptions
173-
services/book_service.py:6-11, services/order_service.py (every method)
174-
Principle: P7 Keep Things Simple | Rule: #13 Use Context Managers
175-
176-
Every function does db = SessionLocal() ... db.close(). If any line
177-
between them raises, the connection leaks. OrderService methods have
178-
multiple early return paths each needing their own db.close().
179-
180-
# Before (services/order_service.py:10-14)
181-
db = SessionLocal()
182-
book = db.query(BookModel).filter(BookModel.id == book_id).first()
183-
if not book:
184-
db.close()
185-
return None
186-
187-
# After -- context manager guarantees cleanup
188-
@contextmanager
189-
def get_session():
190-
session = SessionLocal()
191-
try:
192-
yield session
193-
finally:
194-
session.close()
195-
196-
def get_order(self, order_id: int) -> Order | None:
197-
with get_session() as db:
198-
order = db.query(OrderModel).filter(OrderModel.id == order_id).first()
199-
return Order(**order.__dict__) if order else None
200-
201-
Important
202-
203-
4. God class -- OrderService has 7 responsibilities
204-
services/order_service.py:6 -- 177 lines
205-
Principle: P1 High Cohesion
206-
207-
Handles: order CRUD, discount calculation, email notifications,
208-
inventory management, logging, refund processing, and shipping
209-
calculation. The docstring says it: "Handles orders, payments,
210-
inventory, notifications, discounts, and shipping."
211-
212-
Fix: Extract into focused collaborators --
213-
DiscountCalculator, NotificationService, ShippingCalculator,
214-
RefundProcessor. Keep OrderOperations for pure CRUD only.
215-
216-
5. Broad exception catching -- except Exception: pass
217-
services/order_service.py:163-164
218-
Rule: #12 No Broad Exception Catching
219-
220-
_send_email catches Exception and silently discards it. A typo inside
221-
the method, a TypeError, even a KeyboardInterrupt would be swallowed.
222-
Order appears to succeed but email silently fails with no trace.
223-
224-
# Before
225-
def _send_email(self, to, subject, body):
226-
try:
227-
...
228-
except Exception:
229-
pass
230-
231-
# After -- catch specific errors, log failures
232-
def _send_email(self, to: str, subject: str, body: str) -> None:
233-
try:
234-
...
235-
except (smtplib.SMTPException, ConnectionError) as e:
236-
logger.warning("Failed to send email to %s: %s", to, e)
237-
238-
6. Missing type hints on 15+ functions
239-
Multiple files
240-
Rule: Type hints on all function parameters and return types
241-
242-
File Function Missing
243-
------------------------------ ------------------------------- ---------------------
244-
services/book_service.py:15 get_all_books() return type
245-
services/book_service.py:25 get_book(book_id) param + return
246-
services/book_service.py:34 search_books(query) param + return
247-
services/order_service.py:9 create_order(self, ...) all params + return
248-
services/order_service.py:66 get_order(self, order_id) param + return
249-
services/order_service.py:80 update_status(self, ...) all params + return
250-
services/order_service.py:105 cancel_order(self, order_id) param + return
251-
services/order_service.py:133 get_order_history(self, ...) param + return
252-
services/order_service.py:139 calculate_shipping(self, ...) param + return
253-
services/order_service.py:154 _send_email(self, ...) all params + return
254-
utils/helpers.py:4 format_price(amount, currency) all params + return
255-
utils/helpers.py:15 get_bestsellers(limit) param + return
256-
utils/helpers.py:22 validate_isbn(isbn) param + return
257-
utils/helpers.py:30 generate_report() return
258-
259-
7. Error responses return 200 with error body instead of HTTPException
260-
routers/books.py:21-22, routers/orders.py:13-14,24-25
261-
262-
Returning {"error": "Book not found"} sends HTTP 200 with an error
263-
body. Clients cannot distinguish success from failure by status code.
264-
265-
# Before (routers/books.py:20-23) -- returns HTTP 200 with error!
266-
book = get_book(book_id)
267-
if not book:
268-
return {"error": "Book not found"}
269-
return book
270-
271-
# After -- proper HTTP semantics
272-
from fastapi import HTTPException
273-
274-
book = get_book(book_id)
275-
if not book:
276-
raise HTTPException(status_code=404, detail="Book not found")
277-
return book
278-
279-
8. Missing HTTP status codes on POST endpoints
280-
routers/books.py:8, routers/orders.py:10
281-
282-
POST endpoints default to 200 instead of 201 Created.
283-
284-
# Before
285-
@router.post("/")
286-
def add_book(data: BookCreate):
287-
288-
# After
289-
@router.post("/", status_code=201, response_model=Book)
290-
def add_book(data: BookCreate):
291-
292-
9. Returning raw dicts instead of Pydantic models
293-
services/book_service.py:20-22, services/order_service.py:58-64,71-77
294-
Principle: P6 Start with the Data
295-
296-
Functions return hand-built {"id": ..., "title": ...} dicts instead
297-
of using the Pydantic models already defined in models/. Loses type
298-
safety, validation, serialization control, and IDE support.
299-
300-
# Before (services/book_service.py:19-22)
301-
result = []
302-
for b in books:
303-
result.append({"id": b.id, "title": b.title, "author": b.author,
304-
"isbn": b.isbn, "price": b.price})
305-
return result
306-
307-
# After -- use the model you already defined
308-
return [Book(id=b.id, title=b.title, author=b.author,
309-
isbn=b.isbn, price=b.price) for b in books]
310-
311-
10. Deprecated Pydantic v1 API
312-
services/book_service.py:7 -- data.dict()
313-
314-
Pydantic v2 renamed .dict() to .model_dump(). The old method
315-
still works but is deprecated and will be removed.
316-
317-
# Before (Pydantic v1)
318-
book = BookModel(**data.dict())
319-
320-
# After (Pydantic v2)
321-
book = BookModel(**data.model_dump())
322-
323-
Suggestions
324-
325-
11. if/elif chain for discount logic -- Strategy pattern
326-
services/order_service.py:19-31
327-
Pattern: Strategy -- Callable type alias + dict mapping
328-
329-
# Before (services/order_service.py:19-31)
330-
if quantity >= 10:
331-
discount_type = "bulk"
332-
elif quantity >= 5:
333-
discount_type = "medium"
334-
else:
335-
discount_type = "none"
336-
337-
if discount_type == "bulk":
338-
total = total * 0.8
339-
elif discount_type == "medium":
340-
total = total * 0.9
341-
elif discount_type == "none":
342-
pass
343-
344-
# After -- discount strategies as functions
345-
from typing import Callable
346-
347-
DiscountFn = Callable[[float, int], float]
348-
349-
def bulk_discount(total: float, quantity: int) -> float:
350-
return total * 0.8
351-
352-
def medium_discount(total: float, quantity: int) -> float:
353-
return total * 0.9
354-
355-
def no_discount(total: float, quantity: int) -> float:
356-
return total
357-
358-
def get_discount(quantity: int) -> DiscountFn:
359-
if quantity >= 10:
360-
return bulk_discount
361-
elif quantity >= 5:
362-
return medium_discount
363-
return no_discount
364-
365-
# Usage
366-
discount = get_discount(quantity)
367-
total = discount(total, quantity)
368-
369-
12. if/elif chain for currency formatting -- dict mapping
370-
utils/helpers.py:4-12
371-
Pattern: Registry -- dict mapping replaces if/elif
372-
373-
# Before
374-
def format_price(amount, currency):
375-
if currency == "USD":
376-
return f"${amount:.2f}"
377-
elif currency == "EUR":
378-
return f"E{amount:.2f}"
379-
elif currency == "GBP":
380-
return f"L{amount:.2f}"
381-
else:
382-
return f"{amount:.2f} {currency}"
383-
384-
# After
385-
CURRENCY_SYMBOLS: dict[str, str] = {
386-
"USD": "$", "EUR": "E", "GBP": "L",
387-
}
388-
389-
def format_price(amount: float, currency: str) -> str:
390-
symbol = CURRENCY_SYMBOLS.get(currency)
391-
if symbol:
392-
return f"{symbol}{amount:.2f}"
393-
return f"{amount:.2f} {currency}"
394-
395-
13. Bare string constants for order status -- use StrEnum
396-
services/order_service.py:38,86,89,95,111,114
397-
398-
# Before -- typo-prone strings scattered across the codebase
399-
status="pending"
400-
if order.status != "pending":
401-
if new_status == "shipped":
402-
403-
# After
404-
class OrderStatus(StrEnum):
405-
PENDING = "pending"
406-
SHIPPED = "shipped"
407-
DELIVERED = "delivered"
408-
CANCELLED = "cancelled"
409-
410-
14. Mixed concerns in utils/helpers.py -- split by domain
411-
utils/helpers.py:1-38
412-
Principle: P1 High Cohesion | Rule: #16 Avoid Generic Package Names
413-
414-
Contains formatting (format_price), validation (validate_isbn),
415-
DB queries (get_bestsellers), and reporting (generate_report).
416-
Four unrelated responsibilities. The utils/ name is a dumping ground.
417-
418-
Fix: Move get_bestsellers/generate_report into services/.
419-
Move format_price into formatting.py. Move validate_isbn into models/.
420-
421-
15. Notification side effects interleaved with business logic -- Pub/Sub
422-
services/order_service.py:44-49,89-100,121-125
423-
Pattern: Pub/Sub
424-
425-
_send_email calls are scattered throughout create_order, update_status,
426-
and cancel_order. Each new side effect (Slack notification, analytics
427-
event, audit log) requires modifying every method.
428-
429-
# After -- event-based notification
430-
EventHandler = Callable[[dict], None]
431-
subscribers: dict[str, list[EventHandler]] = {}
432-
433-
def subscribe(event: str, handler: EventHandler) -> None:
434-
subscribers.setdefault(event, []).append(handler)
435-
436-
def post_event(event: str, data: dict) -> None:
437-
for handler in subscribers.get(event, []):
438-
handler(data)
439-
440-
# In create_order:
441-
post_event("order.created", {"order_id": order.id, "email": customer_email})
442-
443-
# Wire handlers in composition root:
444-
subscribe("order.created", send_confirmation_email)
445-
subscribe("order.created", log_order)
446-
447-
---
448-
Summary: 3 critical (layer skip, no DI, no resource management),
449-
7 important (god class, broad catch, missing types, wrong HTTP responses,
450-
missing status codes, raw dicts, deprecated API), 5 suggestions (Strategy,
451-
Registry, StrEnum, split utils, Pub/Sub). The biggest wins would be
452-
introducing a DataInterface Protocol and splitting OrderService.
453-
```
77+
**Example output:** [Full review of a test bookstore-api project](docs/example-review.md) (15 findings with before/after code). See the [color-coded version](https://mktoronto.github.io/python-clean-architecture/) on the landing page.
45478

45579
### All Commands
45680

0 commit comments

Comments
 (0)