How to Be a Better Code Reviewee

In the only guest post ever on Tidy Java, Agnieszka gave you the essentials necessary to take advantage of code reviews in your projects. I hope that by now all of you are doing some sort of code reviews. Actually, I hope that you were doing them before Agnieszka’s post and her writing was just a reminder of the key ideas. In this article, we’re going to look into code reviews from a different angle – how to become a better code reviewee. Let’s begin.

Write Good Code

It’s so obvious and yet it cannot be stressed enough. We’re doing all this stuff because we know how important code quality is to the eventual success of the project. So in the end, the code after the code review should be of good quality. To make your and your reviewers’ lives easier, make sure the quality is good before the code gets submitted for code review. You will save everyone’s time, your priceless pride, and company’s money.

Submit Production Code and Tests Together

You might not be a fan of TDD or refuse to use it for any other reason, I get it. You can write your tests after the code has been written, no problem. But even if you do so, write those tests before you commit the code and submit it for review. There are some good reasons for this:

• You won’t submit broken code – unless you write the tests in a way that they’re always green.
• You won’t submit untestable code – unless you’re using some heavy magic that allows testing the untestable.
• Understanding of your code will be easier – people will be able to read its supposed behavior from the tests.

Make Your Code Consistent

I have recently written a post about consistency. You can check it out here. The most important things about consistency in terms of code reviews are:

• You will have to do it anyway – if you’re serious about code quality, either you will make the code consistent or the reviewers will force you to.
• The code will look more familiar to the reviewers – they won’t be distracted by meaningless details.

With the capabilities of the human brain and IDEs, this should be an easy task. Just stick to the way things are done in the project and configure code formatting. And if you happen to change the way things are done in the project, remember to change it in all relevant places, not just the one that inspired you to do so.

Solve Problems Before Committing

There will be times when you’ll write some code and you won’t like it. It won’t be clean enough, you’ll see some code smells, or it just won’t feel right. Don’t wait for it to be spotted in the review, there’s no point. Take necessary time and people, and solve all the problems before the code review starts. The reviewers will then be able to focus on things that you didn’t think about, not on things that you were too lazy to do right.

Use Descriptive Commit Messages

Too often people care only about the message of the first commit and then, every subsequent one is cryptic and non-informative. It’s common to see a commit list like this:

1. Add user login feature
2. Code review
3. Code review fixes
4. Quick fix
5. Rename variable
6. Review fixes etc.

I think there’s no need to explain that this is far from optimal. Generally, to make things better, you have two options:

1. Group similar code review changes, and use a descriptive message for each group.
2. Squash the commits when the review is finished and the branch is ready to be merged.

Limit the Code to Review

Have you ever tried to review a pull request that consisted of over 50 classes? What did it feel like? In case you haven’t, I can tell you that it sucks. You’re overwhelmed, it takes hours, and, usually, you don’t even bother to go deeply into what has been implemented and how. You just look at it briefly and hope that some other person will give that monster more love. Such bulks of code are also totally against the spirit of continuous integration. How are others supposed to integrate with your code if you keep it for yourself until the last possible moment?

Don’t Take It Personally

I’ve written about it already, although a bit indirectly. You are not your code. The fact that your code can be improved does not mean that you failed or anything like this. It just means that the code can be better. It’s a good thing. As much as you should feel ashamed when people point out your laziness on code reviews, you should be happy when they find ways to do things better. Even if you beat the world record in pull request comments if you did your best and people know that they won’t blame you. Maybe the problem was just that hard, and nearly impossible to right the first time.

Don’t Expect Full-Blown Solutions in Comments

If I were to write a guide for reviewers, I’d advise them to be as specific as they can be – say exactly what’s wrong, provide suggestions on how to make it better and even provide code samples when it feels sensible. At the same time, when you’re a reviewee, you shouldn’t demand any of these. Sometimes people see a piece of code and are sure it can be done better, but they can’t tell you how exactly. When you recognize such a situation, take it on yourself and spend some time trying to figure things out on your own. Don’t go for stances like: tell me exactly what to do or I won’t do anything about it. Usually, a few minutes to an hour of playing with the code and you’ll find exactly what your colleague’s gut was trying to tell him.

Don’t Be Submissive

Some people tend to get really defensive about their code when it comes to code review. Usually, these are the ones that take comments personally and I already suggested that it’s a bad thing. On the other extreme, we have people that reply OK to every single comment and do everything their peers ask them to – everything to satisfy them and be able to finally merge the code! Hey, accepting bad solutions just because somebody suggested that is as bad as defending bad solutions just because you wrote the code. Therefore, stand by your solutions and provide resources and arguments that back up your view when you feel like the reviewer is not right.

Tackle Difficult Problems Offline

When a conflict situation occurs or it’s not obvious which way to go, don’t go into elaborate comment discussions. Taking things offline and discussing them in person, usually, leads to achieving a consensus much faster and with far less frustration. You will also have the advantage of much better tooling – ultimately nothing’s as good as your hands and a whiteboard.