Share

Better Peer Reviews: A Practical Guide for Modern Teams

BLUF: Code reviews are your best defense against technical debt that can destroy a business. A single mistake in cleaning up “ghost code” cost Knight Capital $440 million in just 45 minutes. To stay safe in, you need more than just “checking for bugs” – you need strict limits like the 400-LOC rule, PHPStan Level 9, and tools like Deptrac to protect your architecture.


Why Code Reviews Matter: Avoiding Costly Disasters

Bugs get much more expensive the longer they stay in the system. Fixing a mistake during a review is cheap; fixing it after it crashes production can be a multi-billion dollar risk.

History shows us what happens when reviews fail:

  • Knight Capital (2012): Used old, unreviewed code by mistake. Lost $440 million and the entire company.
  • Southwest Airlines (2022): A collapse caused by decades of technical debt and old systems that lacked modern oversight.
Historical FailureMain Technical CauseEconomic Impact
Y2K CrisisTwo-digit year formats$100 Billion+
Knight CapitalReused “ghost” code$440 Million
FriendsterPoor architectureLost market to Facebook
Southwest (2022)Legacy system collapse$800 Million+

The Numbers Game: How to Keep Reviews Efficient

If a Pull Request (PR) is too big, reviewers stop actually “reading” and start “pattern matching.” This is the “Looks Good To Me” (LGTM) trap, where 70% of bugs are missed because the reviewer is overwhelmed.

The 400-Line Rule

Data shows the “sweet spot” for a PR is 200 to 400 lines of code (LOC). Once you go over 400 lines, the ability to find bugs drops fast.

We measure this using Defect Density (D):

D=ndefectskLOCD = \frac{n_{defects}}{kLOC}
PR SizeLines of Code (LOC)Bug Detection RateReviewer Focus
Micro-Fix< 10087%Very High
Optimal200 – 40075% – 80%Best Balance
Large601 – 1,00042%Low (Fatigue)
“Mega-Merge”> 1,00028%Superficial

The 60-Minute Rule

Never spend more than one hour straight on a single review. The human brain can only hold about 7 +/- 2 pieces of information at once. After 60 minutes, you lose the ability to spot complex logic errors.

  • Target Pace: Review 300 to 500 lines per hour.
  • If it takes longer: The PR is too big. Ask the author to break it down.

The Human Side: How to Give Better Feedback

Technical skills don’t matter if your team is afraid of feedback. Google’s Project Aristotle found that “psychological safety” – the ability to take risks without being shamed – is the #1 sign of a great team.

  • You are not your code: Once you submit a PR, it belongs to the team. Keep feedback professional, not personal.
  • The 24-Hour Rule: Give feedback within one business day. Don’t let PRs sit and rot.
  • Ask, don’t tell: Instead of “You missed this,” try “How should we handle this edge case?”
Instead of saying…Try saying…
“You forgot a DTO here.”We should use a DTO here to decouple these layers.”
“Your loop is slow.”“Could we optimize this to avoid O(n2) complexity?”
“I hate this formatting.”“Let’s stick to the PSR-12 standard here.”

Let Machines Do the Boring Work

A human should never spend time pointing out a missing semicolon or a wrong indentation. If your review focuses on syntax, your automation is failing.

The PHP Stack:

  1. PHPStan (Level 9): The gold standard. It catches type errors that cause runtime crashes.
  2. PHPMD (PHP Mess Detector): Scans for “code smells” and over-complicated logic. It flags high Cyclomatic Complexity, unused variables, and bloated classes/methods.
  3. Rector: Automatically updates your code to modern standards (like PHP 8.4).
  4. PHP-CS-Fixer: Handles all formatting debates automatically.
  5. Deptrac: Checks if your layers are leaking (e.g., making sure your Domain doesn’t talk to your Infrastructure directly).

Mastering PHP 8.4 in Your Reviews

When reviewing PHP 8.4 code, look for these specific modern features:

1. Property Hooks

You can now replace many getters and setters with logic directly on the property.

  • Backed Properties: They store a value.
  • Virtual Properties: They are calculated on the fly (like a $fullName made from $first and $last).
  • Watch out for: Virtual properties doing “heavy” work (like database calls) inside loops.

2. Asymmetric Visibility

You can now make a property public to read but private to write (e.g., public private(set)). This is much cleaner than writing a getter for every single property.

PHP 8.4 Checklist:

  • Final by Default: Mark classes final unless they need to be extended.
  • Strict Types: No mixed types. Use specific types for every parameter and return.
  • Visibility: Use the most restrictive access level possible.

Keeping Your Architecture Clean

Reviews are where you protect your “Domain” layer. In Layered Architecture or DDD patterns, your core logic should have zero dependencies on frameworks like Symfony or Doctrine.

How to check this:

  • Check the deptrac.yaml file.
  • If a developer breaks a rule, do not just change the config to allow it. Fix the code using interfaces and Dependency Inversion.

Using AI Wisely

Tools like GitHub Copilot or Snyk can summarize PRs and find basic security holes (like SQL injection). However, they have “architectural blindness” – they don’t understand how a change in one file might break a microservice somewhere else. AI is a great assistant, but a senior engineer must always have the final word.


Common Mistakes to Avoid (Anti-Patterns)

  1. Nitpicking: Don’t argue about style if it’s not in the official coding standard. If the tools don’t flag it, let it go.
  2. Scope Creep: Don’t ask for unrelated refactoring in a bug-fix PR. Create a new ticket instead.
  3. Over-Engineering: Don’t use a “Strategy Pattern” when a simple if statement will do. Keep it simple.

Summary Checklist for your next PR

  • [ ] Is the PR under 400 lines?
  • [ ] Does it pass PHPStan Level 9?
  • [ ] Has PHPMD confirmed the logic isn’t too complex (low Cyclomatic Complexity)?
  • [ ] Are we using Property Hooks or Asymmetric Visibility where it makes sense?
  • [ ] Is my feedback using Conventional Comments (e.g., suggestion:, nitpick:)?
  • [ ] Did I check for N+1 queries in any new logic?