One of the most critical aspects of software engineering is the code review process. In college, projects are often short lived and only reviewed by the professor. Group projects are generally free for all, and no one teaches how to give proper feedback. I’d like to shed some light on code reviews, and how to do them well.
Good Code Reviews Start with Culture
As with most things, having great code reviews requires a healthy team culture. It’s important to recognize that the code reviews can be scary. You work hard to solve a problem, and then you are essentially asking your entire team to judge your work. Acceptance in our groups is a basic human need, and the fear of being rejected can create a lot of anxiety. Code reviews should always be framed as an opportunity for learning and getting the best outcomes for the product. This means when you review code you should assume the person has the best intent, that they are trying their best, and that they are human and will make honest mistakes. Especially since code reviews are public, you should never criticize the person. Focus on the code and how it can be made better. People should feel safe.
On the other side, when people give you feedback, accept it without getting defensive. If you are constantly rejecting feedback, people will stop providing it and your work will be worse off for it.
Lastly, if there are persistent problems with quality that need to be addressed, don’t do it in a public code review.
Don’t Fight Over Opinions.
Nothing is worse than a code review that goes back and forth on nits and minor rules. It’s a waste of everyone’s time. Code reviews should be focused on the functionality and technical content of the code. Consistent code is important, but rules such as if you should use tabs or spaces should be agreed upon by the team before the review. Even better, for languages that have rich linting tools, anything that is syntactic should be automated in a linting tool. People only have so much brain capacity to review code, don’t have them waste it on formatting.
Keep it Short.
Speaking of brain capacity, everyone has limited mental capacity to think critically. If you publish a 100-file code review, no human is going to be able to do a good job reviewing it. Even worse, if something goes wrong, you must revert the entire change. If there is a bug, there is no easy way to do a bisect to find the offending lines.
It’s also a huge pain to try and merge with a large code change. Nothing burns a team’s time more than when everyone must edit all their current changes because someone moved 400 files around. It is more work to make changes incrementally, but it pays off immensely because you can also test incrementally.
If a large change is unavoidable, ask multiple people to review individual parts of the change so they each have less to think about.
Run the Code.
Part of reviewing code is making sure it does what you think it does. Ensure you have a process in place that makes it easy to run the changes locally to verify the bug is fixed. This also makes it easier to try out corner cases, check performance, and verify that nothing was broken between iterations. For some code changes, the outcome may be obvious, but for more complex systems it can be helpful to have someone verify the fix and play around with it.
Everyone Should Be Doing Code Reviews.
Oftentimes I see only more senior developers doing code reviews; this is a critical mistake. Doing code reviews are the easiest way for junior developers to learn and grow. I see so many developers finally get ready to be a Senior developer, but they have no real experience doing code reviews. They are missing the skills needed to identify problems, and just as importantly, point them out without being a jerk.
The biggest concern that people have is that a junior developer might miss something. If you really need to, you can require two sign offs -one senior and one not. However, this can release the junior of the responsibility to do a thorough job on the review. Also, doing the code review doesn’t mean you need to be an expert in the area. If you don’t know the code area very well that’s okay, it’s a great change to ask questions and learn. If you need to, you can pull in another developer to clarify. The great thing about having junior developers look at code is that if it’s too complicated for them to understand, it’s a sign the code needs to be simplified or broken down.
You should also avoid having only “area experts” review code. It further pigeonholes people into silos, and it prevents knowledge sharing. If knowledge is siloed in your teams, it’s very likely if someone leaves (or even goes on vacation) you will be in a bind.
Ask Questions
So how do you critique code without being a jerk? Ask questions. One of the most important ways to have hard conversations is to have an open mind yourself. Don’t come off as infallible, it will just make the other person dig in. Instead of saying “this is wrong do it my way”, try “Could you explain your thinking here? I might have taken this approach instead”. Since we’re all human, we can all be wrong. The most powerful part of this method is that in asking questions. You help lead the person to the answer which helps them learn. This is also powerful for junior devs.
If you are reviewing code and you don’t know what it does, ask! This seems so silly but often people will just assume the person who wrote the code knows what they are doing. Even if you aren’t responsible for signing off on a code review, look over the review anyways. Worst case you will learn something. And if you ask questions, you might come across a place where the person doesn’t quite know why they did something (or maybe didn’t have a good reason). This can make them think critically about what they wrote.
Make Time
Code reviews can take up a lot of time so it can be distracting when they come in frequently. You often have enough work to do on your own bugs or features, but it’s critical for the product and team that everyone makes time. The technique I’ve best found works for most people is to carve out time when they first get to work to go over any outstanding code reviews. It can be a good idea to block this time on your calendar to make sure it doesn’t get scheduled over. Trying to do it later in the day can increase the odds that someone will schedule over that time, or that you’ll get distracted by some other task and forget to do it.
Make sure you don’t overly rely on one person doing all the reviews. You can also create scripts to randomly assign out reviewers.
Test Code is Code Too
I often find that people won’t review tests, or that they have a separate test reviewer. It is important to remember that test code is code just like any other. It needs to be maintained over time, well understood, and it can also be wrong. I’ve seen code where the tests weren’t verifying what the writer thought. Sometimes they forgot to assert the final condition at the end. Take tests seriously, they are a safeguard of your products’ health.
Over the Shoulder Reviews
Some people like to do what are called “over the shoulder reviews”. In this case the person writing the code literally hovers over reviewers’ shoulders and explains the change. I am not a fan of these for a few reasons.
The main reason is that if you need to explain the code, it needs to be simplified (or at least documented). You won’t be able to explain a change to every person who reads this code in the future. Ideally someone should be able to pick up and understand whatever change you are making.
The second big reason is that I find people tend to explain away and put pressure on the reviewer to accept change they might otherwise feel comfortable commenting on without the reviewer standing right there. It’s also just hard to think and parse a review with someone breathing down your neck.
Don’t Design During Code Reviews
One mistake I often see is that people will write code and then ask for design feedback on the implementation after it’s already been coded. Design should be done before the code is posted. It is an incredible waste of time for everyone if you put up a code review and the approach is entirely off base and needs to be redone. Take the time before you code to verify the design with your team. You’ll save yourself and your team a lot of time and frustration.
This doesn’t have to be an entire design meeting or document (though for complex features this is certainly a good idea). It can just be a quick conversation to make sure you have the right general idea. Some developers will use the excuse that they “think better in code”. While it can be easier to figure something out by writing the code, the danger is that as humans once we put in some work to a change, we will grow attached to it. You may be tempted to just go with whatever approach you have because it’s already done, feeling that it’s too much work to change it.
Keeping these things in mind will help you get off to a great start. You should integrate what’s important to your team into your code review process. Someone making a casual game will have different requirements than someone making hospital ventilators or rockets. A more senior team might also have different requirements than a large team with a lot of junior developers. Regardless of your team makeup though, the code review process is a sensitive one, so make sure you are kind to your fellow team members and show them respect and that you have their back.
