10 tips for conducting code reviews
Code reviews are a common part of most development teams workflow these days and for good reason. Code reviews give us a process of keeping code quality high, sharing knowledge. catching mistakes and keeping a consistent code base. If you use git, you probably already heard about pull-requests, which basically is a merge request with a code review requirement.
I help teams learn to conduct productive and positive code reviews, so the process becomes an enjoyable part of their development life-cycle.
Here are my 10 tips for conducting a productive and positive code review, that gives value to both the reviewer and the author(s).
1. Figure out what the goal of the code review is
Something that often happens in teams that are new to code reviews, is that the scope of the review is not set. So anything the reviewer finds “off” gets commented. And it often ends up in nitpicking that doesn’t give any real value, and might just seem as an attack on the author.
Therefor, before ever starting to use code reviews, you should talk internally in your team about what you want out of the code review.
My recommendation is:
- Does the added feature fit into the architecture?
- Is it consistent with the rest of the implementations of the system?
- Is the architecture of the feature fitting for the short term and long term goals of the product (be pragmatic, not everything needs to be perfect)
There are as many coding styles as there are developers. But all developers in a team, should strive to have a common coding style, this makes bugs less likely to happen because of misunderstood code.
This should be an ongoing discussion in the team, that could be tackled productively in the Scrum review/retrospective meetings.
The point is, the team should have a common and consistent code style, so the code becomes easier to read and fast to reason about.
- How do we declare variables?
- Explicit vs implicit
- How do we name things like interfaces, enums, handlers, services, etc.
- Exception strategy
- Do we allow nullable or null in general
- Braces vs no braces in if statements
- Tabs vs spaces 😉
Figure out what matters to your team. And get everyone on-board. It is important that everyone is heard in this decision if you truly want everyone to embrace it.
One of the key goals of a code review, is to catch bugs that are too obscure to find with just unit testing and integration testing. You get a second pair of eyes on your code, that potentially could catch edge cases that you did not think about.
This should be a mandatory goal of the code review.
A goal I like to include in all code reviews, is whether or not there are tests to prove that the feature is working as expected.
You should figure out in your team, what kind of test coverage you expect when a feature is completed or delivered.
2. Humans vs the code – Keep code reviews objective
I often hear teams complain that they feel that the code review becomes a personal attack on their code and style, and this is definitely not the goal of a code review. It is not about finding out who is the best coder, and who can find the most issues in someone else’s code.
Code reviews should be objective, basically the author and the reviewer should be on the same side, humans vs the code.
Any issues with the code that are raised, should be allowed to be discussed, and out of that discussion should come something positive, and in most cases someone will have learned something.
This point goes out to both the author and the reviewer. The reviewer should have the goal of attacking the code and finding every little thing he disagrees with, but try to help and guide the author if it is needed. The of a code review is important, and should always be kept positive, and productive.
The author should also, not see every comment on his code as an attack, but rather as a question mark on something that could be discussed, either the reviewer could learn something about the way the author wrote something, or the reviewer might have some information that the author did not know about, and maybe could learn to solve the same problem in a simpler way.
In general, I preach that should always talk to your team members with respect, and if you keep that in mind when you are conducting a code review with someone, the tone will always be right.
3. Pick a reviewer that knows the subject
I sometimes see teams, just throw their code up in the air for anyone to catch it on slack, and I don’t think that is such a great idea in most cases.
Most often, there is someone else on the team with knowledge about the systems you have touched, or have some experience with similar features. It would be great to choose someone, who understands your task and can help review it.
The reviewer should also be briefed about what the tasks is, and what accomplishment the code is supposed to achieve.
This does now however mean that it should also be the same person reviewing your stuff, that always leads to someone (often the architect, or lead developer) doing nothing but code reviews every single day. If someone is being burdened too much with code reviews, you should start including a “spectator” that could get some more experience on the subject and offload some of the reviews to him as well.
4. Do not communicate through tools
Most source control solutions today, have some sort of code review tool, especially if they are based on Git, they will always have the pull-request as the main code review tool.
These tools are fine, and worth using, but they should NEVER be the main way communication is done between the author and the reviewer. Text has a downside of not transferring emotions, so a productive comment by the reviewer can be seen as a snarky comment by the author.
If at all possible, you should do code reviews in a pair programming fashion, face-to-face. This almost always eliminates the tension that is created between the author and reviewer, because they can properly communicate and discuss each comment. We are social beings after all.
If a face to face meeting is not possible, I highly encourage at least a Skype call, because it achieves most of the same positive vibes that a face-to-face session does.
When you conduct a code review, you walk through the changes and talk about what the changes achieve, and what thoughts went into the solution, that helps keep the focus on the goal.
This does however not mean, that you are not allowed to write all your comments before, if you think that process is fitting in your team. That way both parties come to the code-review somewhat prepared.
5. Limit the time scope
No one likes meetings, it’s as simple as that, and more so, developers hate time away from developing. So scope the code reviews to a specific time frame, so you stay on target and keep focusing on the important things.
It is okay, to say that a part of the code needs further discussion if the discussion cannot be settled right away or requires another team member or some research to figure out if the solution fits the bill. Just mark as something that needs to be discussed later, and move on to the next issues.
I advice that a code review should take less than 30 minutes, but in some bigger features with a face-to-face pair programming session, 60 minutes could be okay.
If you keep code-reviews short, it helps remove the stigma within the team that code reviews are cumbersome and in general a bad experience. If they are brief, objective, productive and positive, most developers should come out of a code review, feeling empowered.
Code reviews are, after-all, a process for sharing knowledge, and creating confidence around the codebase.
6. Reduce the feature scope
This really comes down to the discipline of the author, when it comes to how they manage feature branches and their own work in general. But too often I see a junior developer, asking someone to do a code review, and the code he needs to review is basically mixed between 5 different features, and a lot of unimportant changes.
A code review, with 50 changed files, and a few hundred lines of code changed in each file, will overwhelm anyone.
Keep the changes to a minimum, and focused on that particular feature, so that the reviewer can focus on one topic rather than having to filter out the noise of multiple feature changes or even of minor unimportant changes.
This should also, make it easier to reduce the time spent in a code review.
And as I said, this ultimately comes down to the discipline of the developer.
Please never do a “changed everything” commit, and ask someone to review that mess…
7. Build and test the code
Before a code-review should even be conducted, the could should have been built and all automated tests, both unit tests and integration tests should have run already.
Most experience DevOps teams, have Continuous Integration as part of the pull-request process so that the build server can “review” the changes before you ask another developer to do so.
This also reduces the mental overhead of assessing whether or not the code can compile, and affects any other part of the program. Again, it helps you focus on the goal of the code review, and leaves the nerdy details of interdependent modules to the already built tests.
No one, even if they say so, can remember every little detail about their entire system, therefor tests should be written when the code is created, so that we can reduce the mental load on the developers in the short and long term. It also helps create confidence in the codebase.
8. No one is too senior to be reviewed
No one, is too senior to be reviewed and no one is too junior to be a reviewer.
I believe that anyone you ever meet can teach you something, and you as well can teach them something. So even if you have one of the fabled, superstar developers, that never fails and is perfect in every way other than their social skill, he or she should also have their code reviewed.
Even if the junior developer, has nothing to add or ask to change in a code review, as the reviewer, he is able to gather knowledge and experience from the team member. It helps him learn faster, and see how his team mates think and solve problems.
It gives a really bad vibe in a team if someone is too “good” to be reviewed, by the “lesser developers”, it creates a “us and them” situation, that is toxic to any development team.
9. Consider your teammates personality traits
If you’ve ever worked in a larger corporation you have probably had a personality test, that describes how you work, and what you value in a work environment. There is no personality that is right or wrong, but we all have different personalities, and some of them work better together naturally, while some need a little deliberate change in communication.
When you work with your team mates, it can be a good idea to assess what type of personality they have, that can help you to realize why their values and focus is different than yours, and can help you to understand what they are actually meaning when they comment something in your code.
There are as many personalities, as there are people, but they often share some of the same values and ways of thinking.
I have included a matrix of the different overall personalities, in the Insights personality profile. This personality profile is heavily used in the JCI world, and makes it easy to understand what matters to your colleagues, and why they behave differently from you.
Director vs Supporter
For example, am the “director” profile, and my direct opposite is the supporter. We have wildly different values in a work environment, so I have to be mindful when working with that kind of profile. My profile is high speed, get the job done, whatever it takes. The Supporter profile is all about having a good team, everyone should be happy, and people are more important than results.
Neither of these two profiles are bad, they are just different, that means when I want to get things done, and think we should get going, I should be mindful as not coming off as cold and controlling, but show the warmth and team spirit, that motivates the supporter profile.
Observer vs Inspirer
The Observer, for example, finds it very important to gather all facts before starting something, whereas the Inspirer profile, cares a lot more about getting the idea, and launch it right away.
No profile is wrong or bad, we are just different, and that also means that if we want our points to come across the way we actually mean them, we should be thoughtful about the receiver and what their world view is.
For example, if you want the Observer to get started on something, you should give him as much detail and information as you can, where the Inspirer or Director, just need some basic information and a direction, and they will fly off before you finished your sentence, and figure it out on the way.
In the same way, an Observer would be frustrated with lack of information and just relying on faith that everything will work out, and the Director or Inspirer might be bored to death if you gave too many details.
10. Automate, automate, automate
We only have 24 hours a day, and of those only 8 hours are spent on the job, and for some it might even be less hours of work actually getting done.
Therefor, make sure to automate as much of the boring stuff that exists around the code review process.
Ideas of what can be automated:
- Analyze code for potential errors.
- Analyze the style of coding.
- Eg. Fail the linting if anyone uses implicit value types. (eg.
- The software must be able to compile, on the build server.
- Run Unit Tests and Integration tests before a code review can be started.
Do not do a machines job! It just wastes your time.
Code reviews, should be a positive, productive and great learning experience. If it is not, you need to motivate your team to make a positive change to your processes, because if done correctly code reviews give your team a lot of value.
And if you know me, I only invest time in things that pays off and gives tremendous value.
// André Kock