Ready! PR! Fight!

By Daniel Samson · 2023-09-07

The code reviews on pull requests (PRs) 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). To quote google "The primary purpose of code review is to make sure that the overall code health of the code base is improving over time. All of the tools and processes of code review are designed to this end."

Analysing Pull Requests Properly

Always clone/pull down their changes and read them in your correctly configured IDE.
Look at 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?
Consider the ticket: is their changes appropriate? are they in scope or out of scope?

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]

Critical Example:

Critical Blocker: Syntax Error Found.
Variable `itnproducts` 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

Important Example:

Important: Edge case may cause data corruption
In your method, you have correctly handled saving the record. However, if an exception is thrown on <file> line 123, it will leave the related record with the wrong data.
The easiest fix would be to consider using database transactions. That way, the changes are committed to the database, after your method has completed successfully

Suggestion Example:

Suggestion: improve readability
Can we use assetCount instead of assertEquals? it will help improve the intent of your code changes.
<Code example>

Nit pick:

Nit pick: formatting issue (Non blocking)
if statements should have the opening curly brace { on the same line as the condition
-- if ($this->connected)
-- {
++ if ($this->connected) {
Please see our documentation on how to setup your IDE. This would help you to prevent this sorts of comments on your PR.

When reviewing code:

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 towards that person.
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.
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 Examples

When there are serious issues to address

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.

When there a lot of nit picks

The feature correctly meets the acceptance criteria, balanced well with automated tests. The three suggestions I made are cosmetic only. However, one of the suggestions is for a formatting error, and it is requirement for this project to fix. It is the only reason I am requesting changes. Excellent Work!
Also: you may want to double check the settings and plugins of your IDE, so that these sorts of things gets fixes automatically. It would help to prevent the comments I made on your PR. Most IDEs provide a feature that will auto-format your changes before you commit them. We have a guide on this [HERE] that can help.

Approval Example:

Lazy but effective

LGTM!

A better way to give feedback. It is important to praise people when they produce good work.

The fix is correct and well targeted. 0 Critical. 0 Important issues found, the new tests cover the changes well, and it meets the acceptance criteria. I do not have any suggestions to improve the code. This is really good work!