Code Review Best Practices
A Code Review (peer review) is the process of managing changes in the source codebase. It’s part of the git workflow which can be ignored (when direct pushes to branches, like master, are allowed) or reduced to almost automatic merges. Usually we like to perform code reviews not only for direct changes to the codebase, but also for design evolution and obtaining/sharing knowledge.
Why Code Reviews
Spending time on Code Review has the following benefits:
- Ensure code satisfies requirements (does what it has to do compare to ticket description, modeling, etc)
- Ensure code meets standards
- Find bugs before code reaches users
- Share knowledge
- Check that code is clear (understandable), effective, maintainable, secure
- Collaborate on design
- Evolve application code (for every bug or feature, we should fix known tech debt to make the code better)
When Code Review is complete
First and foremost, we rely on the trust of the code author to make updates to the code base as necessary. There are some cases (urgent, hot fixes) when we can obey the rules, but that’s an exception.
The conventional workflow requires the following items before we can mark code review as completed:
- Critical stakeholders (gatekeepers) are agreed
- No tasks and comments left unresolved
- All CI checks pass
Who is doing Code Review
Usually multiple teams are working on the system, and they have areas of responsibility. In micro-service architecture each team could have few services in them. It’s considered a good practice to include your own team members as code reviewers as well as knows stakeholders.
If there are default code reviewers configured, don’t remove them: they were added for a reason. If you need to adjust that list, ask a question and reach out for some help. Do the same (ask question) if you don’t know dedicated domain experts.
Don’t include too many reviewers — more reviewers increase review time (not due to the number of comments, but because of social loafing).
Who signs it off
You, a software engineer, a Code Author, are responsible for overall changes and for the PR itself. You should merge or decline your PR depending on code review and testing results, not the code reviewers.
Code Author Guide
The Code Author is personally responsible for the submitted PR. The code author should make sure that the PR:
- actually solves the problem statement from the associated task (usually JIRA ticket) in the way it was preliminarily discussed (we assume that Design stage appears prior to Development in your software development process)
- doesn’t have remaining bugs, logical problems, uncovered edge cases, or known (unannotated) vulnerabilities (whenever possible, provide proof that your solution is working by, for example, testing your code on some environment, providing screenshots, logs etc)
- is clear what should be reviewed and is small and easy to understand
Any necessary refactoring, if possible, should be extracted into the separate PR, to reduce complexity. Any necessary refactoring, if possible, should be extracted into the separate PR, to reduce complexity. See also Ghost Review anti-pattern for detailed explanations.
Self-review checklist
- Design matches the implementation
- All CI checks passed
- Backward compatibility was managed were necessary
- Unit tests meet requirements
- Code was annotated were necessary
- You did self-review (you reviewed your own PR)
Working on Code Review comments
- Don’t take it personally: the review is of the code, not of you
- Kindly ask questions (@mention specific person if needed, don’t make demands)
- Consider separate slack chats and/or 1:1 calls in the case of too many questions, post a follow-up comment summarizing the discussion
- Answer/fix all code review comments (unless they were marked as Minor)
- Don’t hesitate to create separate tickets if code review comments significantly change the scope
- Respond in a timely manner
- Just review your notifications from time to time and let the reviewers know when you provide feedback
- Leave an explicit message when you finished responding to code review comments (that could easily notify the code reviewers to take another look at your PR)
Code Reviewer Guide
Think of why this code should NOT be merged.
If you feel that the self-review checklist (see above) is not completed, just mention the items and ignore the PR request until it is resolved.
Check that the solution meets requirements (ticket description) and modeling (design) first. Then consider the following areas of checks:
- Solution fits overall architecture and meets Corporate Guides
- Over-engineering
- Readability
- Code reuse
- Performance (efficiency)
- Security
- Testing the right things, test completeness
During the code review process please follow general principles like
- Following time constraints
- Provide meaningful comments
- Accept or Raise Concern (as a separate ticket)
Consider it as a good practice leaving special comment meaning that code review is completed and you are looking for answers and adjustments.
Sometimes it’s worth checkout branch associated with PR and review it directly in IDE. Don’t hesitate to do that as well.
Code Review Anti-patterns
Please avoid the following anti-patterns:
- Skip formatting, whitespace and style issues and leave them to automated tools
- If you still need to change what was agreed on at the design (modeling) stage, think about creating a separate ticket.
- Try to be explicit and precise in your comments
- Sometimes someone in the review process simply doesn’t respond
- When the code author makes more changes after more comments and suggestions from code reviewers, which repeats forever
References and further readings
- GitLab | Responsibility of the merge request author
- GitLab | Code review best practices
- Reviewing changes in pull requests
- Trisha Gee — What to Look for in a Code Review
- JetBrains | Core Review Practices
- Five Code Review Antipatterns
- https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/