Ready! PR! Fight!
By Daniel Samson · 2023-09-07
Code reviews do not need to be antagonistic. In fact they should be a supportive process, to help an engineer raise the quality of their code, so that it may continue through the Software Development Life Cycle (SDLC).
Analysing Pull Requests
Always clone/pull down their changes and read them in your correctly configured IDE.
Look the ticket/issue description and acceptance criteria.
Confirm that their changes meets the acceptance criteria.
Confirm that their changes meets linting rules, Automated Testing Requirements, Design and Architecture Best Practices.
Ask yourself, How you would implement the feature/bugfix? How would you do it differently?
is their changes appropriate, in scope or out of scope, for the ticket/issue?
Categorise Errors into Three Buckets
Critical Errors.
Important Issues.
Suggestions / Nitpicks.
Critical Errors: are show stopping errors. Like syntax errors, or errors that could cause critical damage to the data of the application.
Important Issues: are more considerations of design and architectural choices. Like edge cases they didn't consider. They may not be a show stopper but are important to consider and may cause issues further down the line.
Suggestions: are more changes that would improve the code but you would be willing to ignore. Nitpicks like stylistic choices, whitespace, variable names.
Set tasks (if available)
In some issue trackers you can set tasks that need to be carried out in order for you to consider approving the changes.
Give good Feedback
The Ideal Format:
[Category]
[Brief few word summary of error/issue]
[Explanation or reason for raising error/issue, with examples]
[Code Suggestion or Example]
[Give helpful references when applicable]
eg:
Critical Blocker: Syntax Error Found.
Variable `itn
products`has a spelling issue of the data type. This will break in production when deployed.Consider renaming data type to int:
---itn
products+++int
products
Things to remember:
Keep language neutral.
Be precise in what you are saying.
Give space for them to research and resolve the issue on their own.
Frame as requests, not commands: Avoid making feedback sound like a strict demand. Use phrases like "Could we extract this into a helper method?" or "What do you think about..." instead of direct instructions.
Tie comments to principles: Frame your feedback around established team style guides or architecture rules, not personal preferences.
Uphold a consistent level of quality across different members of your team
Good words to use:
blocker:Use this for critical items that must be addressed, such as security vulnerabilities, broken logic, or major architectural gaps.suggestion / consider:Use this to offer actionable, better ways to write a piece of code, such as extracting a helper method or simplifying an expression.nit:Short for nitpick, this denotes minor, non-blocking suggestions about style, naming, or polish. The author can choose whether to resolve these before merging.praise / note:Use this to highlight good code, clever solutions, or excellent use of design patterns.
Things to avoid:
Don't make the comments/tasks personal or targeted.
Don't act aggressively. Instead be conscientious and show empathy.
Avoid condescending words: Words such as "just", "obviously", "easy", or "only"can come across as belittling. You can use linters like Alex.js to automatically flag these.
Don't let pressures such as deadlines prevent you from holding the line. If you missed a bug in a code review, then you are just a responsible for it.
Follow up with a summary
Most issue trackers offer a comment box before requesting changes or making approvals.
Request Changes Example:
I found 0 Critical Issues, 1 Important Issue and have left some non blocking suggestions. The important issue is what is blocking me from approving your changes. Please consider my comments and address the important issue.
Note: Overall, I believe that your solution is architecturally sound, and your changes meets the acceptance criteria.
Approval Example:
LGTM!