DEV Community

Sir Max
Sir Max

Posted on

I Reviewed 50 PRs Last Month — The 4 Things I Kept Commenting On

#webdev #programming #tutorial #beginners

After a particularly review-heavy month, I looked back at my comments and noticed a pattern. Four issues kept coming up — across different repos, different teams, and different experience levels. None are language-specific. All are fixable in under five minutes.

Here's what I kept saying, and what you can do to never get these comments on your PRs.

1. "What does this error actually mean?"

Someone catches an exception and re-raises it with zero context. I see this weekly.

# ❌ You'll regret this at 2 AM
try:
    user = db.get_user(user_id)
except DatabaseError:
    raise APIError("something went wrong")
Enter fullscreen mode Exit fullscreen mode

When this hits production, your logs say APIError: something went wrong. No user ID. No query context. No original stack trace. You're debugging blind. You don't know if it's a timeout, a duplicate key, or a schema mismatch — and at 2 AM, every minute you spend guessing is a minute your users are staring at an error page.

The fix is three characters and a string interpolation:

# ✅ The 10-second fix
try:
    user = db.get_user(user_id)
except DatabaseError as e:
    raise APIError(f"Failed to fetch user {user_id}") from e
Enter fullscreen mode Exit fullscreen mode

from e preserves the original traceback chain. The user ID tells you exactly which record was problematic. Combined, you go from "guess what happened" to "fix it immediately."

This pattern applies everywhere — API calls, file I/O, database queries, message queues. The rule is simple: if you're catching an exception, ask yourself "will I know what went wrong from this error message alone at 3 AM with no coffee?"

A good error message answers four questions:

  • What operation failed? (fetch user)
  • On what resource? (user_id=58291)
  • Why did it fail? (preserved via from e — the DB driver tells you)
  • What should I do next? (the traceback shows you the call chain)

If your error message answers fewer than three of those, rewrite it.

2. The 800-line PR with a 3-word description

I'm not exaggerating. I've reviewed PRs that touched 40 files with the description: "refactor stuff." One memorable submission across 17 files just said: "fix."

Here's the thing: reviewers are not mind readers. If I have to reverse-engineer your intent from the diff, I will miss things. Worse, I will approve changes I don't fully understand — and that's how subtle bugs ship to production.

A good PR description has exactly three sections:

## What
Moved user auth logic out of the views layer and into a dedicated AuthService class.
Added rate limiting to the login endpoint.

## Why
Auth checks were duplicated across 12 view functions. When we added MFA support
last sprint, we had to update all 12 places — and missed 3 of them. Centralizing
into one service eliminates this class of bug permanently.

The rate limiting addition is in response to the brute-force incident from Tuesday.
Now the login endpoint throttles after 5 failed attempts per IP per minute.

## Risk / Testing
- Full unit test coverage for AuthService (34 tests)
- Manual QA: login, logout, MFA enrollment, password reset, account lockout
- Staging: deployed for 2 days, zero auth-related errors in logs
- No DB schema changes
Enter fullscreen mode Exit fullscreen mode

This takes three minutes to write and saves twenty minutes of back-and-forth on every review cycle. My favorite side effect: writing this description forces you to think through what you actually changed. I've caught my own logic errors while writing PR descriptions — things I was about to ship.

3. "Where are the tests for the edge cases?"

A PR lands. The feature works on the happy path. Tests pass. I ask: "what happens when the input is empty?" The author pauses. "Uh. It probably crashes."

The happy path is easy. The bugs live in the edges:

  • What if the file doesn't exist?
  • What if the API returns a 429?
  • What if the user double-clicks the submit button?
  • What if the input string is 10,000 characters?
  • What if the timestamp is from the year 1970?
  • What if the list is empty?

You don't need to test every edge case. But you should test the ones that would wake you up at 3 AM, and you should tell your reviewer which edges you chose to test — and which you consciously deferred.

Here's a pattern I use:

def test_get_user_not_found():
    """If the user doesn't exist, we return 404 — not 500."""
    response = client.get("/users/99999")
    assert response.status_code == 404


def test_get_user_invalid_id():
    """Non-numeric IDs should return 400, not crash the server."""
    response = client.get("/users/abc")
    assert response.status_code == 400
Enter fullscreen mode Exit fullscreen mode

Two tests. Two lines each. Together they catch the two most common failure modes: missing resources and malformed input. This pattern costs 30 seconds per endpoint and pays back in hours of not-debugging.

The rule of thumb I give junior devs: for every function you write, identify the two most likely ways it could fail, and test those. Not the theoretical edge cases from a CS textbook — the real ones that happen when a user copy-pastes wrong or a network blips.

4. Hardcoded values buried in logic

I found this in a production codebase last month:

# ❌ Found in a payment processing service
if attempt_count > 3:
    time.sleep(60)
    return retry_payment()
Enter fullscreen mode Exit fullscreen mode

Three separate problems:

  1. Why 3? Why 60? Nobody knows. The original author left the company.
  2. Changing either requires a full deploy. Through CI, code review, and staging — for a number change.
  3. Different callers need different values. The payment retry should be aggressive (retry fast). The email-sending retry can be patient (retry slow). With hardcoded values, everything gets the same policy.

The fix costs two lines:

# ✅ Self-documenting, configurable, environment-aware
MAX_RETRIES = int(os.getenv("PAYMENT_MAX_RETRIES", "3"))
RETRY_DELAY_SECONDS = int(os.getenv("PAYMENT_RETRY_DELAY", "60"))

if attempt_count > MAX_RETRIES:
    time.sleep(RETRY_DELAY_SECONDS)
    return retry_payment()
Enter fullscreen mode Exit fullscreen mode

Now it's self-documenting, configurable per environment, and tweakable without a deploy. The environment variables default to the original values, so existing behavior doesn't change.

This isn't about premature abstraction. It's about not having to grep for the number 3 across fifty files when your product manager asks you to change the retry policy from "3 attempts" to "5 attempts."

The common thread

All four patterns share the same root cause: writing code for the machine, not for the next human.

The machine doesn't care about error messages, PR descriptions, edge case tests, or named constants. It runs the code either way.

But the developer who gets paged at 3 AM? The teammate reviewing your PR between meetings? The person inheriting this codebase six months from now — who might be you?

They care. A lot.

Fix these four things, and your PRs will sail through review. More importantly, your code will break less often, be faster to fix when it does, and be a codebase people actually want to work in.

What's the most common thing you comment on during code reviews? I'd love to hear what patterns keep showing up in your PRs.

Top comments (0)