5 Steps of Good Code Review

In this video I'm explaining why code review is a process that requires a lot of thought, especially about the way we communicate our thoughts about the code. I present 5 steps towards successful, productive and positive code reviews.

📚Resources:

• https://research.google/pubs/pub47025/ - Modern Code Review: A Case Study at Google - a research paper about the efficient ways of doing code reviews
• https://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/rigby2013convergent.pdf - Convergent Contemporary Software Peer Review Practices - a research paper comparing code review practices among multiple projects, including open source projects like Linux and KDE
• https://storage.googleapis.com/pub-tools-public-publication-data/pdf/7ac08fa960dfe10561c1f5953419a0c945279faa.pdf - Predicting Developers’ Negative Feelings about Code Review - paper I mention in the episode, that measures what causes negative feelings about Code Review and how to prevent it
• https://www.microsoft.com/en-us/research/publication/characteristics-of-useful-code-reviews-an-empirical-study-at-microsoft/ - Characteristics of Useful Code Reviews: An Empirical Study at Microsoft - a research trying to find what kind of comments during code review are considered usefu and not useful by the code author. This paper partially contradicts my anegdotal experience (although it might be due to different understanding of what "useful" means); nevertheless it is worth checking out

Notes

Introduction

The first mention of code reviews comes from 1976. The process invented then was called "code inspection" and was done by a team of developers who scheduled a long meeting and sat together in a room with their pens and notepads.

The code reviews we do today are completely different. They're informal and asynchronous, they might often take less than an hour,

Modern code review is:

• informal (need to check what it means)
• tool-based (I guess that comes with the more computer power)
• asynchronous (reviewer can do it without author's presence)
• focused on reviewing code changes (what was it focused on before?)

Step 1: know the goals

Code review is a process where code written by one developer is read and then eventually approved or rejected by another developer (or multiple developers).

Usually it is performed once a developer finishes certain part of their work and wants that code to be released to production - what is that part of their work depends on the situation, some companies do code review per feature, some do it per release, in some companies you can split your feature into a few pull requests and have them reviewed and released independently - that doesn't matter for now. What matters are the goals of the code reviews.

I believe the primary goal of the code review is to verify the 4 characteristics of good code:

• is it correct?
• is it efficient?
• is it secure?
• is it well designed?

With particular focus on the 4th one.

The first 3 are also important, but they should also be covered by other processes: automated tests, manual tests, security tests, performance tests - the last part can't be automated as much as the previous ones (I'll talk about automation later today).

So in short: code review should determine whether the code is well designed and easy to understand by other developers. It also provides side benefits, which are finding bugs, security issues and performance issues. Remember, these are side effects.

Step 2: everyone should do it

Yes, everyone!

Ok, that depends on the company culture, on the team leader etc etc. but hear me out: even if your company only requires senior developers to review code, there's a lot of value in reviews done by junior and mid-level developers:

• first is the contribution — from my experience, reviews done by less experienced developers are very insightful, because they are more sensitive to things that are difficult to understand. Junior developer have less experience, therefore less understanding of certain constructs or patterns. By asking questions whenever they notice something that's not clear, they can help the team to settle on simpler solutions
• second is the education — reading and discussing code written by other developers is very valuable, as it helps learn new things and understand certain techniques that might not be obvious without knowing the context and the reasoning behind them
• finally third is team building — by encouraging everyone in the team to participate in code reviews, people get a chance to talk to each other more, to learn more about each other's habits and practices, therefore improving the communication and overall feeling of knowing each other in the team.

Excluding junior developers from certain processes, like core review and planning, might lead to alienation, slower progress and lower satisfaction from work, which might lead people to

Step 3: the attitude

Now that you know that you should do the code review and you know what's the goal, you're almost ready to start. There's one more thing to calibrate: attitude.

Some people are stressed when their code is being reviewed, and I understand them. You produced a piece of work that you believe is of good quality, you spent a lot of time on it and put a lot of effort. And now someone else is judging this code, telling you what's wrong and explaining how you should rewrite it. I must admit it: sometimes code reviews really suck, and I used to do such reviews in the past.

The one thing that helped me to turn it all around and make code reviews one of the most productive and interesting team activities was the attitude. I've realized that I am not there to judge someone else's work, but instead I was there to ensure that we, together, as a team, produce good, correct, maintainable code.

This one mental switch from "me - a reviewer, a judge", to "me - part of the team, a consultant" let me improve the mood among my teammates, my relation with them, as well as the quality of the code. Because when people do not feel judged, and instead they know that your comments are encouraging them to solve problems and improve the code, they are much more eager to follow these suggestions.

Let me emphasise it again: your attitude during code review is a critical part of the whole process.

Treat the code review as integral part of your daily job responsibilities. Ensure that you focus on it just as you focus on writing code. Remember that any comment you made will be read by your colleague and write comments as if you were talking to them face to face (unless you're an asshole, then don't; but even better, stop being an asshole at all).

For example, instead of saying "this is wrong, it should be like this: ..." say "I believe there's a bug here, did you mean ...?".

Instead of saying "this design sucks, it makes no sense", you should say "I think this can be done in a more efficient and readable way". Make sure to remain a positive attitude, even if the quality of the code is subpar. The negative comments might be perceived as more personal and aggressive. Below this video I'm posting links to some research around code reviews, make sure to check it out!

In short: don't put other developers down, lift them up and help them to become better.

Step 4: the process

Ok, now the process. Code review is a complex activity, and in some cases requires as much focus and attention as writing code.

In order to do this well, make sure to prepare well:

• schedule time for it - unless the pull request has less than 20 lines of code changed, don't do it during 10 minutes break between 2 meetings
• read the task description and see if there are any comments there, make sure you understand the acceptance criteria
• read the description of the pull request, see if the author pointed to any specific parts of the code
• start from the overall design of the feature — does it solve the problem? Is it easy to extend this code in the future? Can you understand why it was solved the way it was?
• stay focused on the important parts of the code. Things like consistent indentation or naming are easy to spot, but they should be detected and when possible fixed by automated tools. You should focus on the things that machines can't detect. If you don't have automated tools for that, you should keep marking such issues, but make sure to automate it as soon as possible
• if you notice something that's been done well, make sure to point it out, and say "good work" or "nice idea!"
• if you have some suggestion which is more a matter of preference than difference in quality, either make it very clear that this is not a requested change, just a remark, or simply don't write it. It is confusing for the authors of the code when you write such suggestions, because they don't know whether you expect them to change the code or whether they can ignore it.
• if something is a matter of preference, and is not a convention that the team agreed on, the author of the code chooses which version they prefer
• finally, before you publish you review, reread your comment. If some of them might seem to harsh, change the tone, make it positive
• if there are some really bad mistakes in that code, it might be good to talk to the author and go through the code together and explain them why it is bad. Usually when talking face to face we tend to be more respectful, because we immediately see the reaction of the other person; you can use the pair review technique I mentioned in the last episode

Step 5: the results

• the process of code review ends in 2 states: either the code is accepted and passed to other stage like testing or it is rejected and closed
• if you request some changed, it doesn't mean that the code review is over. The next step is a discussion over certain requests, and you and the author should agree on what changes will be introduced and which won't. The agreement should be that once the changes are introduced, you will review the new code and unless it introduces other problems, you will accept the pull request
• the worst that you can do is to request new, unrelated changes in every round of code review. this leads to a lot of frustration and negativity, try to limit the number of rounds to 2, at worst 3
• try to make the next rounds of review faster; the changes should me much smaller, so it won't take you a lot of time; don't be the person that delays the release. Make sure that you review the new changes, because you have reviewed the previous ones, it will be much easier for you