Code Review Best Practices for High-Quality Software

The activity of systematically examining computer source code with the intent of finding mistakes created or overlooked during the development process, thus improving the overall quality of the software.

Processes to do it

There’re commonly three processes to do Code Review.

Pair Programming (Mostly used in the Extreme Programming)
Informal Walk through
Formal Inspections

We commonly use following ways during code review:

Over-the-shoulder – One developer looks over the author’s shoulder as the latter walks through the code. In case of remote working, Visual Studio live share is one of the common ways to do it.
Email pass-around – Source code management system emails the code to reviewers automatically after check-in is made.
Pair Programming – Two authors develop code together at the same workstation, either on same desk or remote. This is common in Extreme Programming.
Tool-assisted code review – Authors and reviewers use specialized tools designed for peer code review. Pull requests on Github and azure Devops are good examples of this.

Why do we need to do code review?

Consistent design and implementation throughout the project: There’s no one right way of writing code. Every developer has his/her own style of doing it. This can sometimes lead to a lot of different styles of implementations/designs to a similar problem, in one project. Code Reviews and discussions toward implementation, will help bringing consistency to that.
Minimizing your mistakes and their impact: The most obvious benefit of code reviews is that it will pick up mistakes very quickly. A wrong checked-in code will waste time in many ways. For example; Wasting your time in deploying or QA’s time in prioritizing your task and the focus gets shifted to not-so-worthy areas. Hence, code review is an opportunity to reduce that impact.
Ensuring project quality and meeting requirements: It’s very common for developers to misunderstand the requirements and make assumptions about future requirements. Picking up issues at an early stage will improve the quality. Picking up late will make it patches.
Improving code performance: One of the key focus of code review is performance of the code. So, code reviews always result in better Performance.
Sharing new techniques: Sharing is Caring. Code review gives you a lot of opportunities to share trickiest tips/techniques with your colleagues. It also gives you equal chances to read your colleagues codes and learn from their techniques.
Code reviews helps with better estimates: When it comes to estimating a problem’s solution; Consistency in Project structure, knowledge of your team member’s modules and equal understanding of the techniques helps a lot. All of the three things improve with code review.
Code reviews enable time off: With code review, you start owning your teammate’s modules. There’s no one guy who owns a modules/page. Which means everyone gets to take some time off without creating a Business Risk.
Code reviews mentor new engineers: It mentors new developers in many ways. It creates awareness about the things that senior developers focus on. It gives new developers the scope to learn from their Sr. fellows. It gives them opportunities to ask questions like why things are being done differently from their understanding. Important: It’s very important the Jr. developers’ comments on Pull requests are taken as seriously as a Sr. developer.
Code Reviews Reduce Bugs/Defects (by 60% as per global stat): Yes, you’re right, I do not have any data to back this up. But I read in many blogs that Code Reviews reduce the bug count by 60 to 80%. I decided to believe the 60%.
Code Reviews Improve the Speed of Development: If we measure, speed of development= number of lines of code. It’s the number of Stories that met definition of done. With reducing bugs, high quality code, reusing of code, focusing on right solutions – it’s a No-brainer to understand that it improves speed of Software Development.
Code Reviews Encourage a Healthy Engineering Culture: This is one of the biggest gains of a Code Review process. It encourages a healthy engineering culture.
A culture where team members care about each other’s task. A culture where giving critique feedback is seen as a positive thing. A culture of Collaboration, Courage, Trust, Growth and Continuous Improvement.

“Code reviews help spread knowledge through a development team. Reviews help more experienced developers pass knowledge to less experienced people. They help more people understand more aspects of a large software system. They are also very important in writing clear code. My code may look clear to me, but not to my team. That’s inevitable–it’s very hard for people to put themselves in the shoes of someone unfamiliar with the things they are working on.” –
Martin Fowler, Refactoring: Improving the Design of Existing Code

So, what exactly is a code review?

Are there any obvious logic errors in the code?
Looking at the requirements, are all cases fully implemented?
Are the new automated tests enough for the new code? Do existing automated tests need to be rewritten to account for the changes in the code?
Does the new code conform to existing style guidelines?

Four sure-shot Rules in the Process of Code Review

In my opinion, four rules must be followed without exception, to successfully integrate code reviews into a team:

Everyone gets code reviewed: If you write a code, whether it’s x86 assembler or HTML; whether you’re an intern or the CEO; Every one of your commits needs to be reviewed.
Everyone reviews code: We can all learn from code and we all have something to teach. Making sure that everyone on the team participates in reviews & ensures that teaching and learning happens.
Every PR gets reviewed: To say that some PRs don’t need to be reviewed is to say that you know which PRs will have defects. Every PR should be reviewed, so flip the switch in your SCM tool of choice and require every PR entering the project to be code reviewed.
Every change gets reviewed: Like the previous but at a lower level and just as important. Every change made within a PR needs a review. Be it code or comment, it all needs a once-over.

Submitting a Pull Request

All code reviews start with a pull request (PR). Every tool handles them a little differently, so you’ll want to check the documentation for the specifics for your tool. When submitting a PR, there are three guidelines you need to follow:

Review your changes: Do this to avoid unnecessary follow up commits. Here you should double check to make sure everything is included in the PR; all caveman debugging code is removed (i.e. Logger.info “got here”); and all the tests pass.
Describe your changes: There was a reason for the changes you made, those need to be provided in the subject and description of the PR. Oftentimes, it makes sense to give reference to the “ticket” associated with the changes. Also, if there are areas you believe warrant greater attention or require clarification, make a note about them in the description.
Request the right reviewers: It seems obvious, but you want the right people reviewing your code; people who will have a background in the PR. It probably wouldn’t make sense for an iPhone developer to request an Android developer to review his or her code.
Keep the Pull Request as short as possible: Try to divide your task into small logical tasks. Submitting your PRs in small portion helps with a quicker feedback cycle.

Reviewing a Pull Request

Once a pull request has been submitted, it’s time to review it. To do that, you’ll want to first be mindful off these four things:

Know what the problem is: It’s hard to tell if a solution will solve the problem if you don’t know what the problem is. To that end, read the description provided in the PR and any associated documentation provided in the ticket.
Complete the review within 4-6 hours: This means exactly what it says. A short feedback loop allows developers to keep focused on the issue at hand. There’s nothing worse than submitting a PR, starting a new feature, and then finally getting feedback a week later.
Take your time: Reading code is mentally demanding and there’s only so much you can do before your focus begins to deteriorate. Try not to exceed 500 lines an hour, and if you must break up the review into multiple sessions. Your brain will thank you, and so will your teammates.
You’re accountable: When you click the “Approve” button, you’re saying that you not only reviewed the code, but that it solves the problem and it is good enough to be included in the code base. You are also taking partial responsibility for problems that arise from it.

What to Look For

When you receive a request for review and you begin looking through the code, what should you look for? Rather than providing you with a list of a hundred code smells, potential problems, or gotchas you might see, I think it makes sense to provide a high-level view of what to look for:

Does the code do what it’s supposed to do? This doesn’t mean you need to start the app and enter into a full QA cycle. You should be able to tell from the code if it is likely to solve the problem or not.
Are there tests? If so, that’s great. Do they pass? Do they make sense? If tests aren’t provided for the code, reject it and explain why.
Has the documentation been updated to reflect the changes? If your project has documentation, make sure the changes in the PR are reflected in it. The only thing worse than no documentation is wrong documentation.
Are things named well? Naming is one of the hardest things about programming, do the names used in the PR accurately describe what they represent or what they do?
Is there duplicate code? As you are reviewing the code, do you see blocks which are duplicated? Would it make sense to extract them into their own class, module, or function?
Are there potential performance issues? If so, explain what they are and provide a solution.
YAGNI (You Ain’t Gonna Need It) Has code been added which isn’t needed yet and isn’t part of the solution? Comment on it. If it isn’t needed yet, it shouldn’t be there.
The Boy Scout rule If there is one of the guidelines you should remember, it’s this: “Has the code been left in a better state than what the developer found it?”

Giving Feedback

If you’re performing a code review, you’re going to have to give feedback. It may be as simple as “Looks good!”, “ship it!”, or even just a “👍”, but more often than that; you’ll need to critique the code, point out issues, and ask questions. As developers, we tend to rush through the communication related tasks to get back to what we’re most comfortable with. In our haste, we become terser and more direct; resulting in unnecessary problems. To avoid any potential problems, try following these guidelines:

1. Ask Questions

Just because someone’s asked you to review their PR doesn’t mean your initial response must be either “Approve” or “Reject”. Instead, think of it as the beginning of a conversation. Like any conversation, there will be points you will need clarification on, in order to keep moving forward. The same is true for PRs. If there is logic or design choices you don’t understand, ask the author to clarify those choices. It may be that he or she doesn’t fully understand it either. If you don’t understand it now, what’s the likelihood you’ll understand it the next time you encounter it?

2. Be Articulate

Text can be a horrible medium for communication. It doesn’t convey facial expressions, tone of voice or any sort of body language. It’s just text. The meaning of a sentence can vary just by where we as the reader put emphasis. For example, read the sentence “Why did you write it this way?” in the following ways:

Why did you write it this way?
Why did you write it this way?
Why did you write it this way?
Why did you write it this way?
Why did you write it this way?

Do you hear the difference? It’s the same text, just read with different emphasis. As much as we hate to admit it, we’re emotional creatures, and as such we bring our emotions with us into whatever activities we do, including reading. If we’ve had a bad day or didn’t sleep well, it’s easy to read feedback in a more negative light than what was intended. It’s therefore incumbent upon the reviewer to take extra care when writing comments.

By taking more time to clearly communicate about what’s good or bad in the code, ask clear questions and provide potential solutions; it reduces potential back-and-forth between you and the submitter. This leads to faster development times. Furthermore, adding clear explanations gives the submitter context and insight into what you were thinking at the time of the review, reducing the likelihood of them taking things the wrong way.

3. Critique the Code, Not the Coder

I’ve never seen this played out within an organization, but I’ve heard stories and I’ve seen it in open source projects. I hate that I even must mention it, but reviewing code is neither time for the reviewer to play a game of “Gotcha” Nor should it be used as an opportunity to belittle the submitter.

We all make mistakes and there isn’t a single individual that gets to know the right way to do everything. When a developer submits their code for review, they’re trusting the reviewer to be fair and provide honest and helpful feedback. Don’t betray that trust.

4. Compliment Good Code

The last thing to remember to do as a reviewer is compliment good work when you see it. Simple comments like “That’s cool!”, “I didn’t know you could do that!”, or even just, “Good job” can be great encouragements to the submitter. It doesn’t have to be done in every code review, but when you see something good, call it out.

As my mom likes to say, “A candle loses nothing by lighting another candle.”
Someone’s mom 🙂

Receiving Feedback

Choose not to be harmed — and you won’t feel harmed.
Don’t feel harmed — and you haven’t been.
– Marcus Aurelius

This piece of the code review process is the hardest: it’s how to receive feedback. It’s the hardest because it involves our emotions and our ego. Unlike other creative endeavors which are subjective in their nature, programming has a strong objective element to it. When an art critic gives a negative review of a piece of work, the artist can always respond with, “The critic failed to see my vision.” With programming, on the other hand, a code reviewer can say a piece of code is wrong and then provide evidence to support the argument. There isn’t a comeback for facts.

On a personal level, receiving code reviews is the most important part of the code review process. It’s at this point that you have the opportunity to grow. Will you listen to the suggestions offered or disregard them? Listening will require humility; you might have to admit that you’re wrong about some piece of your work. You might have to change how you’ve “always done it”. But it’s the only way to grow.

In most cases, the feedback we receive is done with the best intent. Most of the time we review code, we’re just looking for potential problems – it’s rare that someone has a personal grudge against the submitter – so try to see it in that light. Furthermore, both submitter and reviewer want the same thing: high quality functioning software – Hence, focus on that.

In Closing

Code reviews are the most effective activity development teams can adopt to drive down defects. Perhaps more importantly, they’re the best pursuit your team can engage in; to transfer knowledge, increase overall development speed, promote a healthy engineering culture and build a sense of ownership within the team. None of this should be underestimated as healthy engineering cultures are far more effective at attracting and keeping the best developers.

When incorporating code reviews into your organization, remember the four rules.

Everyone’s code gets reviewed
Everyone reviews code
Every PR gets reviewed
Every change gets reviewed

Whether your submitted code is to be reviewed or you have been asked to do the review, remember foremost that your teammates all have the same goal: doing quality work to create a substantial product. You’re on the same team. Encourage one another, hold each other accountable, and regardless of whether you’re the submitter or reviewer you can learn and grow from each Pull Request.