Skip to content

Commit a05ed7d

Browse files
MKTorontoMKToronto
committed
updates
1 parent fda105f commit a05ed7d

2 files changed

Lines changed: 220 additions & 0 deletions

File tree

‎README.md‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,118 @@ The review checks:
7474

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

77+
<details>
78+
<summary><strong>Example output</strong> (fictional bookstore-api)</summary>
79+
80+
```
81+
/review-architecture bookstore-api/
82+
```
83+
84+
#### Architecture Summary
85+
86+
```
87+
Layers
88+
──────
89+
API: routers/books.py, routers/orders.py
90+
Logic: services/book_service.py, services/order_service.py
91+
Database: db/models.py, db/database.py
92+
Unclear: utils/helpers.py (mixed concerns — formatting + DB queries)
93+
94+
Dependency Flows
95+
────────────────
96+
routers/books.py → services/book_service.py → db/database.py ✓
97+
routers/orders.py → db/models.py ⚠ layer skip
98+
routers/orders.py → services/order_service.py → db/database.py ✓
99+
100+
Layer Violations
101+
────────────────
102+
⚠ routers/orders.py imports db/models.py directly — should go through services
103+
```
104+
105+
#### What Works Well
106+
107+
- Clean separation in `routers/books.py` — calls service layer, never touches DB directly
108+
- Good use of Pydantic `BookCreate` / `Book` model separation in `models/book.py`
109+
- `book_service.py` accepts `data_interface` parameter — proper dependency injection
110+
111+
#### Findings by Severity
112+
113+
**Critical**
114+
115+
**1. Layer skip — router imports DB directly**
116+
`routers/orders.py:3` — `from db.models import OrderModel`
117+
Principle: **P3 Depend on Abstractions**
118+
Fix: Move DB access into `services/order_service.py`, pass `DataInterface` from router.
119+
120+
```python
121+
# Before (routers/orders.py)
122+
from db.models import OrderModel
123+
124+
@router.post("/orders")
125+
def create_order(data: OrderCreate):
126+
order = OrderModel(**data.dict())
127+
db.session.add(order)
128+
...
129+
130+
# After (routers/orders.py)
131+
@router.post("/orders", status_code=201)
132+
def create_order(data: OrderCreate):
133+
return create_order_op(data, data_interface=db_interface)
134+
```
135+
136+
**Important**
137+
138+
**2. God class — `OrderService` has 6 responsibilities**
139+
`services/order_service.py:1` — 280 lines, handles orders, payments, inventory, notifications, discounts, and shipping.
140+
Principle: **P1 High Cohesion**
141+
Fix: Extract into `PaymentService`, `InventoryService`, `NotificationService`.
142+
143+
**3. Missing type hints**
144+
`utils/helpers.py:12` — `def format_price(amount, currency):`
145+
Principle: **P7 Keep Things Simple** (type hints catch bugs early)
146+
Fix: `def format_price(amount: Decimal, currency: str) -> str:`
147+
148+
**Suggestions**
149+
150+
**4. if/elif chain for discount logic**
151+
`services/order_service.py:145`
152+
Pattern: **Strategy** — `Callable` type alias
153+
```python
154+
# Before
155+
if discount_type == "percentage":
156+
...
157+
elif discount_type == "fixed":
158+
...
159+
elif discount_type == "bogo":
160+
...
161+
162+
# After
163+
DiscountStrategy = Callable[[Decimal, int], Decimal]
164+
165+
DISCOUNTS: dict[str, DiscountStrategy] = {
166+
"percentage": apply_percentage,
167+
"fixed": apply_fixed,
168+
"bogo": apply_bogo,
169+
}
170+
171+
discount_fn = DISCOUNTS[discount_type]
172+
final_price = discount_fn(price, quantity)
173+
```
174+
175+
**5. Bare string constants for order status**
176+
`services/order_service.py:22` — `status = "pending"`, `"shipped"`, `"delivered"`
177+
Fix: Use `StrEnum`
178+
```python
179+
class OrderStatus(StrEnum):
180+
PENDING = "pending"
181+
SHIPPED = "shipped"
182+
DELIVERED = "delivered"
183+
```
184+
185+
```
186+
187+
</details>
188+
77189
### All Commands
78190
79191
**Review & Analysis:**

‎docs/index.html‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,20 @@
3232
.card h3 { font-size: 1rem; margin-bottom: 0.4rem; }
3333
.card p { font-size: 0.9rem; color: #555; margin: 0; }
3434
footer { margin-top: 3rem; padding-top: 1.5rem; border-top: 1px solid #e0e0e0; font-size: 0.85rem; color: #777; }
35+
.terminal { background: #0d1117; color: #c9d1d9; border-radius: 8px; padding: 1.25rem; font-family: 'SF Mono', 'Fira Code', 'Cascadia Code', monospace; font-size: 0.8rem; line-height: 1.5; overflow-x: auto; margin: 1rem 0; border: 1px solid #30363d; }
36+
.terminal .prompt { color: #7ee787; }
37+
.terminal .heading { color: #79c0ff; font-weight: bold; }
38+
.terminal .ok { color: #7ee787; }
39+
.terminal .warn { color: #d29922; }
40+
.terminal .err { color: #f85149; }
41+
.terminal .dim { color: #6e7681; }
42+
.terminal .file { color: #d2a8ff; }
43+
.terminal .principle { color: #79c0ff; }
44+
.terminal .pattern { color: #ffa657; }
45+
.terminal .kw { color: #ff7b72; }
46+
.terminal .str { color: #a5d6ff; }
47+
.terminal .fn { color: #d2a8ff; }
48+
.terminal .comment { color: #6e7681; font-style: italic; }
3549
@media (max-width: 600px) { .grid { grid-template-columns: 1fr; } }
3650
</style>
3751
</head>
@@ -91,6 +105,100 @@ <h2>Slash Commands</h2>
91105
<li><code>/refactor-legacy</code> — Plan safe refactoring for untested code</li>
92106
</ul>
93107

108+
<h2>Example: /review-architecture</h2>
109+
<p>Sample output from reviewing a fictional <code>bookstore-api/</code> project:</p>
110+
111+
<div class="terminal">
112+
<span class="prompt">$ /review-architecture bookstore-api/</span>
113+
114+
<span class="heading">Architecture Summary</span>
115+
<span class="dim">──────────────────────</span>
116+
117+
Layers
118+
<span class="dim">──────</span>
119+
API: <span class="file">routers/books.py</span>, <span class="file">routers/orders.py</span>
120+
Logic: <span class="file">services/book_service.py</span>, <span class="file">services/order_service.py</span>
121+
Database: <span class="file">db/models.py</span>, <span class="file">db/database.py</span>
122+
Unclear: <span class="file">utils/helpers.py</span> <span class="dim">(mixed concerns — formatting + DB queries)</span>
123+
124+
Dependency Flows
125+
<span class="dim">────────────────</span>
126+
<span class="file">routers/books.py</span> → <span class="file">services/book_service.py</span> → <span class="file">db/database.py</span> <span class="ok">✓</span>
127+
<span class="file">routers/orders.py</span> → <span class="file">db/models.py</span> <span class="warn">⚠ layer skip</span>
128+
<span class="file">routers/orders.py</span> → <span class="file">services/order_service.py</span> → <span class="file">db/database.py</span> <span class="ok">✓</span>
129+
130+
<span class="heading">What Works Well</span>
131+
<span class="dim">──────────────────</span>
132+
<span class="ok">✓</span> Clean separation in <span class="file">routers/books.py</span> — calls service layer, never touches DB
133+
<span class="ok">✓</span> Good Pydantic model separation: <span class="fn">BookCreate</span> / <span class="fn">Book</span> in <span class="file">models/book.py</span>
134+
<span class="ok">✓</span> <span class="file">book_service.py</span> accepts <span class="fn">data_interface</span> parameter — proper DI
135+
136+
<span class="heading">Findings by Severity</span>
137+
<span class="dim">──────────────────────</span>
138+
139+
<span class="err">Critical</span>
140+
141+
<span class="err">1. Layer skip — router imports DB directly</span>
142+
<span class="file">routers/orders.py:3</span>
143+
Principle: <span class="principle">P3 Depend on Abstractions</span>
144+
Fix: Move DB access into services, pass DataInterface from router.
145+
146+
<span class="comment"># Before</span>
147+
<span class="kw">from</span> db.models <span class="kw">import</span> OrderModel
148+
149+
<span class="kw">@</span>router.post(<span class="str">"/orders"</span>)
150+
<span class="kw">def</span> <span class="fn">create_order</span>(data: OrderCreate):
151+
order = OrderModel(**data.dict())
152+
...
153+
154+
<span class="comment"># After</span>
155+
<span class="kw">@</span>router.post(<span class="str">"/orders"</span>, status_code=<span class="str">201</span>)
156+
<span class="kw">def</span> <span class="fn">create_order</span>(data: OrderCreate):
157+
<span class="kw">return</span> create_order_op(data, data_interface=db_interface)
158+
159+
<span class="warn">Important</span>
160+
161+
<span class="warn">2. God class — OrderService has 6 responsibilities</span>
162+
<span class="file">services/order_service.py:1</span> — 280 lines
163+
Principle: <span class="principle">P1 High Cohesion</span>
164+
Fix: Extract <span class="fn">PaymentService</span>, <span class="fn">InventoryService</span>, <span class="fn">NotificationService</span>.
165+
166+
<span class="warn">3. Missing type hints</span>
167+
<span class="file">utils/helpers.py:12</span> — <span class="kw">def</span> <span class="fn">format_price</span>(amount, currency):
168+
Fix: <span class="kw">def</span> <span class="fn">format_price</span>(amount: <span class="fn">Decimal</span>, currency: <span class="fn">str</span>) -> <span class="fn">str</span>:
169+
170+
<span class="dim">Suggestions</span>
171+
172+
<span class="dim">4. if/elif chain for discount logic</span>
173+
<span class="file">services/order_service.py:145</span>
174+
Pattern: <span class="pattern">Strategy</span> — <span class="fn">Callable</span> type alias
175+
176+
<span class="comment"># Before</span>
177+
<span class="kw">if</span> discount_type == <span class="str">"percentage"</span>:
178+
...
179+
<span class="kw">elif</span> discount_type == <span class="str">"fixed"</span>:
180+
...
181+
182+
<span class="comment"># After</span>
183+
DiscountStrategy = <span class="fn">Callable</span>[[<span class="fn">Decimal</span>, <span class="fn">int</span>], <span class="fn">Decimal</span>]
184+
185+
DISCOUNTS: <span class="fn">dict</span>[<span class="fn">str</span>, DiscountStrategy] = {
186+
<span class="str">"percentage"</span>: apply_percentage,
187+
<span class="str">"fixed"</span>: apply_fixed,
188+
<span class="str">"bogo"</span>: apply_bogo,
189+
}
190+
final_price = DISCOUNTS[discount_type](price, quantity)
191+
192+
<span class="dim">5. Bare string constants for order status</span>
193+
<span class="file">services/order_service.py:22</span>
194+
Fix: Use <span class="fn">StrEnum</span>
195+
196+
<span class="kw">class</span> <span class="fn">OrderStatus</span>(<span class="fn">StrEnum</span>):
197+
PENDING = <span class="str">"pending"</span>
198+
SHIPPED = <span class="str">"shipped"</span>
199+
DELIVERED = <span class="str">"delivered"</span>
200+
</div>
201+
94202
<h2>Links</h2>
95203
<ul>
96204
<li><a href="https://github.com/MKToronto/python-clean-architecture">GitHub Repository</a></li>

0 commit comments

Comments
 (0)