How to give great code review feedback
Code reviews are a beneficial engineering practice to ensure high-quality and maintainable code and spread knowledge amongst team members. But, the value and the benefits teams get out of code review rise and fall with the value and usefulness of the code review feedback. In this article, I show you how to give great code review feedback. The findings are backed by research and experiences of hundreds of high-performing engineering teams at Microsoft.
This article is part of a large code review blog post series you might want to check out. If you are interested in code reviews, make sure to check out my exclusive code review e-Book.
Code reviews are about problems with and the quality of the code
In a code review, recent code changes of one developer are inspected and discussed by other developers. The focus and goal of such a code review are to find problems with the code and to ensure the code is of high-quality. Even though code reviews provide a much broader set of benefits, such as knowledge dissemination, learning and mentoring, it is important to keep those two main goals in mind.
Some team fear and some teams experience the main drawback of code reviews: reduced code velocity. This means, that the team’s productivity is reduced because code reviews slow the team down.
But why is that?
The causes for reduced velocity can be manifold, but often it has to do with long waiting times on feedback and slow response times. If this is paired with meaningless code review feedback, code reviews become a nightmare for all involved. But teams can easily circumvent those code review pitfalls by implementing proven best practices.
In this article, I focus on the type of feedback that makes code reviews beneficial and valuable for your team.
Code review studies at Microsoft
At Microsoft, I have performed several studies to understand code reviews. In one of those, we analyzed over 2 Million code review comments to understand which code review feedback is valuable, and which is a waste of time. But let’s get started with what to look for in code reviews.
What should you look for in Code reviews?
Let’s imagine you just have been asked to review some code. The code author sends you a couple of code files, with a short description of the purpose and type of change she or he implemented. So, now you look through the code. What are you looking for?
- Functional Defects
- Problems with the logic
- Missing Validation (e.g., edge cases)
- Usage of API
- Design Patterns
- Architectural Issues
- Testability
- Readability
- Security
- Naming conventions
- Team Coding Style
- Documentation
- Use of best practices
- Language-specific issues
- Use of deprecated methods
- Performance (e.g., complexity of the solution)
- Alternative solutions…
Wow, that was a lot. To look for all those issues in a systematic way, it is a good idea to use a code review checklist, that you can quickly go over to make sure you aren’t forgetting anything. You can find my complete and detailed checklist for code reviews also on GitHub.
Now, that you saw all those issues, you asked yourself, but which of those are the most valuable issues to look for?
Which code review feedback is the most valuable?
Let’s pack up a bit and let us imagine again how you actually start the code review.
Probably once you open the review, you will look through all the files and start orientating yourself. Where and what has been changed? What parts of the code are affected? Where is the center of the change?
And while you familiarize yourself with the change, you will notice some issues: typos in comments and variables, missing comments, style-related issues such as indentation. And even though these are relevant and beneficial issues to look for, don’t get caught up in those issues. The fact is, those are valuable and relevant but shouldn’t be the main purpose of code reviews. I’ll explain in a bit.
So, what else are you looking for?
Feedback on defects, missing validation, and best practices are most valuable
Yes, the most valuable code review feedback is about actual problems with the code. This type of feedback is perceived by all developers as the most useful and beneficial. But, in the study, we also saw that those issues make up only a small part of all the code review feedback.
In the figure below, you see which kind of issues are discussed in code reviews, and also what their perceived value is from the perspective of the code review author. So what do we see?
We see that the most valuable comments in code reviews address the following issues:
- Functional defects. This is like a no-brainer. The most highly rated code review feedback is when a reviewer finds a functional defect in the system. But, code reviews are not the best tool to find functional defects. In fact, only a very small part of all comments are about functional defects. Nevertheless, if one is found, the pay-off of code reviews is obviously huge.
- Missing validation and corner cases. Code review feedback that shows validation scenarios that have been forgotten, problems with the logic, or corner cases that have not been covered is also highly valued by developers. This feedback revolves around finding edge cases and alternate scenarios in which the current implementation would fail.
- Best practices and coding conventions. Code reviews are very beneficial for ensuring a coherent, maintainable, and understandable codebase. Therefore, feedback that points out and identifies code that does not follow coding conventions or best practices is also very highly valued.
- API usage and design patterns. Other highly valued code review feedback focuses on how APIs or third-party libraries are used correctly and on missing or wrongly implemented design patterns.
Code Review feedback can be a two-sided sword
Some of the issues discussed in code reviews do not reveal their value as easily as finding defects. Depending on the concrete feedback and circumstances those comments can either be valuable or wasting everybody’s time. Maybe you have already guessed it. But we are talking about issues with the coding style, coding conventions, and comments. Often such issues are referred to as nit-picking issues.
Documentation, coding style, and coding conventions. Addressing missing or outdated documentation, highlighting typos in comments, or pointing to badly named variables, is the feedback you will get often during code reviews. But is this really valuable?
Sometimes the value of such feedback isn’t immediately visible to the code reviewers. And finding a typo isn’t really a biggy, is it? Well, the real value of such comments comes from the compound effect over time. Resolving such issues fast ensures the code base stays comprehensible and maintainable over time.
Still, they can come across as “nit-picking”, and already the word has a negative connotation to it. So, teams should make sure the value of such comments is understood by the whole team.
On the other hand, it’s important to avoid lengthy and repeated discussion on such issues as indentation, or coding style. This definitely slows the productivity of teams down. To ensure teams stay productive, let’s settle on one style convention and move on!
A no-go is feedback that is beyond the code review purpose
Most feedback that focuses on the current scope of the code review is seen as valuable. But, the scope of the code review isn’t all code that is visible in the files within the code change. Neither is it beyond the purpose of the code change. Because of that, opening up new issues that are out of the scope for the current implementation is experienced as not useful for the majority of the developers.
- Alternative implementations. Even though alternative solutions that improve the code are seen as valuable, discussions of alternative implementations that have no obvious benefit for the current piece of code aren’t in favor of your team’s productivity.
- Existing technical debt and refactoring: Similar, starting to highlight old technical debt or potential refactoring opportunities is beyond the scope of a regular code review. Those issues should be discussed in a separate thread.
- Planning and future work. Another not useful feedback type are comments that highlight future work or work that isn’t planned for the current development cycle. If you see such issues, take notes in tools such as backlogs, or issue tracker, where they are actually valuable for your team. And then, discuss them in situations where such discussions are appropriate.
- Asking questions merely to understand the implementation. Even though code reviews are an extraordinary tool for learning and to spread knowledge within the team, asking questions merely to understand the code isn’t the purpose of code reviews. Do not forget, the main goal of the code review author is to get the code approved so she or he can move on.
What should you do if you do not understand the code?
So what do you do if you do not understand the code? How should you give valuable feedback to code that you do not understand?
Good question. And, actually, research and experiences taught us, if you do not understand the code, you aren’t in a place to give valuable feedback. At least not a lot of it.
If that is the case, you have to better understand the underlying issues first. Why aren’t you understanding the code? Are you new to the team? Do you have limited experience as a software engineer? Haven’t you worked with the codebase before? Is the newly written code an incomprehensible mess?
If the last one is true, all your questions are valid and should be part of the code review. But probably, you can add much more than questions. Probably, you add feedback on how to improve the code, why it is incomprehensible, and so on.
What should you do if you aren’t familiar with the codebase?
If you haven’t worked within this codebase before, chances are you have a hard time understanding what’s happing in the code review. A good way to resolve this is to ask a colleague if she or he can walk you through the code and explain it. The important thing here is to not use the code reviews to ask a random question about the codebase.
It is true, learning and spreading knowledge, are two substantial benefits of code review. Still, those should be side effects of doing focus driven code reviews. And the focus should be to check whether or not the code is correct, and of high-quality.
This is, unless, the code reviews are explicitly set as a way to teach you. If that’s the case, this should have explicitly communicated to you. But, if you are added to “normal” code reviews, it is better to just observe and learn what others are doing. Over time, you will understand the codebase better, learn about team conventions and best practices, and the possibilities to add valuable feedback to the code reviews will present themselves.
Code reviews are an excellent tool to learn and spread knowledge within your team. Still, don't ask questions merely to get familiar with the code base. Click To TweetLess experienced developers give less valuable feedback
No, it isn’t just you. It also isn’t the junior developer’s fault. It is just a fact. Our research findings show that experience levels highly impact the ability of a developer to give useful feedback. Code reviewers that just started within an organization – independent of their seniority – give significantly less valuable feedback for about three months. After that, we can see how their feedback value increases and plateaus over the course of a year.
You have to be familiar with the code to give valuable feedback
Multiple code review studies have shown that the most useful code review feedback comes from developers that have either changes or reviewed the code under review before. The good news is, that it is enough to edit a file before once. That means, that in our study we saw no significant difference between feedback from developers that changed the code once or developers that changed the code hundreds of times, as you see in the figure below.
Domain experts can boost your code review value
Nevertheless, domain experts from other teams or cross-team reviewers can add tremendous value to your code reviews. You might choose to add for example security experts, big data experts, or usability experts even though they aren’t as familiar with your codebase as your own team.
The benefit here is that they bring this specific expertise and an outside view. Their purpose in the code review is also different. They probably aren’t looking for design best practices, and team conventions, but inspect the code for exactly the issues you added them for, for example, security.
Treat others as you would like to be treated yourself
Code review is a very social activity. And in companies and teams where people actively work on their feedback culture, it is a highly appreciated and highly-valued engineering practice. Unfortunately, this isn’t the case everywhere. In some teams, code reviews are misused as a means to demonstrate power or superiority or even to have power fights. This isn’t helpful.
If you set out to benefit from code reviews, it is wise to reflect on how to give constructive feedback. Pointing out that something isn’t of high quality isn’t enough. If you critique the code of a colleague make sure to do so in a respectful manner and always include concrete ways for improvement.
On the other hand, adding too many compliments about code pieces in code reviews isn’t necessary. In the code review study at Microsoft, we saw, that code review authors did not value comments that praised their code.
Why? Well, again it comes down to the focus and goal of code reviews. Normally, each comment is a small work item. Having more of those, even though they are just compliments, does not add value. It just aggravates the task of processing the comments.
Pointing out good work and definitely helps the team spirit and is a good motivator. But again, it is better to do so in a different context, like a meeting, or a coffee break than in the context of code reviews.
Outside circumstances that influence the value of the feedback
There are also a couple of things that influence how much value you get out of code review. In our study, we saw that developers just have a hard time reviewing non-code files, such as configuration or build files. On the other hand, developers give the most useful feedback for source code files.
Something else that drastically impacts how good the code review feedback is is the number of files under review. The larger the changeset under review, the less valuable code review feedback you will receive. Keeping chode reviews small just has many benefits, and is one of the most valuable code review best practices.
To summarize, good and valuable code review feedback is feedback to focuses on the goal of the code review: to inspect whether the current code change is correct and of high-quality. Discussions that aren’t helping to achieve this goal should happen outside of the code review process. Because good code review feedback is also feedback that is given in a timely manner and can help the code author to quickly get the code change approved.
Code review checklist and more code review insights
If you want to get more code review insights, make sure to subscribe to my mailing list. That way I keep you updated on every new post. As an email subscriber, you get quite a few nice perks, such as my code review e-Book or the downloadable code review checklist.
Finally, let’s discuss and connect on Twitter.
