Are you doing code reviews with your current team? Do you feel they help a lot or are they just a waste of time? In part 1 of this blog post series, we explore the reasons why we should (or should not) do code reviews.
This post is part of a series of posts about code reviews:
- Code Reviews – Why?
- Code Reviews – What?
- Code Reviews – Preparation
- Code Reviews – How?
- Code Reviews – The Human Aspect
At this year’s MeetingC++ conference in Berlin, I had the honor to talk about code reviews. The slides do not contain too much text – I experimented a bit with single-emoji slides. Therefore, I’ll replicate the presentation in this blog post.
When asked, why people do code reviews, some of the more frequent and less inspiring answers are “because everyone does it”, “because that guru says so” or “because it’s agile”. We shouldn’t do things only because “everyone does it” – at least that’s what my parents taught me when I was little. Gurus won’t only tell us to do something – they usually tell us reasons. And, of course, doing something blindly because it’s supposed to be agile is not very – uhm – agile.
Reflect and adapt are the keywords: See if the reasons to do code reviews a guru or book gives apply to our team. Keep what works well, improve or remove what doesn’t. Of course, sometimes we have to do code reviews. An example is regulated development, e.g. for medical devices. In those cases, it still is useful to know what makes the code review process necessary and what could improve it.
Time is money, at least when we develop software professionally – and code reviews cost time. If we do them the wrong way, they will provide no value that would be worth investing the time. If we do them ineffectively, there may be some value, but it still may not be worth our time.
To evaluate the usefulness of our code reviews, we have to determine for ourselves, why we do them, i.e. what our goals are. Here are some better reasons to do code reviews, and some that aren’t as good reasons as we might think.
QA is the main reason why we do code reviews. As Steve McConnell writes in the classic “Code Complete”, it is one of the earliest quality gates we have for our code. And the earlier we catch possible problems in our code, the cheaper it is to fix them.
Quality is a very broad concept though when it comes to code, so let’s have a look at the different ways code reviews can help to improve the quality of our software.
In the 60s, there have been studies about the effects of formal “code inspections” for larger code changes and short, less formal reviews for smaller changes. They have shown that both can reduce bugs in the ballpark of 20-30%.
Today, we also have automated tests, and those also have been shown to reduce defects in our code vastly. The two effects don’t add up unconditionally since many of the bugs that might have been found back in the days by manual inspection are found by our unit test suites today.
That does not mean that code reviews can be completely replaced by automated tests. Unit tests and acceptance tests can only assert that the cases we thought of are implemented correctly. A code reviewer can tell us about the tests we have forgotten to implement. Sometimes it also happens that the same flawed thinking that leads to errors in our program leads to similar errors in our tests, so the tests expect the wrong outcome our program produces.
Code reviews can also tell us, whether the code is as efficient as it can be, i.e. whether we have chosen the right algorithm. Don’t, however, fall into the trap of trying to reason about performance in a code review. We can’t possibly reason about what the optimizer does with our code, and the only thing that can really help us in that area is a profiler.
Code quality is one of the points where code reviews ave real benefits. It is very hard or impossible for tools to determine whether a given piece of code is readable and maintainable for humans.
It is also something that can better be judged by someone who does not know the code than by the person who has written it. That’s because as the writer, we have the insight knowledge and it’s all very clear to us what the variable names mean, how the code is structured etc. The view of someone who has to maintain the code they have never seen before can only be given by someone who really has not seen it before.
Doing code reviews is also a great opportunity to learn, both about the business domain and technical stuff like design patterns and the programming language.
When we look at a piece of code without the pressure to fix a bug ASAP or implement some feature, but with the sole purpose to take a really good look at that code, we see things we didn’t know before and can talk to the reviewee about them.
The learning goes both ways. If someone reviews my code, it’s an opportunity for them to show me where I could have used different techniques or another language feature.
Even if there is nothing new for us to learn from the code we review, it is a good opportunity to just get familiar with it. That way, we already have seen it once when we engage it in the battle against bugs.
That also means that it’s not a single developer’s responsibility anymore. Having a second person who has read (and understood) the code increases the bus factor of the project. If we see it that way, code reviews are the point in time where code really becomes team property.
There are many good reasons to do code reviews, including, but not limited to QA.
In the next posts I will go into the “what” and “how”, as the original talk title promises.
All emojis by emojione.com
Hey, nice post as always!
A few months ago we started migrating from svn to git and with git came gitlab.
The migration was exhausting but the resulting options (distributed code reviews / CI / CD) are absolutely worth it.
As our whole (development- & support-) team keeps growing, organizing workflows became more and more important to keep up efficiency, which itself reduced efficiency for those, who had to organized it as they had less time developing.
At the moment we’re still missing a testframework for automated testing – this will be the next step – but the code is at least compiled and we’re able to do code reviews, which already improved overall code quality by orders of magnitude.
I think the learning-aspect should be emphasized more!
In those few months I was able to teach something to nearly every developer and I also learned a few new things regardless of the bug/feature that was worked on.
This is invaluable!
I also helps guiding new team members or motivating those, for which coding is more work instead of passion/fun.
We require code reviews on a per-merge request-basis. However we’re currently struggling with how much code review is required for a merge request to be “safe”.
At the moment we – kind of – leave it to senior developers to decide wether their change should be reviewed or not, so we have a shortcut for fast fixes or irrelevant changes (correcting typos / removing unused variables and the like). Other parts of the pipeline (compilation and soon tests) are still executed.
For everything else we require (any) two developers to give a “thumbs up” if they don’t find any errors. More “thumbs up”s -> more safety.
We imagined experimenting with a dedicated code reviewer but discarded that pretty fast.
I’d like to get some ideas/insight how other teams with different sizes handle code reviews in a way that not only the codebase but also (all) the developers profit from it.
Hi Tomas, thanks for the comment! I’ll touch sizes of code to review and other topics you mentioned in the next posts, so stay tuned!