TL; DR: You probably don’t need to be doing code reviews. Have confidence in the code you write, and understand it well.
There are three reasons why you should not do code reviews.
1. Code Reviews are usually not needed.
2. Code Reviews build false confidence.
3. Code Reviews waste time.
————————-————————-————————-————————-
Let me break it down.
1. Code Reviews are usually not needed.
For the past 12 years, I have made roughly 800-900 commits per year. Given all those changes, I get code reviews done about a handful of times per year; I’ll mention exactly when in a bit, but let’s say it is roughly 8-9 times a year, although I know the actual number is probably lower than this. It puts my review-to-commit rate at about 1%.
Historically, the code I’ve written has not caused many regressions in the existing software. That means, for a given code change I’m not very likely to get a bug report about something else not working due to that change. Of course, I’m human; I make mistakes; I create unintentional bugs. Software can get complex and it is impossible to consider every avenue on how a particular piece of code can get executed. It is important to realize that this is okay, but only if you can manage the incoming bug rate and quickly fix newly reported problems.
The main point is that I can write good code, but I’m not the only one! Most people tend to write good code because they understand the code they write. If you don’t understand the code you are writing, then you are doing something wrong. Take the time to learn what it does, or ask someone about the code so you understand it. I frequently have been requested to review code simply because the coder did not understand why their change worked!
2. Code Reviews build false confidence.
They do not catch all mistakes. I know a lot of people who swear by code reviews and require them in their code base. In theory, this means they shouldn’t be introducing any bugs, since, heck, they had someone else review all the code before they committed it. In practice, another human looking at the same piece of code doesn’t guarantee they will consider all possibilities of how it is executed. In fact, they don’t even guarantee that the code will even build!
This false confidence leads people to write sloppy code and hope that the person doing the code review will catch any mistakes. This should not be done; as a software engineer, you should look over your code incredibly carefully before committing it, and be confident with it. You should catch your own mistakes, since you are the one that knows your code the best. I’ll discuss how to successfully do this later on.
3. Code Reviews waste time.
This is really the main reason to not do code reviews. It takes another engineers time to review code. That takes their time away from doing some actual coding, debugging, or considerations on their own code.
If you understand your code well, and know when your change is right, then there is no reason to get a change reviewed. Having someone else read the code and give a “thumbs up” will just waste their time. In fact, if you see that 95% of your changes are always getting a “thumbs up”, then this is a clear indication that the code reviews are pointless and wasting someone else’s time. Consequently, if you give another person a “thumbs up” 95% of the time, then you probably don’t need to review their code.
In fact, I have seen many code review requests for small code changes that are obviously correct. Don’t waste someone’s time by doing this; if you think it is right, then it probably is. Sometimes company policy dictates this has to be done, and it is a shame.
Pull Requests are a code review time suck because they can draw in a group. Some people will prepare a change and send out a pull request to five people. As soon as one person hits “accept” they will merge the code — since, at this point, they got it reviewed (this assumes you don’t require all reviewers to accept a request). The other four people have essentially become a lame duck; the code is already merged to master. The other four people will tend to do a sloppier code review, or simply not look at it all. The take away: when you must do a code review, you should only request a review by an essential one or two people at most, and only merge it when everyone has accepted it. Otherwise, you are guaranteeing to be wasting someone’s time.
Some people send out pull requests just to make someone else aware of a change. They aren’t really looking for feedback, and this type of code review is totally a waste of time. If you care a lot about some piece of code or some code that you “own”, then you should be watching all incoming changes that other people make to that code. It should be okay to have other people change your code. If you see something questionable, then talk to the person about it, and come up with a better solution. More often than not, this process works great, and saves a ton of time from having to do an “official code review” via a change request.
The other time waste are the code nit pickers. These are people who suggest small minute changes to a pull request when they just aren’t necessary for the logic and overall concept. Something like a coding style change would fall into this category. If someone suggests that you should change some particular style then one of two things went wrong: you, as a developer, didn’t follow the style guide that your source code is written in, or a style guide just doesn’t exist. If more than one person is working on code, then there needs to be a style guide. If there isn’t a style guide, then you should just follow the style that the code is currently written in. If some code is questionable, then you all need to create a style guide and everyone needs to abide by it. Let me just say this: it is such a PIA when I see code that mixes spaces and tabs, and it looks sloppy when a public header mixes different personal coding styles.
Finally, understand that code builds on code, and managing every change with a code reivew bogs down progress. Frequently I’ll make one change, push it to the repository, and start making more changes that build on top of that last change. If you have to wait for a code review, then the process slows down your progress. In a typical git workflow, you will have to branch from master, apply your diff, get it reviewed, and then merge it back to master. Depending on another person to review code takes time; someone may not be available and this will stall your work. To make progress, you’ll have to branch from your branch, and build from there. This quickly becomes unmanageable if you make a lot of small changes to build towards your progress. In fact, any coding workflow that involves branching for every single change is a terrible way to do development; it is too much process, and slows things down.
The next article will be “When to do code reviews”. They are sometimes required, and very important.
[…] Part 1 discussed why you should not be done code reviews. […]