Good Reviews vs Bad Reviews: A Real-World Example
It's easy to explain what makes a "good" code review in theory. But when you're actually facing a pull request, the difference between good and bad reviews becomes subtle and crucial. The subtlety lies not in what is being reviewed, but in what the reviewer chooses to look at first, and what they prioritize as important.
Let me show you with a real, concrete scenario: a junior team member just implemented a password change feature for your API. This is a common feature, deceptively simple-looking, yet it's exactly where security vulnerabilities sneak into production systems.
The Code Under Review
The junior developer submitted this code for a password change API endpoint. It compiles. Tests pass. No syntax errors. Everything looks functional at first glance:
@PostMapping("/users/password")
public ResponseEntity changePassword(@RequestBody Map<String, String> request) {
String userId = request.get("userId");
String newPassword = request.get("newPassword");
User user = userRepository.findById(userId);
user.setPassword(newPassword);
userRepository.save(user);
return ResponseEntity.ok("Password changed");
}
The function works. Tests pass. No exceptions are thrown. But look what two different reviewers see.
Reviewer A's Comments (Bad Review)
Reviewer A opens the PR and focuses on style and structure:
- "Use DTO class instead of Map. Raw Map types reduce type safety."
- "Add generic type to ResponseEntity. Change to ResponseEntity<String> for clarity."
- "It's more RESTful to name the method updatePassword rather than changePassword. REST conventions matter."
- "Consider using a proper Java bean with validation annotations instead of raw request parsing."
Nothing in these comments is technically incorrect. The reviewer has valid points about style and best practices. But here's the critical problem: security vulnerabilities just went into production without being caught.
Reviewer B's Comments (Good Review)
Reviewer B opens the same PR and immediately focuses on what matters most — security and correctness:
- [Required] The password is being saved in plain text. This is a critical security vulnerability. Passwords MUST be hashed using BCryptPasswordEncoder or similar cryptographic hashing. If the database is ever exposed, all user passwords are compromised. This is a mandatory fix before merge.
- [Required] There's no verification that the currently logged-in user can only change their own password. Anyone who knows another user's userId can change their password by providing it in the request body. Extract userId from the authentication token instead of trusting user input. This is a horizontal privilege escalation vulnerability.
- [Required] If findById returns null (user doesn't exist), an NPE is raised. Add explicit null handling for non-existent users and throw a meaningful UserNotFoundException.
- [Suggestion] Adding password validation (minimum length, special characters, complexity requirements) would prevent weak passwords from being set even by authorized users.
- [Suggestion] Using a proper DTO class instead of Map<String, String> increases type safety and makes testing easier.
Reviewer B looked at the same code and saw something completely different: two critical security vulnerabilities and a potential null pointer exception. Two security flaws that could compromise every user account.
The Corrected Code Based on Reviewer B
Here's how the code changes after addressing Reviewer B's comments:
// ChangePasswordRequest.java - new DTO class
public class ChangePasswordRequest {
@NotBlank(message = "Current password is required")
private String currentPassword;
@NotBlank(message = "New password is required")
@Size(min = 8, message = "Password must be at least 8 characters long")
@Pattern(regexp = ".*[A-Z].*", message = "Password must contain at least one uppercase letter")
private String newPassword;
// getters and setters
}
// UserController.java - corrected implementation
@PostMapping("/users/password")
public ResponseEntity<String> updatePassword(
@AuthenticationPrincipal UserDetails currentUser,
@Valid @RequestBody ChangePasswordRequest request) {
// Extract userId from authentication token, not request
User user = userRepository.findById(currentUser.getUsername())
.orElseThrow(() -> new UserNotFoundException(currentUser.getUsername()));
// Verify current password is correct before allowing change
if (!passwordEncoder.matches(request.getCurrentPassword(), user.getPassword())) {
throw new InvalidPasswordException("Current password is incorrect");
}
// Hash the new password before storing
user.setPassword(passwordEncoder.encode(request.getNewPassword()));
userRepository.save(user);
return ResponseEntity.ok("Password updated successfully");
}
Now passwords are properly hashed, only the authenticated user can change their own password, the current password is verified, and password validation prevents weak passwords.
The Core Difference: What You Look For First
The fundamental difference between reviewers A and B is not knowledge or experience level alone. It's what they prioritize and look for first.
Reviewer A read the code and looked for "things that could be made better" — style improvements, naming conventions, structure refinements. These are valid concerns, but they're surface-level.
Reviewer B first asked, "What could go wrong when this code runs in production? How could this code be exploited? What damage could occur?" This is the right mental model for security-conscious development.
A good reviewer always imagines worst-case scenarios and failure modes first. What damage can occur when this code is exposed to actual users? Could this be exploited? Could this lead to data loss? What would a bad actor do if they tried to abuse this endpoint? That question is the starting point of effective code review.
How to Write Better Review Comments
A good comment on code contains three essential things:
- What is the problem? Be specific and concrete, not vague or hand-wavy.
- Why is it a problem? Explain the consequence, the risk, and the context.
- How can it be improved? Show a concrete direction or solution, ideally with a code example.
Compare these two approaches to the same issue:
Bad comment: "It's SQL injection."
This comment identifies a problem but leaves the author stranded. I understand there's a problem, but I don't know why it's specifically dangerous or how to fix it. If I'm a junior developer, I have to search the internet to understand SQL injection basics and then figure out the fix myself.
Good comment: "[Required] This code has a SQL injection vulnerability. External input is directly interpolated into the SQL query. If the id value comes from a user request, a malicious input like `x' OR '1'='1'; DROP TABLE users;` could execute arbitrary SQL, deleting all data or exfiltrating sensitive information. Please use parameterized queries instead: `await db.query('DELETE FROM users WHERE id = $1', [id]);` This ensures the value is always treated as data, never as executable SQL."
Now I understand the problem (SQL injection), why it's dangerous (data loss/theft), what specifically is wrong (direct interpolation), and exactly how to fix it (parameterized queries). I learn something in the process and can apply this knowledge to other situations.
The Five Principles of Better Comments
- Talk about code, not people. Say "An exception may occur in this code path" not "Why did you write it like this?" or "You should have known better." Separate the code from the coder.
- Write as a suggestion, not a command. "I think using a DTO would improve type safety and make testing easier. What do you think?" sounds collaborative and respectful, versus "Change to DTO" which sounds authoritarian.
- Include the reason. Criticism or suggestions without underlying reasons don't lead to learning. Explain the "why" so the author understands the principle, not just the rule.
- Provide examples of improvement. Show, don't just tell. Code examples are clearer than verbal explanations and give authors something concrete to work from.
- Praise what's done well. Highlight good approaches, clever solutions, and clean implementations so developers know which direction to repeat and reinforce in future work.
Consider these before and after examples of review comments on the same code:
Bad comment: "This is inefficient."
Better comment: "[Suggestion] The current approach fetches all users, then filters in memory. With millions of users, this loads unnecessary data. Consider filtering at the query level with `SELECT * FROM users WHERE status = 'active'` instead. This scales better and is semantically clearer."
The second comment identifies the problem (inefficiency), explains the consequence (scale issues), shows the specific direction (filter at query level), and provides the code example. The author learns not just the specific fix, but the principle: delegate filtering to the layer best equipped to handle it.
How to Receive Review Comments Well
There's a skill just as important as writing good reviews: receiving them well and responding productively.
When you receive a comment that you instinctively disagree with, the human tendency is to defend immediately. But this closes conversation. Instead, pause and try to understand first: What is this person seeing that I'm not? What context or perspective did I miss? If you still disagree after genuine understanding, communicate with evidence and reasoning, not emotion.
Senior developers have a different mindset about code ownership. They're not emotionally attached to their code being "theirs." Code is a means to achieve business goals. If changing the code makes it better, safer, faster, or more maintainable, then the change is good regardless of who wrote it originally.
Advanced: Prioritizing Issues by Severity
Not all comments are equal, but many reviewers treat them as if they are. Expert reviewers distinguish severity levels:
- [Blocker]: Security vulnerabilities, data loss risks, obvious bugs. Must be fixed.
- [Required]: Significant issues with correctness, architecture, or error handling. Should be fixed.
- [Suggestion]: Improvements that would make code better but aren't blocking. Author decides.
- [Question]: Genuine questions seeking understanding, not judgment.
- [Praise]: Explicitly calling out what's well done.
A PR with five [Required] comments and ten [Suggestion] comments feels different than one with two [Required] comments and one [Suggestion]. Clear prioritization helps authors understand urgency and importance.
The Culture That Creates Better Code
The real value of code reviews isn't captured in a single PR. It's built over time through accumulated small actions and interactions. When seniors consistently write good comments with reasoning, when authors respond thoughtfully rather than defensively, when praise is given for good approaches and learning — the entire team's code review culture strengthens. The psychological safety to ask questions increases. The collective knowledge compounds.
Next time you receive or write a review comment, remember this principle: safety comes first, style second. Intent matters more than form. Learning matters more than ego.
Handling Disagreements in Code Review
Sometimes reviewer and author genuinely disagree. Maybe the reviewer prefers functional programming style and the author prefers imperative. Maybe the reviewer thinks something should be split into a helper function and the author thinks it's clear as-is.
For stylistic disagreements: These usually don't block merges. Document the team preference if it's important, then let it go. "I'd prefer this split into a helper function, but it's readable as-is too. Up to you."
For correctness disagreements: These need resolution. Use evidence: code samples, performance benchmarks, production data, requirements documents. The person with better evidence usually has the better argument.
For architecture disagreements: These sometimes need conversation beyond comments. Schedule a quick call, whiteboard it, then return to the PR with a decision.
The ability to disagree respectfully and resolve disagreements productively is what separates teams that ship great code from teams that are stuck in conflict.
Comments
Post a Comment