Now that we know good reasons to do code reviews, which parts of our code need to be reviewed? What does not need review?

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

1. Code Reviews – Why?
2. Code Reviews – What?
3. Code Reviews – Preparation
4. Code Reviews – How?
5. Code Reviews – The Human Aspect

Disclaimer: everything that is written here may or may not apply to your team. Try things out, reflect and adapt. Every team has a different culture and different code base to work on. Think about what may and what may not be good for your specific situation.

Things to review

Features

New features are a pretty obvious candidate for code reviews. There’s new code, new names, and new business logic to understand. They touch every aspect I had mentioned in part 1 about the reasons we do code reviews.

But also the implementation of older features needs a review from time to time. Requirements change, and the old implementation may need some work. Does the old implementation still fit into the architecture? Do new features affect how the old feature works or how it is integrated into the application? From time to time there will indeed be old code to be reviewed, even if it has not changed.

Bug fixes

Bug fixes can be fairly simple. Does a single character fixing an off-by-one error need a review? It might seem silly, but there can be reasons to actually do it. Send those tiny fixes to code review to raise awareness towards that kind of errors. It may even trigger a discussion in the team how to avoid them altogether. The obvious, simple fix may not be the right thing to do, either because a more verbose solution would make it more readable, or because it’s the wrong fix regarding the business logic.

And, of course, a bug that automated tests did not find before, usually needs an automated regression test, so there’s more than that silly fix for our code review. In the end, if it actually is a simple fix that needs no further comment, reviewing it is a matter of seconds and doesn’t hurt at all.

tests

Many developers regard test code as second class citizens and therefore do not review test code. But test code is just normal code that can have bugs and can be unreadable and unmaintainable. This alone merits the same care as production code, including code reviews.

In addition, reviewing tests can reveal missing edge cases. If those edge cases are not tested, we can not be sure whether the code under test handles them correctly. Tests also show a good summary of how the code under test is supposed to be used. Therefore, when we review the tests, we also review the code under test, e.g. its usability and its interface.

Refactoring

Although there is no new functionality, refactored code is a good candidate for code reviews. After all, we refactor to make the code more readable and maintainable, and those qualities can best be assessed by another human. As is the case with new code, there may be new names and a new structure and design of classes and their interactions.

However, smaller refactorings that are done by smart refactoring tools may be of less interest for code reviews. Discuss with the team whether you would want to review every single refactoring or if smaller changes can be committed without going through code review. On the other hand, as with the one-line bug fixes, reviewing those small changes only takes very short time, so it probably won’t hurt to review even the smallest refactorings if the reviews are done soon enough.

The thing you will not want to review are changes to indentation, braces and the like. First of all, there should be a consensus about brace style and indentation in the project style guide. Modern IDEs and tools like clang-format can be used to check and maintain that kind of thing. There generally is no need to review things an automated tool checks for us, so checking these things in our build pipeline is a good thing. Secondly, there is hardly anything more boring than staring at whitespace.

Documentation

While, strictly speaking, documentation is not part of our code, it is part of the project. Since documentation should be at least as readable and understandable as the code, it is also a candidate for reviews. Check in the documentation as plain text format, so it’s easy to diff between revisions, not only for code reviews. I’ve written more about clean documentation in the past.

Configuration and infrastructure

While it may not be obvious, configuration files are code, too, and should be included in our code reviews. That way, we may find bugs like redundant or contradictory configuration options or missing test cases.

Infrastructure files like build scripts and CI server configurations can be reviewed as well. Especially when we are not DevOps experts, we can learn by reading those files and over time get enough knowledge to make minor changes ourselves.

Conclusion

There’s a lot of things that can be reviewed. It’s important to sit down with the team and discuss which changes need reviews and which don’t. If you’re not sure, a good starting point can be to review everything at first and then determine which parts can be checked in without review over time.

The next post will be about preparing for the review process itself, so stay tuned!

All emojis by emojione.com