Three steps for better code reviews
Code reviews can incredibly useful as learning and quality improving tools. They also can be tedious and completely useless for everyone if the process is focused in the wrong areas. The following three steps can help you make sure your team’s code reviews are actually productive and useful for everyone:
1. Style guides (standardize, standardize, standardize)
A pull request should never have comments about tabs vs spaces, or the folder structure used or why a class is defined like this instead of like that. All of those things should have been defined through a style guide before anyone even opened their code editor.
Everything that works the same should also look the same, leaving the reviewer free to think about how things work instead of how they were written. Creativity should be devoted to the workings of the code instead of the formatting.
Of course there’s no real need to write a style guide from zero for each project, being able to point to an external one is enough, as long as everyone agrees about its contents.
The Angular 2 style guide is one of the best examples I can think of: it has guidelines with different levels of importance, the reasoning behind each guideline and examples for each one of them. It’s extremely clear and covers most of the necessary boilerplate.
Other style guides to check:
2. Linting (Protecting yourself against yourself)
Automatically check your code against your style guide. Check for spelling errors. Check for unicode weird spaces. Check everything that can be checked every time you save. Use linters in your code editor, after saving or through git’s hooks before committing. By the time the code is in a pull request every trivial error should have been fixed through those checks (and even then a final lint should be made by a bot).
I’d have thought this was an uncontroversial thing but some weeks ago someone told me that linting was bad because it breds bad habits and that developers should write correct code from the beginning.
While it is true some errors are caused by lack of effort, most of the errors corrected by linting can be non-intentional. Linting removes the need for the code reviewer to be a human spell checker. A code review focused on spelling or style errors is probably a waste of time for everyone.
Style guides and linting solve the easy stuff, leaving all of the developer’s expensive time free to be used in solving the hard problems, the logic of whatever is being programmed. Testing helps you make sure that the logic actually works.
Having a good test coverage leaves the code reviewer free to check and comment on the how the code works instead of having to worry whether it works at all.
Sadly, it’s perfectly possible to unit test every single function in your pull request and end up with something that doesn’t work simply because the real use case wasn’t tested. Integration tests can help with this but good communication is probably the only sure way to make sure that everyone is working towards the same goal.
Implementing the previous steps team-wide can help you make sure that every second the code reviewer spends checking a pull request is used towards finding ways to improve the code instead of having to make sure it works or having to waste time trying to understand it.
There’s no need to implement all of them, even one of them can help you improve the quality of your code base and your team’s quality of life.