Code Reviews Done Right: Your Missing Guideline
“Your code sucks!” – It’s usually not what the reviewer said, but that’s how we perceive it sometimes when our code is being reviewed. We feel challenged or even provoked. Our pride is at stake! So we either begin lengthy discussions to defend ourselves or we blindly adapt the code for the sake of friendship and harmony, silently still believing that our own solution was the best. The reviewer just didn’t understand it. Sounds familiar?
Best Practices for Code Reviews
Code reviews are an essential part of our daily routine as professional app developers. They are crucial for creating stable and maintainable software in teams. Strange to say, no-one ever tells us how to do code reviews. We just do it, somehow.
There are tons of online courses and tutorials available for learning how to write code – but almost none for learning how to review code. As a result, every developer does it differently. That’s not inherently a bad thing, but it can be a problem. What’s worse: We frequently fall into some typical behavior patterns that dramatically reduce the value of the entire reviewing process. We waste time, money, energy and patience without really improving the quality of our code.
So how to do it the right way? This article introduces some typical pitfalls and explains why things can go wrong in code reviews. It provides you with a handy toolbox of tips and tricks to avoid those mistakes and best practices of how to do code reviews properly and efficiently.
The Reason Why
Evaluating the quality and efficiency of our code reviews is only possible if we are clear about our goal:
Why do we do code reviews?
Here at QuickBird Studios, we have three main reasons:
1. Increasing Code Quality
We put a strong emphasis on the quality of our software. Some of our apps are used in the medical sector by doctors and patients. They need to be 100% reliable, stable, maintainable, testable and easily extendable. No developer is perfect, but everyone is different and has a unique perspective. Code reviews enable us to discover possible bugs or architectural flaws early on, before even manually testing and delivering the app.
2. Learning
As developers, we never stop learning as our programming languages and concepts constantly evolve. Code reviews are an important part of that learning process for both the reviewer and the reviewee.* The reviewer often discovers new ways of solving a particular problem while reading through the code or asking questions. The reviewee receives important feedback and valuable suggestions on how to write cleaner or more efficient code.
3. Knowledge Exchange
Code reviews are also a great means of project- and company-specific knowledge exchange. When developers get onboarded on a new project, code reviews help them to understand the structure of the code faster. They implicitly learn how “things are done” in this project. While it’s always advisable to have the most important guidelines written down in a Readme file or something similar, code reviews speed up the process of “diving in”. Long-time developers benefit as well, because they become aware of the most recent changes in the code base.
There might be other reasons for doing code reviews that matter for your team, but it’s probably safe to say that the reasons mentioned above play an important role for every team.
* We use the term “reviewee” as a shorthand for describing the person whose code is being reviewed. Despite the fact that this word doesn’t exist in English language and might sound awkward to native speakers, we find it less verbose that writing “the person whose code is being reviewed” 17 times in this article. 😉
🚨
Perils
Common Pitfalls For The Reviewee
Now that our goal is clear, let’s have a look at the typical behavior patterns that interfere with that goal! We will first inspect the common pitfalls that the reviewee is facing, i.e. the person who writes the code and submits the merge request.
1. 📚 Overly Large Merge Requests
Let’s be straight: Code reviews are usually no fun. As developers, we favor writing code much over reviewing someone else’s code. So apart from some boring meetings, there is only one thing that’s worse than reviewing merge requests: reviewing large merge requests! And there’s a reason for that:
First of all, it simply takes longer to review a large merge request. It’s quite exhausting to do a good code review and if we read through thousands of lines of code for hours, our minds go crazy. We get tired and frustrated.
Secondly, an essential part of the reviewer’s job is to try to see the big picture, how the different pieces of code interact and fit together. We need to understand the structure of the code and its underlying patterns. That becomes increasingly harder the larger the merge request gets. Our minds only have a certain capacity and if too many things changed in a single merge request, that capacity might be exceeded. Reviewing the code then becomes much more exhausting for us and we might miss important problems. No wonder we get heavily demotivated when confronted with a seemingly never ending merge request…
On the other hand, when we write code ourselves, it’s usually hard to resist the temptation to change more lines of code than we are supposed to. Sometimes, we spontaneously feel like it’s time to do some refactoring first. Our new feature implementation of 200 lines might drown in 1000 lines of refactoring code, making it really hard for the reviewer to see the feature changes in context. At other times, we feel like we want to add some other cool feature while we’re at it, either to impress someone or because it’s fun. We could do that in a separate merge request but then we are too lazy to create a new branch and possibly a new ticket for this “tiny little change”, which frequently turns out to be much bigger in the end. We feel like we are evading a “bureaucracy overhead” while in reality we are creating a bigger overhead on the reviewer’s side.
2. 🛠 Over-Engineering due to Appreciation Bias
We developers like to be perceived as smart – especially by other developers. Smart is usually associated with elegant and elegant is associated with short, abstract and modern. But that’s not always what’s best for the project!
Overly short code can be more difficult to read. Abstract, generic code can obscure the intent and usually takes more time both to write and understand. Modern code that uses cutting-edge technology might be fancy, but not the best solution for the problem. Yet, in many teams, that’s the standard by which we measure ourselves as developers.
When you were writing your code, you might have gone through a complex thinking process, considered all options of abstraction and in the end arrived at the conclusion that the simple, straight-forward and non-reusable option might be the best for the particular use case. Unfortunately, the reviewer doesn’t know that. All they see is the final result of inelegant code. As a consequence, they might come to the conclusion that you didn’t fully understand the problem or that you simply lack skills as a developer. Or at least that’s what you fear when writing the code.
For this reason, we tend to over-engineer our code when we know that it’s going to be reviewed. We lose time thinking and implementing ultra-composable, generic code that’s never reused and the reviewer loses time understanding your brilliant solution, just as other developers do when they need to touch your code in the future. The only win is that the reviewer might get convinced of your brilliancy at some point. Everything else is a loss.
3. 👩🎨 The Creator’s Pride
Let’s be blunt: The code you write is your baby! Just like an author’s text, an artist’s painting or a musician’s song, we developers treasure and protect our own code. In most cases, we’d rather have no-one mess around with it. It’s our piece of art!
As a result, we are usually biased towards our own solution. Some requested changes might not really be up for debate as they are objectively a mistake, like a bug that leads to a crash or a simple typo. But for less obvious change requests, we often fight for our own solution – even if that solution is not as good as the one suggested. Instead of doing what’s best for the project, we try to find reasons that support our own solution and if we only try hard enough, we might even convince the reviewer.
Common Pitfalls For The Reviewer
Now let’s turn the page around and look at the typical pitfalls the reviewer is facing. Here’s what can go wrong when you review code:
1. 😎 Showing Off
Just as a reviewee has pride in their craft, so has the reviewer. It might be conscious or unconscious, but sometimes we just add a comment because we want to impress or to strengthen our position as an experienced developer. We suggest “improvements” not because they really improve the code but because they show our advanced skills. This is more likely to happen if the reviewer is senior to the reviewee.
2. 😰 Fear of Showing Weakness
The opposite effect can occur as well. We notice something weird or don’t really understand what the code does, but we neglect to mention it out of fear. After all, if we ask a question in a merge request, it might be interpreted as a weakness. If we do that regularly, we might be judged and perceived as a bad developer. Our reputation decreases.
3. 😬 Fear of Annoying The Reviewee
If we exceed a certain number of comments we add to a merge request as a reviewer, we often start feeling bad about it – because we know: Each additional comment means more work for the reviewee. The more comments we write, the more we fear that we might bother him or her too much. They might perceive our review as an attack if we criticize everything. Consequently, we have a subconscious throttle integrated in our reviewer mind: After a certain threshold has been reached, every comment we add reduces the likeliness of us adding more comments, even if they would be equally or possibly more important than the existing comments from our perspective – another reason for keeping merge requests small.
4. 🤬 Offending The Reviewee
Many of us don’t like to admit it, but developers are humans as well and as such, we do have feelings that can be hurt – may it be conscious or not. Depending on the language we use and the fierceness of our comments, the reviewee might feel offended or judged. We might give him or her the impression that we value our own opinion more than theirs. This will either poison the climate in your team or reduce the reviewee’s confidence in the long-run. As a result, the reviewee might be more likely to give in and agree to the reviewer’s “demands” – even if his or her solution is the better one.
5. 👀 Wrong Focus
Another typical reviewer behavior is to focus on the least important things. If you’ve been working in the software industry for a while, you’ve probably come across those code reviews that contain dozens of comments on personal formatting preferences, but not a single one on the actual logic of the program: “Add a new line here”, “remove an empty line there” or “wrong indentation”.
Of course, it’s great to have those things consistent. New lines and indentation guide our eyes and help us reading and digesting chunks of code faster. The problem is that too many of those comments keep us from focusing on or even discovering the more crucial issues that might lead to a crash or unexpected behavior (see pitfall #3).
This happens either because the reviewer never learned what to look for in code reviews or due to time pressure. It’s much more challenging and time-consuming to review a merge request when you try to understand the program flow and the logic as compared to looking for formatting issues only. Unfortunately, we still get rewarded with a feeling of accomplishment when adding a dozen comments of little importance and to others, it appears as if we’ve done our job.
6. 🥱 Lack of Motivation
Time pressure, exhaustion, large merge requests and other obligations can also lead to a lack of motivation for a developer to do code reviews properly. This can lead to low quality reviews with little value (see pitfall #5).
7. 🤔 Lack of Context
Sometimes it’s good to have “a complete stranger” take a look at your code. They see it under a different light and might find flaws in the architecture for which the project team might have developed a blind eye. That’s why consulting agencies exist!
However, in regular code reviews it’s usually a big disadvantage if the reviewer lacks context and doesn’t know anything about the project. Without knowing the purpose and the general structure of an app, it’s much harder to find programming errors and hence many of those slip through our fingers.
💡
Solutions
Best Practices to Make Code Reviews Valuable and Efficient
As we have seen in the previous sections, there are lots of social and technical obstacles in our way when doing code reviews. Each one of them can prevent us from reaching our goals:
- Increasing Code Quality
- Learning
- Knowledge Exchange
How do we overcome those obstacles to make code reviews valuable and efficient?
It’s crucial that we follow some basic rules, both in the role of a reviewer and in the role of a reviewee. However, creating the right working environment is equally important.
Creating The Right Environment
There are a couple of conditions on which we only have a limited influence as individual developers: things like workflows, processes or company culture. But depending on the size of your team, you might be able to improve some of those conditions with the right arguments.
1. ✅ Use Linters
A linter is your personal digital reviewer. It analyzes your code and throws warnings for stylistic errors and possible bugs. There are different linters available for each platform, most of them can be customized by creating project- or company-specific rules with regular expressions. (For iOS we use SwiftLint, for example.) It should be a best practice to use them because they solve two problems at once:
On the one hand, linters can probably find and enforce more than 95% of any formatting issues. If you agree on a common style guide for your team, you can customize your linter accordingly. Before creating a merge request, developers must resolve all linter warnings. (This can be enforced by a continuous integration pipeline.) Reviewers should then not make any comments on stylistic issues by default and only start a discussion when the linter obviously missed a rule that was previously agreed upon. As a result, you won’t lose any more time on personal formatting preferences, arguing whether there should be a new line, an additional whitespace or not. This helps you focus on more important issues when reviewing code, like programming errors or architectural flaws.
On the other hand, a linter reduces the potential for arguments and the risk of offending the reviewee. As Mathias from our team put it:
People get angry when another person tells them how they should style their code. They take feedback way better when it comes from a machine.
A linter might still be annoying at times, but at least it can’t hurt our feelings. It’s neutral by design. You can be pretty sure that it doesn’t follow its own agenda and it won’t pull your leg just to show off how smart it is.
2. ❤️ Create a Culture of Mutual Respect
A great team and company culture is a fundamental requirement for efficient code reviews. If asking questions and showing knowledge gaps are interpreted as a weakness in your company there is definitely something wrong and you should try to change it. If that’s not possible, change the company.
With an established culture of mutual respect, developers are less afraid of speaking up when they don’t understand something. They are more likely to ask questions in code reviews, which usually either turn into a learning for the reviewer or for the reviewee. In many cases the reason for the reviewer’s lack of understanding is that the code is not as readable as it could and should be.
3. 🗣 Talk to Your Coworkers!
Personal feedback and looking each other in the eye is worth a lot and often underrated by developers. There is a lot of non-verbal communication going on when you talk to each other in person, most of which is subconscious. Thanks to that, it’s much easier to detect if your coworker feels uncomfortable because one of your comments bothered him or her. From the reviewer’s intonation and body language you can usually tell if a remark is meant as an insult. In most cases it’s not, but if the reviewer does sound hostile to you, it’s easier to address the problem directly. With eye-to-eye contact you can resolve conflicts faster or prevent them before they emerge.
Regular personal contact helps tremendously to realize that the reviewer is not an anonymous enemy who constantly tries to keep us from doing our job, but rather a valued colleague and a human being who respects us. Sometimes, it’s easy to forget that – especially when working remotely for most of the time.
This doesn’t mean that you need to do every code review together. But it’s usually beneficial to do a “live” reviewing session every once in a while. At QuickBird Studios, we also give each other a two-way 1-on-1 feedback from time to time. This ensures that we remain honest with each other and increases our mutual trust.
4. 👉 Assign Clear Responsibilities
When you resolve issues in code reviews, you should come up with a solution that both developers are fine with by default. But defining clear responsibilities is a best practice that often helps in conflict situations. At QuickBird Studios, we have a simple rule:
Only the person who opened an issue in a code review may resolve it.
This ensures that a developer cannot simply ignore a comment – he or she has to either adapt the suggested changes or to win the reviewer over with solid arguments. Due to this process, there is usually a great learning effect on both sides. If the exchange of comments runs the risk of escalating into an endless discussion thread, we talk in person (see #3) or do pair programming.
We also assign the ownership for merging requests to the reviewer. When a merged branch doesn’t build or has some other issues that could have been discovered during code review, it’s the reviewer’s responsibility. Having these roles clearly defined motivates the reviewer to take code reviews seriously and to look beneath the surface. It also prevents bitter feelings when one developer just merged his or her branch without the other one’s approval.
5. ⏳ Allocate Enough Time For Merge Requests
A lack of time is the number one-killer for proper code reviews. It reduces motivation and causes developers to review superficially only. Thorough code reviews are challenging and time-consuming. Therefore, when you estimate how long it will take to implement a ticket, you should always consider and include the reviewing time. Your project manager or team leader should then allocate the necessary time for you.
6. ↔️ Always Review Each Other
This is a matter of course in most companies, but it’s so important that it shouldn’t be missing in this list. As a best practice, make sure that you always review mutually. It’s much easier to understand each other when every developer regularly assumes both roles: the one of the reviewer and the one of the reviewee.
The reviewer usually has the stronger position and it’s important for them to see how it feels to be in the weaker position as well. Reviewing each other contributes to keeping a balance of power and mutual respect.
Best Practices for Being a Good Reviewee
As the person who created a merge request, there are several things you can do to improve the results of the review:
1. 🔍 Keep Merge Requests Small!
As an experienced developer, it’s your responsibility to keep your merge requests small. 500 lines of code changes are usually still within an acceptable tolerance, but if you exceed 1000 lines of changes, you’ve certainly reached the threshold at which the reviewer loses motivation and performs the code review less thoroughly.
If a merge request is too large, it’s usually due to one of the following reasons:
- The ticket or the user story you implemented was too large in the first place. If that’s the case, you can learn from it and split large tickets up into smaller (sub)tickets next time. If you don’t have permission to do this yourself, you should discuss this with your team in the next planning meeting.
- You changed parts of the code that were not related to the scope of the ticket or user story. This happens when we spontaneously decide to refactor code because it seems like the right thing to do. In many cases, this refactoring might be beneficial to the project, but it dramatically reduces the quality of the subsequent code review. The number of changed lines related to the refactoring usually exceed the number of changes lines related to the actual task. Thus, it gets much harder for the reviewer to see the right connections and to make sense of your changes. If you encounter a situation like this where you feel a necessity to refactor large parts of the code, just create a new ticket for it and a separate merge request.
In conclusion, if you want to have a motivated reviewer doing thorough code reviews without procrastinating them, follow the best practice of keeping merge requests small!
2. ⭕️ Keep it simple!
Don’t try to impress or show off with the code you write. Instead of over-engineering, carefully evaluate the requirements and find a solution that best solves the problem in the most expressive way. Keep in mind that every layer of abstraction adds complexity and may cost your team valuable time. There is no silver bullet to kill the appreciation bias, but being aware of the effect is really helpful.
3. 🛡 Don’t Defend Yourself!
There is no reason to defend yourself when your code is being reviewed. (Hopefully) you’re not being attacked! Due to our creator’s pride it’s not always easy for us to accept the reviewer’s suggestion. If you are convinced that your solution is better and you have solid arguments for that, you shouldn’t give in easily, of course. But you should always reflect whether you’re only desperately trying to push something through because it’s your solution.
A strong indicator that you fell victim to the creator’s pride is when you argue that your code is “not that bad”. That’s a clear sign that you’re trying to defend yourself or your pride instead of trying to find the best solution possible. If the reviewer has a better solution and the time allows it, it should be implemented. Period.
4. 🧑🎓 Learn From Each Review!
When going through the comments the reviewer added to your merge request, don’t just adopt the suggested changes blindly! Try to understand why the reviewer suggested those changes and ask yourself if they are actually improving the code. This is your opportunity for learning and becoming a better developer – don’t waste it!
Best Practices for being a Good Reviewer
A code review only achieves its goal when both sides are working together. Here are the guidelines that you should adhere to when being in the role of the reviewer:
1. 🧩 Understand The Code Structure!
If possible, make yourself familiar with the project and the code base before reviewing code. If you’re new to a project, ask a colleague developer give you a general overview and introduce you to the core concepts. This will help you understand the program flow better and make it easier to spot structural flaws.
2. ❓ Ask Questions!
Do not let your fear of showing weakness take over! If you don’t understand a piece of code, don’t just jump over it in the assumption that the developer who wrote it probably knows their stuff and you’re too stupid to see that. There are two possible scenarios:
- Your fear turns out to be true and the implementation actually makes sense. In this case, you’ll learn something new from the answer you receive. You’ll become a better developer step-by-step.
- The code is really illogical, incorrect or difficult to read. In this case, it’s important to fix the problem or make the code more readable.
Each of the two cases satisfies one of our goals. Not asking means missing an opportunity to grow or missing a flaw in the code.
3. 🙋 Speak For Yourself – Not For Others!
Feedback is always subjective. Every developer will comment on different things in the same merge request. There are, of course, many issues that most experienced developers would agree on, but it’s still important not to create the impression that your opinion is the absolute truth, the only one that matters.
There is one simple rule to prevent this from happening:
Always speak for yourself and formulate feedback from your own perspective.
Instead of absolute claims like “this is messy code” or “you can’t do it like this”, use personal and constructive sentences like “I have difficulties understanding this code” or “What do you think about changing it as follows?”
It’s much easier for us to accept and think about feedback when it’s formulated as a personal opinion because we know: Every person has the right to their own opinion, but no-one can claim the absolute truth for themself.
4. 👁 Focus on The Right Things!
Refrain from adding comments for things that are clearly personal preference. Let a linter take care of formatting if possible or else, try to not let formatting comments distract you from discovering real bugs. Here’s a short (non-exhaustive) list with suggestions on what to look for:
- Single responsibility principle / Separation of concerns:
Is a class or function taking care of too many things at once? - Naming:
Is a variable’s, function’s or class’ name expressive and precise enough that it’s easy for you to understand its purpose? - Hierarchy & data flow:
Is it difficult to see how the data flows due to a missing hierarchy? - Imports:
Does a class import a library that it shouldn’t be concerned with? (For example, a networking class normally shouldn’t know a thing about UI components.) - Reference cycles:
Could the code possibly create a reference cycle and cause a memory leak? - Edge cases:
What happens if something unexpected happens in the program flow (like an interruption of the network connection, an illegal string that the user entered etc.)? Are errors handled properly? - Code Repetition:
Is the same or strikingly similar code used in multiple places?
5. 🖍 Clearly Mark Optional Changes!
Depending on the project, decide for yourself if a comment you added is crucial to be discussed or if it wouldn’t hurt to keep the code as it is. Agree on a word or a symbol with your team to indicate optional changes of little priority. At QuickBird Studios, we prefix comments for optional changes with the word nit (short for nitpicking). You may also use an emoji like this one: 🤷♀️, or any other symbol.
Marking optional changes speeds up the process of adaptation as it delegates the responsibility for whether or not to perform the change back to the author of the code. This best practice helped our team to avoid long discussions about things with little relevance or personal preferences.
Conclusion
As we have seen, many things can go wrong with code reviews that have the potential to undermine their very purpose and render the entire process useless. But if you create the right environment in your team and adhere to the best practices introduced in this article, your reviews will hopefully become both efficient and developer-friendly. 😊
