Effective Code Review

Reviewing pull requests is a critical part of the software development process. It ensures code quality, adherence to standards, and catches potential issues before they reach the production environment.

Following are the main topics that should be addressed in the code review process:

1. Understand the Context:

• Read the PR description and make sure you understand the rationale behind the changes.
• Read the Acceptance Criteria (AC) and make sure everything is covered.
• Consider the impact of this PR on the overall system and its users. If critical, will the team have time to validate and test this change before releasing? What kind of test is required? Should we target this for a major release? Are other projects impacted by this change? Should representatives be added as reviewers? Etc.

2. Start with a Visual Check:

• Quickly scan the code changes. Look for any obvious issues, such as syntax errors, misplaced code, duplications, design improvements, etc.
• Evaluate variable, method and class names; and overall readability.
• Ensure consistent formatting and adherence to coding conventions and existing code.

3. Review Like You Own It (Always):

• Assume responsibility for the reviewed code. Future bugs in the same area may fall on your shoulders.
• Be thorough and diligent in your review.

4. Test Locally (if Applicable):

• Not all PRs require this level of commitment, but if you feel appropriate, don’t hesitate to checkout the development branch and run it locally. It may help you understand why certain decisions were made and even provide better feedback.
• Play with added tests (especially for bug fixes) and manually test the scenarios described in the acceptance criteria.
• Make sure tests are thorough — i.e. tests are expected to fail before code fix.

5. Provide Constructive Feedback:

• Create a friendly environment and instead of making statements, consider asking questions. Understand the reasoning behind design decisions. Focus on the code and its quality.
• You can and must criticise the code, but never the person. Keep in mind that people may be hurt unintentionally and even unnecessarily. So take into consideration how your comments or suggestions will sound to whoever is reading them.
• Explain your reasoning behind your comment and provide code suggestions. This way the other developer can also learn something new.

6. Explore Edge Cases:

• Test not only the primary scenario but also similar and alternative scenarios.
• Consider areas of the codebase that are less known. If you see possible impacts, test or provide insights in the comments.

7. Evaluate Code Alternatives:

• Ensure there’s no better way to achieve the same result.
• If it’s a personal preference, add a comment (e.g., “nit”) expressing your concern.

8. Check for Clarity and Isolation:

• Is the code clear and well-isolated? Avoid overly complex or tightly coupled solutions.
• Address different code levels (e.g., business logic, error handling, edge cases).

9. Test Coverage and Documentation:

• Verify that tests cover the changes adequately and that the most appropriate test was added. Keep in mind that integration/browser tests can be time-consuming, fragile, and complex to maintain. So if the code is suitable for unit test, give preference for that.
• Check if inline comments, README, etc. reflect the new functionality.
• If there’s a documentation page, make sure screenshots and intructions are updated.

10. Green Build:

• If your project has a CI/CD suite, ensure that the build is passing and the upcoming change won’t impact other developers. Also check latency or the necessary builds if there are browser test changes or changes that may impact performance.

11. Communication:

• If you can, use “Start/Stop Review” feature from your Bitbucket/Gitlab. It’ll send a single notification email with all your comments at once.
• If you haven’t approved the PR due to pending changes, let the other person know that you’ve finished the review. You can do this by writing a comment (and optionally tagging the person).
• If there are too many changes or if you believe the person is heading in the wrong direction, consider having a quick call ASAP to express your concerns or to clear explain the changes you believe are necessary.
• A clear and open communication can significantly expedite the approval process.

Conclusion

Remember, effective PR reviews contribute to better code quality, faster delivery, and a more robust system. By following these best practices, you’ll help maintain a healthy codebase and foster collaboration within your team.

References:

The (written) unwritten guide to pull requests — Work Life by Atlassian

Pull Request Best Practices: Our Tips | Dev Interrupted Powered by LinearB

Understanding and Effectively Utilizing Pull Requests