There are code reviews you forget five minutes after approving them.
Then there are code reviews that permanently change how you think about software.
I've had both.
Early in my career, I thought code review was mostly about catching bugs.
Someone writes code.
Someone else reads it.
They find something wrong.
They leave a comment.
The developer fixes it.
Pull request gets merged.
Done.
That mental model is incomplete.
The best code reviews I've experienced weren't really about syntax, formatting, or whether a function was ten lines too long.
They were about how to think.
A good review can expose an architectural assumption you didn't realize you were making.
It can reveal that your data model doesn't represent reality.
It can show you that an apparently harmless abstraction will become a nightmare six months later.
It can demonstrate why an API should be designed around behavior rather than implementation.
And sometimes, the most valuable review comment is only one sentence long.
Over time, I started realizing that experienced engineers don't necessarily write better code because they know more syntax.
They write better code because they have developed better instincts about:
- boundaries
- failure
- data
- complexity
- ownership
- change
- observability
- trade-offs
These instincts are difficult to learn from tutorials.
You usually learn them by getting burned.
This article is about five hypothetical-but-realistic code reviews based on the kinds of engineering lessons that changed how I approach software.
Each review taught me something different.
And together they changed my definition of what "good code" actually means.
1. The Code Review That Taught Me That Correctness Comes Before Cleverness
The first lesson was simple:
Code that looks intelligent is not necessarily code that is correct.
I remember working on logic that involved processing a collection of records.
The implementation was compact.
It used a clever combination of filtering, mapping and reduction.
Something like:
total = sum(
item.price * item.quantity
for item in items
if item.active
)
At first glance, this looks great.
It's short.
It's readable.
It's idiomatic Python.
But the review wasn't about the syntax.
The reviewer asked:
"Why are inactive products excluded from the calculation?"
That question stopped me.
I had assumed:
inactive product
↓
don't include it
But the business requirement was actually:
Product becomes inactive
↓
New purchases cannot use it
↓
Existing orders must preserve it
The calculation wasn't supposed to ask whether the product was currently active.
It was supposed to use the historical state captured when the order was created.
The bug wasn't in the loop.
The bug was in my mental model.
Code Can Be Locally Correct and Globally Wrong
This is one of the most important engineering lessons I've learned.
Consider:
def calculate_total(items):
return sum(
item.price * item.quantity
for item in items
)
The function itself may be perfectly correct.
But what if:
item.price
is mutable?
Suppose:
Monday:
Laptop = $1000
Tuesday:
Laptop = $1200
A customer bought it Monday.
If we calculate their historical order using the current product price:
Order total = $1200
we have created a logical bug.
The arithmetic is correct.
The system is wrong.
This is the difference between local correctness and system correctness.
The Fix Was a Data Modeling Decision
Instead of relying on the current product:
OrderItem
│
└── Product
│
└── current price
we capture the price at purchase time:
Order
│
└── OrderItem
├── product_id
├── quantity
└── price_at_purchase
For example:
CREATE TABLE order_items (
id BIGSERIAL PRIMARY KEY,
order_id BIGINT NOT NULL
REFERENCES orders(id),
product_id BIGINT NOT NULL
REFERENCES products(id),
quantity INTEGER NOT NULL
CHECK (quantity > 0),
price_cents INTEGER NOT NULL
CHECK (price_cents >= 0)
);
Now:
Product.price
represents the current catalog price.
While:
OrderItem.price_cents
represents historical truth.
That tiny modeling decision has enormous consequences.
The Diagram
PRODUCT
┌────────────────┐
│ id │
│ name │
│ current_price │
└───────┬────────┘
│
│ reference
▼
ORDER_ITEM
┌────────────────┐
│ product_id │
│ quantity │
│ price_cents │
└────────────────┘
│
▼
ORDER
The review taught me something bigger than "store historical prices."
It taught me:
Before optimizing code, make sure the code represents the correct model of reality.
The Engineering Lesson
A lot of bad software starts with developers asking:
"How do I implement this?"
before asking:
"What is actually true?"
That's backwards.
I now try to ask:
What does this data mean?
↓
What must always be true?
↓
What changes over time?
↓
What is historical?
↓
What is current?
↓
Then:
How should I implement it?
This is one reason I became much more interested in data modeling and system design.
The code is downstream from the model.
If the model is wrong, elegant code simply produces the wrong answer faster.
Implementation Principle: Make Invariants Explicit
Suppose an order quantity must always be positive.
Don't rely only on:
if quantity <= 0:
raise ValueError()
You can also enforce it at the database level:
CHECK (quantity > 0)
Now you have:
Application validation
+
Database constraint
=
Defense in depth
The system becomes harder to corrupt.
That review changed how I see constraints.
Constraints aren't obstacles.
They're executable statements about reality.
2. The Code Review That Taught Me That Abstractions Have a Cost
The second review taught me something even more painful:
Not every abstraction makes software better.
I once encountered the temptation to create a generic repository layer.
It looked sophisticated.
Something like:
class BaseRepository:
def find(self, filters):
...
def create(self, data):
...
def update(self, id, data):
...
def delete(self, id):
...
Then:
class UserRepository(BaseRepository):
pass
class ProductRepository(BaseRepository):
pass
class OrderRepository(BaseRepository):
pass
At first:
Less duplication!
Beautiful.
Then requirements changed.
Orders needed:
transactional operations
state transitions
locking
historical records
Users needed:
email uniqueness
authentication
soft deletion
Products needed:
inventory
pricing
availability
The generic abstraction started becoming:
BaseRepository
│
├── if User
├── if Product
├── if Order
├── if special case
└── if another special case
Eventually the abstraction was more complicated than the duplication it was supposed to eliminate.
The Review Comment
The reviewer essentially asked:
"Are these things actually the same, or do they just look similar right now?"
That changed how I think about abstraction.
Two pieces of code being structurally similar does not mean they share the same concept.
This distinction is enormous.
Similarity Is Not Identity
Imagine:
user.update(...)
product.update(...)
order.update(...)
They all technically update something.
But semantically:
User update
≠
Product update
≠
Order update
The abstraction:
update(entity)
may hide meaningful differences.
A better design might be:
user_service.update_profile(...)
product_service.change_price(...)
order_service.cancel(...)
Now the operations describe domain behavior.
The Diagram
Bad abstraction:
BaseRepository
│
┌─────────┼─────────┐
▼ ▼ ▼
User Product Order
│ │ │
└───── shared ──────┘
Eventually:
BaseRepository
│
┌──────────┼──────────┐
▼ ▼ ▼
User Product Order
│ │ │
hacks hacks hacks
│ │ │
└──────────┼───────────┘
▼
abstraction hell
Better:
UserService
ProductService
OrderService
│
▼
Shared infrastructure
│
▼
Database
Share infrastructure where it is genuinely shared.
Don't force domain concepts into the same shape.
Abstraction Should Follow Understanding
This became one of my favorite engineering principles.
Don't abstract what you haven't understood.
Early in a project, duplication can be useful.
It gives you information.
Suppose you have:
send_user_email()
send_order_email()
send_payment_email()
They look similar.
You could immediately create:
send_email(template, recipient, context)
Maybe that's correct.
But perhaps six months later you discover:
User email
→ transactional
Order email
→ retryable
Payment email
→ audit-sensitive
Now the generic abstraction becomes restrictive.
Sometimes three similar implementations are telling you:
"We don't yet know whether these things are actually the same."
That's valuable information.
The Rule I Use Now
I don't ask:
"Can I remove duplication?"
I ask:
"Does the duplication represent the same concept?"
If yes:
Abstract.
If no:
Keep them separate.
If uncertain:
Wait.
Waiting is sometimes the better engineering decision.
3. The Code Review That Taught Me to Design for Failure
The third review changed my relationship with error handling.
I had written code around an external API.
The happy path looked something like:
response = payment_provider.charge(
amount=amount
)
if response.success:
mark_order_paid(order)
Simple.
The reviewer asked:
"What happens if the payment succeeds but this process crashes before
mark_order_paid()?"
That question opened an entire world.
Distributed Systems Exist Even in Small Applications
The system looked like:
Your API
│
▼
Payment Provider
│
▼
Payment
But now there are two independent systems.
Your database might say:
PENDING
while the payment provider says:
PAID
The network can fail.
The process can crash.
The response can disappear.
The user can retry.
The payment provider can timeout.
You don't control any of these.
The Naive Implementation
def pay(order):
result = payment.charge(
amount=order.total
)
if result.success:
order.status = "PAID"
db.save(order)
Potential sequence:
1. Request payment
2. Provider charges customer
3. Provider returns success
4. Application crashes
5. Database still says PENDING
Now what?
The Better Design
Introduce explicit payment states:
PENDING
AUTHORIZED
PAID
FAILED
REFUNDED
And idempotency:
Client
│
│ Idempotency-Key: abc123
▼
Backend
│
▼
Payment Provider
Store:
abc123 → payment result
If the request arrives again:
abc123
│
▼
Already processed
│
▼
Return existing result
Implementation
A simplified schema:
CREATE TABLE payments (
id BIGSERIAL PRIMARY KEY,
order_id BIGINT NOT NULL
REFERENCES orders(id),
idempotency_key TEXT NOT NULL UNIQUE,
status TEXT NOT NULL,
amount_cents INTEGER NOT NULL
CHECK (amount_cents > 0),
created_at TIMESTAMPTZ NOT NULL DEFAULT now()
);
Then:
def create_payment(order, key):
existing = payment_repo.find_by_key(key)
if existing:
return existing
payment = payment_repo.create(
order_id=order.id,
idempotency_key=key,
status="PENDING",
amount_cents=order.total
)
return payment
The unique constraint protects against duplicates.
The Deeper Lesson
The review wasn't really about payments.
It taught me:
Assume the world can interrupt your program at any moment.
A process can die.
A machine can restart.
A database can timeout.
A network packet can disappear.
A user can click twice.
A worker can execute the same job twice.
An external API can return success after you timeout.
Once you think this way, software design changes.
You stop asking:
"Does this work?"
and start asking:
"What happens when this works halfway?"
That is a much more important question.
Design Around Failure States
A robust system doesn't pretend failure doesn't happen.
It models failure.
Operation
│
┌────────┼────────┐
▼ ▼ ▼
Success Timeout Failure
│ │ │
▼ ▼ ▼
Commit Retry Recover
And sometimes:
Unknown outcome
│
▼
Reconcile
│
▼
Determine truth
This is especially important for:
Payments
Orders
Messaging
Distributed jobs
File uploads
External APIs
Inventory
4. The Code Review That Taught Me That Complexity Is a Budget
The fourth review was about performance.
I had optimized something before it needed optimization.
The code was relatively straightforward.
But I thought:
"This will eventually be slow."
So I introduced caching.
API
│
▼
Redis
│
▼
PostgreSQL
The reviewer asked:
"What evidence tells us the database is the bottleneck?"
I didn't have any.
I had optimized a problem that didn't exist.
Premature Optimization Is Really Premature Complexity
People often quote:
"Premature optimization is the root of all evil."
I think the more practical version is:
Premature complexity is expensive even when the optimization is technically correct.
Caching introduces:
Cache keys
TTL
Invalidation
Stale data
Memory usage
Failure modes
Monitoring
Now instead of:
Request
↓
Database
you have:
Request
↓
Cache
├── HIT
│ ↓
│ Response
│
└── MISS
↓
Database
↓
Cache
↓
Response
That's not free.
Measure First
Suppose we have:
SELECT *
FROM products
WHERE category_id = 42;
Before caching, measure.
Maybe the query takes:
4ms
Maybe there are:
2,000 requests/day
Caching this might provide almost no value.
Instead, suppose the query takes:
1.8 seconds
and receives:
50,000 requests/minute
Now we have evidence.
Optimization becomes engineering instead of speculation.
A Better Optimization Loop
Observe
↓
Measure
↓
Identify bottleneck
↓
Hypothesize
↓
Change
↓
Measure
↓
Keep or revert
This is very different from:
Guess
↓
Add technology
↓
Hope
Example: Database Index
Suppose:
SELECT *
FROM orders
WHERE user_id = 123
ORDER BY created_at DESC;
The table grows.
Instead of immediately introducing a caching layer, check the query plan.
You might discover that an index solves the problem:
CREATE INDEX idx_orders_user_created
ON orders(user_id, created_at DESC);
Now:
Before:
Query
↓
Scan huge table
↓
Sort
↓
Response
After:
Query
↓
Index
↓
Relevant rows
↓
Response
A one-line index may be better than another distributed component.
Complexity Has Carrying Costs
Every new component introduces:
Operational cost
Debugging cost
Deployment cost
Security cost
Learning cost
Failure modes
Think of architecture as having a complexity budget.
For example:
Database
1 unit
Redis
+1
Queue
+1
Search cluster
+1
Microservice
+2
Event bus
+2
Multi-region
+3
The numbers aren't literal.
The idea is.
Complexity compounds.
The Diagram
Simple:
Application
│
▼
Database
More complex:
Application
├── Redis
├── Queue
├── Search
├── Service A
├── Service B
└── Event Bus
│
▼
Database
The second architecture may eventually be necessary.
But it should earn its complexity.
Complexity Should Buy You Something
This became a rule I use often:
Every architectural component should solve a measurable problem.
Redis should solve:
Latency
Database load
A queue should solve:
Asynchronous processing
Traffic smoothing
Retryability
A search engine should solve:
Search requirements
Microservices should solve problems like:
Independent deployment
Team ownership
Isolation
Scaling boundaries
Not:
"Big companies use microservices."
5. The Code Review That Taught Me That Readability Is a Performance Feature
The fifth review might have been the most subtle.
The code worked.
It was reasonably fast.
It passed tests.
But the reviewer said something like:
"I understand what this code does, but I don't understand why it does it."
That sentence changed my view of readability.
I had previously thought readability meant:
Good variable names
Short functions
Formatting
Comments
Those things matter.
But deeper readability is about preserving intent.
Code Should Explain the System's Decisions
Consider:
if user.status == "ACTIVE":
process(user)
Easy to read.
But why?
Maybe inactive users shouldn't be processed.
Or maybe only verified users should.
Or perhaps:
ACTIVE
means something very specific in the domain.
A better design can make the intent explicit:
if user.can_receive_notifications():
notify(user)
Now the code communicates behavior.
Business Logic Should Read Like Business Logic
Instead of:
if (
order.status == "PAID"
and order.created_at < cutoff
and not order.refunded
):
...
you might encapsulate the concept:
if order.is_eligible_for_archive():
...
And:
class Order:
def is_eligible_for_archive(self):
return (
self.status == "PAID"
and self.created_at < cutoff
and not self.refunded
)
The complexity didn't disappear.
It moved to a place where it can be named.
That's valuable.
Names Are Compression
A good function name compresses a lot of knowledge.
Consider:
calculate_total()
versus:
calculate_refundable_amount()
The second name tells me more.
Or:
transition_to_paid()
instead of:
update_status("PAID")
The second is a generic mutation.
The first communicates a domain transition.
APIs Should Express Intent
Compare:
PATCH /orders/123
with:
POST /orders/123/cancel
The first exposes implementation.
The second expresses behavior.
This doesn't mean action-oriented endpoints are always better.
The larger lesson is:
Interfaces should communicate the domain's concepts, not merely expose its database.
Implementation: State Transitions
Instead of:
order.status = "CANCELLED"
use:
order.cancel()
Then:
class Order:
def cancel(self):
if self.status not in {"PENDING", "PAID"}:
raise InvalidTransition()
self.status = "CANCELLED"
Now the domain rule lives with the behavior.
You can test it directly:
def test_paid_order_can_be_cancelled():
order = Order(status="PAID")
order.cancel()
assert order.status == "CANCELLED"
Readability Reduces Future Bugs
This is why I increasingly see readability as a reliability feature.
Suppose an engineer joins the project six months later.
They see:
order.status = "CANCELLED"
They might assume:
Any order can be cancelled.
But if they see:
order.cancel()
they know:
There is probably domain logic here.
The code guides the engineer toward the correct behavior.
That's powerful.
Comments Should Explain Why
Bad comment:
# Increment counter
counter += 1
The code already explains that.
Better:
# Keep this counter monotonic because downstream
# reconciliation uses it to detect missing events.
counter += 1
Now the comment preserves context.
The best comments explain:
Why this exists
Why this approach was chosen
What constraint must not be violated
Not:
What the next line does
The Deeper Pattern Behind All Five Reviews
Looking back, these five reviews seem different.
One was about data modeling.
One was about abstraction.
One was about failure.
One was about complexity.
One was about readability.
But they all point to the same thing.
Engineering is mostly about managing change and uncertainty.
Let's put them together.
Lesson 1: Model Reality Correctly
Reality
↓
Domain model
↓
Data model
↓
Code
If the model is wrong:
Wrong model
↓
Correct implementation
↓
Correctly implemented mistake
Lesson 2: Don't Abstract Before Understanding
Similarity
↓
Observation
↓
Understanding
↓
Abstraction
Not:
Similarity
↓
Generic framework
↓
Pain
Lesson 3: Design for Failure
Operation
↓
Success
OR
Failure
OR
Timeout
OR
Partial completion
OR
Unknown state
Good systems account for these.
Lesson 4: Spend Complexity Carefully
Problem
↓
Measurement
↓
Solution
Not:
Technology
↓
Find a problem for it
Lesson 5: Preserve Intent
Code
↓
Meaning
↓
Future engineer
The goal isn't merely for today's developer to understand the code.
The goal is for someone six months from now to understand it without needing to reconstruct your entire thought process.
Code Review Is Architecture Review
This changed my attitude toward pull requests.
A pull request is not just:
Does this compile?
It's also:
Does this model the domain correctly?
Does this introduce unnecessary coupling?
What happens when it fails?
Will this still make sense in six months?
Does this add complexity we actually need?
Can another engineer understand the intent?
These questions are much more valuable than arguing about tabs versus spaces.
A Practical Code Review Framework
Today, when reviewing code, I like thinking in layers.
Layer 1: Correctness
Does it work?
Check:
Happy path
Edge cases
Invalid input
Failure states
Concurrency
Layer 2: Data
Does the model represent reality?
Ask:
What does this field mean?
What happens when it changes?
What history needs preserving?
What constraints exist?
Layer 3: Architecture
Where does this responsibility belong?
Ask:
Does this create coupling?
Does this violate boundaries?
Is this abstraction justified?
Layer 4: Operations
What happens in production?
Ask:
How do we debug it?
How do we monitor it?
What happens when a dependency fails?
Can we recover?
Layer 5: Future Change
What happens when requirements change?
Ask:
Can this evolve?
Will this force a rewrite?
Does this lock us into a decision too early?
This is where code review starts becoming engineering review.
What I Don't Care About as Much Anymore
Experience changed what I consider important.
I care less about:
One-line functions
and more about:
Correct boundaries
I care less about:
Perfect abstractions
and more about:
Useful abstractions
I care less about:
Maximum performance
and more about:
Measured performance
I care less about:
How clever the implementation looks
and more about:
Whether another engineer can reason about it
And I care much more about:
Failure
Data integrity
Operational visibility
The Best Code Is Often Boring
This is probably one of the biggest changes in my engineering philosophy.
When I was younger, interesting code meant:
Complex algorithm
Clever abstraction
Advanced architecture
New technology
Now, interesting engineering often looks boring.
For example:
def create_order(user, items):
validate_items(items)
with transaction():
order = create_order_record(user)
create_order_items(order, items)
return order
There is nothing flashy here.
But if:
validation
transactionality
constraints
authorization
idempotency
observability
are all correctly handled around it, this boring code can be extremely valuable.
Engineering Is Not About Impressing the Compiler
The compiler doesn't care about your architecture.
The database doesn't care about your design philosophy.
The user doesn't care how elegant your dependency injection system is.
Production cares about:
Correctness
Reliability
Performance
Security
Recoverability
And future developers care about:
Clarity
Boundaries
Intent
Changeability
Good engineering balances both.
The Difference Between Code and Software
This distinction became much clearer to me.
Code is what you write.
Software is what happens after you write it.
Software includes:
Code
+
Data
+
Users
+
Infrastructure
+
Failures
+
Deployments
+
Operations
+
Future changes
A code review that only looks at the code is incomplete.
You have to think about the system around it.
A Pull Request Is a Prediction
This is another mental model I like.
Every pull request is a prediction about the future.
When you merge:
This code will behave correctly.
But you're also saying:
This data model will remain useful.
This boundary will make sense.
This complexity is worth it.
These failure modes are acceptable.
Future developers will understand this.
That's a much bigger claim.
Code review is the process of challenging that prediction.
The Five Questions I Ask Now
If I had to reduce everything I've learned into five questions, they would be:
1. What reality is this code modeling?
If I don't understand the underlying concept, I shouldn't approve the implementation yet.
2. What happens when this fails halfway?
Timeouts, retries, crashes and duplicate requests are normal parts of production.
3. Is this abstraction actually earned?
Similarity isn't enough.
4. What complexity am I introducing?
Every abstraction, dependency and service has a maintenance cost.
5. Will the next engineer understand why?
Because eventually, someone else will have to change it.
And that person might be me six months later.
A Complete Review Example
Imagine this pull request:
def process_payment(order):
result = provider.charge(order.total)
if result.success:
order.status = "PAID"
db.save(order)
return result
A shallow review says:
Looks good.
A deeper review asks:
1. What if provider succeeds and db.save() fails?
2. Can this function be called twice?
3. Is provider.charge() idempotent?
4. Where is the payment ID stored?
5. What if the provider times out?
6. How do we reconcile unknown states?
7. Is order.status allowed to change directly?
8. Is payment amount derived from trusted data?
9. How do we observe failures?
10. How do we test duplicate requests?
Now we're reviewing a system.
A More Robust Version
Conceptually:
def process_payment(order, idempotency_key):
payment = payment_repo.find_by_key(
idempotency_key
)
if payment:
return payment
payment = payment_repo.create(
order_id=order.id,
amount_cents=order.total,
idempotency_key=idempotency_key,
status="PENDING"
)
try:
result = provider.charge(
amount=payment.amount_cents,
idempotency_key=idempotency_key
)
if result.success:
payment.mark_paid(
provider_id=result.id
)
else:
payment.mark_failed()
payment_repo.save(payment)
return payment
except TimeoutError:
payment.mark_unknown()
payment_repo.save(payment)
raise
This still isn't a complete payment system.
Real systems need reconciliation, webhooks, retries, provider-specific behavior and more.
But notice how different the mental model is.
We're explicitly modeling:
Identity
State
Failure
Retries
External systems
Historical records
That's engineering.
Code Review Should Teach, Not Just Judge
Another lesson I learned is that the best code reviews don't simply say:
"Wrong."
They explain:
"Here's the risk."
For example, instead of:
Don't do this.
say:
This couples order state directly to the payment provider response.
If the provider succeeds but this transaction fails, our local state
can remain PENDING. Consider modeling the payment state separately and
using an idempotency key so retries don't create duplicate charges.
That teaches a principle.
The developer can use it elsewhere.
That's a much better review.
From Reviewer to Better Engineer
Eventually, code review stops being something you do to other people's code.
You start reviewing your own code mentally before opening the pull request.
You start thinking:
Will someone question this abstraction?
What happens if this times out?
Is this data historical?
Do I need this dependency?
Can I explain why this exists?
That's when code review has done its job.
It has changed how you think.
Final Thoughts
The five code reviews I remember most weren't necessarily the ones that caught the biggest bugs.
They were the ones that changed the questions I asked.
The first taught me:
Correctness starts with modeling reality.
The second taught me:
Abstraction is a tool, not a virtue.
The third taught me:
Failure is not an edge case. It is part of the normal execution environment.
The fourth taught me:
Complexity is a budget. Spend it where the problem demands it.
The fifth taught me:
Readable code preserves intent for people you haven't met yet.
Together, these lessons changed my definition of engineering.
I used to think engineering was mostly about making software work.
Now I think it's about making software continue to work as reality changes.
Users change.
Requirements change.
Traffic changes.
Databases grow.
Dependencies fail.
Teams change.
Business models change.
The original developer leaves.
And eventually, somebody opens the code six months later and asks:
"Why did they build it this way?"
The quality of your engineering is partly determined by how easily that person can answer that question.
That's why I don't think the best code is necessarily the shortest code.
Or the cleverest code.
Or the most abstract code.
Or even the fastest code.
The best code is code that makes the system's important truths visible.
It tells you:
What the system believes.
What the system guarantees.
What can fail.
Where responsibility lives.
Why a decision was made.
And when those things are clear, something interesting happens.
The code becomes easier to change.
The architecture becomes easier to reason about.
The bugs become easier to diagnose.
The reviews become more meaningful.
And the system starts feeling less like a pile of instructions and more like an engineered machine.
That's probably the biggest thing code reviews have taught me.
Good engineering isn't about writing code that looks impressive.
It's about building systems that remain understandable when the original assumptions start breaking.
And eventually, they always do.
Top comments (0)