Contents

We finally come to the core post of this series – how to do code reviews.

This post is part of a series of posts about code reviews:

Timing

As I stated in the first post of this series, time is money. Finding the right timing, therefore, is as important for code reviews as for anything else.

Don’t wait

The reviewee is waiting for your feedback and may have to build some next steps on top of what just was submitted to review. In addition, if there are any findings or even discussions to be had, it’s best if the code in question has been written recently. You don’t have to drop everything when a pull request comes in, but don’t put it off until tomorrow either.

That also means that you should give feedback if you don’t have time to do code reviews right now. Consider coordinating who reviews what right after the daily standup. And, of course, if it happens too often that people don’t have time to do code reviews, figure out why that is the case. Are you overplanned or are code reviews simply deemed unimportant?

Timebox code reviews

Don’t take too much time for the code review. If the code is prepared well for the review, trust your colleagues. That means you don’t need to compile and run tests – they already did that, and ideally, your CI confirmed that everything is well. This also means that we might not want to dive too deep into complicated algorithms. If the test coverage is good, we can assume that the algorithm does the right thing and we don’t need to understand every single detail of how it works. (THe code should still be readable though)

On the other hand, take enough time. For example, don’t assume that the code is clean and correct and every corner is tested simply because it has been written by your local guru. They’re just human, they commit errors, and they can use the feedback, even from juniors. In addition, you still have the opportunity to learn a thing or two while reviewing, so taking the necessary time is a good investment.

The review process

The following process is just an example of how it can be done. As usual, inspect and adapt: Pick the parts that seem useful to your team, modify or leave out the others.

Review alone

When we check code for accessibility and understandability we can do so best if we don’t have prior knowledge or someone whispering explanations in our ears. For that reason, we should do code reviews alone in the beginning. We can ask the reviewee for explanations later if needed. Taking notes of points that need additional information also gives us a list of things where additional documentation, code comments, or better naming might be needed. This also saves the reviewee some time if there is little or nothing to explain because they don’t have to watch us scroll through their code silently.

Just looking, no touching!

The code is not ours yet, so any quick fixes should be done by the reviewee. At the very least, they’ll have the opportunity to learn from their mistakes. Sometimes, what looks like an error has been done on purpose and instead of our fix, a clarifying comment is what is actually needed.

If we don’t touch the code and since we don’t have to compile it and run the tests, we don’t need to check it out at all. Your version control system may have all the tools you need to view the changes and annotate them with your findings as needed.

Black box code reviews

When we look at a new feature there can be a considerable amount of new code that we need to review and understand. It is a good practice to start from the outside instead of deep-diving into the implementation of the feature right away. Treat the new feature as a black box and peel away the layers one by one as follows:

After knowing what the new feature is about, e.g. by reading the requirements document or ticket in the issue tracker, start with the names of the automated tests for the feature. They should reflect the use cases that have to be covered.

Then, have a look at the test implementation. It will show you an outside view of the new functionality, its API, and how it is used. We now should know whether the class design feels right, whether the function and class names of the API are fitting etc. We also see whether the test implementations are understandable and do what the test names suggest. The need for clean and maintainable code does not stop at test code. It should be even more readable, if possible since it is a natural documentation of the production code.

Now have a look at the actual API, i.e. the header files of the new functionality. Does it look like what you expected from its use in the test cases? Are there untested public functions? The last step is to review the implementation itself.

Findings

During code reviews, there will be things that can be improved or have to be fixed. Among those things are obvious bugs in the code, missing tests, algorithms with a bad complexity, code smells, and also unclear code that needs to be documented with a clarifying comment if it can’t be made clear with better names or a different structure.

There are things that should not need to be discussed in code reviews, like formatting, naming conventions and other style issues. As I had written in the “What?” part of this blog series, those should be covered by automated tools.

In any case, code reviews are not the place for lengthy discussions. If you encounter things that need to be discussed in more detail, it is usually best to not discuss it between the reviewer and the reviewee, but with the whole team.

Take notes

Remember, time is money, and it is probably not a good idea to interrupt the reviewee every five minutes with a “quick question” about a single line of code they wrote. They wouldn’t get done much during the whole review. Instead, take notes of anything you find that needs fixing or clarification. If the list gets very long, prioritize it and deal with the most important things first. And don’t attach those notes to the pull request just yet.

Verbal communication

The stereotype of the socially awkward developer finds it easier to just dump their findings as comments on the pull request. However, written communication is flawed, and a back-and-forth of asynchronous comments takes more time than it should. Instead, sit down with the reviewee and your list of findings either in person or via a Skype call or similar and go through it to work out a to-do list. That list is the one that can (and often should) be added to the pull request.

Accept or reject?

In the discussion with the reviewee, come to a consensus whether the pull request should be accepted or rejected. Obvious bugs, missing test cases and anything else that needs more than a few keystrokes usually should be rejected, as they merit another review for the additional code that has to be written.

Other than that, depending on how formal your code review process is, you might agree that they fix smaller findings and merge without anyone having to review the code again. Sometimes, e.g. if they are doing more work on the code in question on another branch, you might even just merge and tell them to fix the two typos you found in a later commit.

Conclusion

This post is a list of “best practices” on how to do code reviews. As always, not all of it necessarily applies to your specific situation. Inspect and adapt what is best for your team.

The next and last post of this series will be about the human aspect in code reviews.

All emojis by emojione.com

Write clean and maintainable C++

Code Reviews – How? (Part 4)