What are code reviews?
Code review is a very common and important process throughout development phase where code developed by individuals gets peer reviews in order to improve the overall quality of the source code.
Is there a problem with this practice?
Not per se, I’ve seen it poorly implemented too many times. Close your eyes and imagine you are sitting in a kickoff meeting and you hear something along this line. Usually from a very eager, but inexperienced team leader.
“I would like that every line of code to be reviewed by the entire team. Everybody makes mistakes, even senior developers do, and even junior developers can have valuable feedback, so everybody should participate in reviewing.”
Don’t get me wrong, I like this way of thinking. It comes from a very noble place. Unfortunately when put in practice with a mid-size team, you will see some shortcomings surface.
How code reviews can derail
Let’s do a simple calculation. There are 8 team members, each asks for one review per day for his full day work. Every team member reviews the code which takes about 15 minutes per review per person. Even if the math is a bit off we can see that this would take each team member about 2 hours of reviewing per day, and let’s not consider the context switching cost.
In my experience, if everybody is doing reviews, nobody is really doing reviews. People tend to wait on each other, things can drag out.
Of course everybody can have valuable feedback. Context and project knowledge most of the time is more valuable that overall years of experience. Especially when you take into account the technical inclination of individuals. You might have the “UI Ninja” or the “DB Expert” in your team. Why not use them?
I go with Vegas rules on this one. I’m trying to maximize my value from a 15 minute bet on a colleague. Should I chose the new guy who has 10% chance to help me out, or should I ask the domain expert who has 70%? If you don’t understand these odds, don’t step into a casino.
As a technical/team leader you need to ask yourself the following question. Do you thrust these people to carry out the work? If not, what are they doing in your team? My point is, you should have faith in your team. Even if they will make mistakes, they should feel empowered around you.
Alternative
I adopted a very lean view on it. “I expect of you to have a professional attitude. You decide when you need a code review and when you don’t. You should also be able to decide who can give you valuable feedback. Select two to three reviewers. Please don’t forget, if you have doubts that you’re doing the right thing, I’m here for you.”
Keep in mind that this process will not save you from bugs. It’s just a second safety net that reduces the chance that something will pass through. It’s a lot better to have a simple process that developers enjoy, instead of a strict one that they will game eventually.
Before we go on with our day, I would like to leave you with a few “my best practices” alongside those that you already know.
For the developer
- When asking for a review, indicate what the root of the change is so it is reviewed first. The rest is probably ripples of the change you made and propagated through the rest of the code.
- Select reviewers who have prior knowledge in the are you made the changes, or know the requirements for your task very well
For the reviewer
- Don’t care about code styling. I know this might be controversial, but most of the people I talk to don’t like when they receive feedback about where the curly brackets are and if the file is well indented. Don’t get me wrong, I want to have readable code but I think we should aim for more substance in our reviews.
- Give alternative solutions. I find it really annoying when people comment in a review that “I don’t like this solution”, “there should be another way”. Be a little proactive and tell your solution so the person who requested the review at least can understand your thought process.
- Prefer face to face communication. When there are a lot to discuss, don’t rely on text to convey your thoughts. I like when developers sit together (or have a call) and try to work out a better solutions.
- Think of possible user scenarios that are not covered and should be.
- Look for high level issues like, object lifetime issues, threading problems.
I wish you a productive day!