Code Review Best Practices

4 min readJan 16, 2021

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:

Nit-picking

Skip formatting, whitespace and style issues and leave them to automated tools

2. Design change requests

If you still need to change what was agreed on at the design (modeling) stage, think about creating a separate ticket.

3. Inconsistent feedback

Try to be explicit and precise in your comments

4. Ghost reviewer

Sometimes someone in the review process simply doesn’t respond

5. Ping-pong reviews

When the code author makes more changes after more comments and suggestions from code reviewers, which repeats forever

References and further readings