Code Review. 7 Mistakes You May Be Making
Code review can really change the quality of your code. There’s one condition - you must do it properly to fulfill its potential. Let’s list the most common mistakes we make while reviewing the code.
1. Focusing on style and syntax
I saw this type of review many times: lots of focus on how something was written and no comments on more significant issues. This mistake is made so often because these errors are really easy to pick out. Every developer’s eye is used to correcting additional spaces, unnecessary lines, etc. We do it automatically.
How it should work:
If you do something automatically, a machine will do it even better. It’s best to establish one coding style guide and use certain tools to automatically improve your code. All developers in your company should use it, no matter if they operate on Emacs, VIM, Visual Studio or intelliJ. Tools for static code analysis can spot a lot of irregularities and they do it better than any human. Defining the coding style guide and code analysis should be the prerequisite for a good code review.
A review ought to start with syntax simplifications not detectable by automatic tools. It’s easy to focus on minor mistakes and typos. But the main goal is to understand the big picture and improve it. So if you don’t understand, don’t click “approve,” try to figure it out!
2. Skipping tests
You see lots of tests in your review, but you just take a quick look at them and rush to implementation. Trust me, I’ve been there :) Reviewing the tests is boring. Setup, assert, teardown over and over again. This repeatability quickly puts us off guard and we easily assume that if the first few tests are OK, the rest must be OK too.
Unfortunately, they almost never are. As a result, some tests slip through code review even though they shouldn’t; e.g. tests that aren’t passing once a month (usually on 30th or 31st), tests dependable on the previous one or tests checking trivial things.
How it should work:
Tests are also code and they deserve a proper review. It’s an excellent opportunity to check if your code is properly tested. It takes a great deal of focus and understanding of every change - that’s why it’s not easy. Everyone in the company should pay attention to this, because without the support from every team member it will be just tilting at windmills. Think again what do you want to test and how to do it, decide what is crucial for your app’s operation. And make a pact to respect the rules.
3. Only looking at the just-added code
Do you pay more attention to those green lines rather than the red ones? Yes, most of the time added code is more important, but an old line deleted accidentally can be a real troublemaker if you don’t have a well tested system. What if the deleted method was still needed? What if you deleted the last call?
How it should work:
During all the changes, the concept of the previous implementation will be visible in deleted lines. This way you can compare old and new lines and establish where any differences came from, if that’s not obvious already.
Additionally, if a method was removed in the changeset, code review is the time to see if all the calls were deleted too. On the other hand, it’s worth checking whether deleting a call was the last use of a given method in the code. Maybe you can remove it and clear the codebase.
4. Bad timing
Half an hour before the demo is the worst time for a code review. Often it goes like this: along with a review request you get a message from the author “I just need you to okay it, we’re deploying a demo in 15 minutes.” A proper review takes time and peace, a rushed one can’t ensure quality. Such situations often come from bad management so programmers don’t feel responsible. No matter who’s guilty, some poor code was merged to the wrong branch and will probably stay there.
How it should work:
It’s a good habit to close your tasks long before a presentation, release or any other deadline. Nothing else will give you enough time for a review and, more importantly, for necessary changes.
Sometimes there’s a big setback and presenting a new killer-feature is a matter of life and death. In that case, try preparing a build from a different branch than the one where all the reviewed code goes instead of rushing the review. You need to think about this possibility beforehand and prepare building and deployment processes accordingly.
5. Not going into design
Another mistake we make by not taking our role as reviewers seriously. It’s easier to tell that some code will work properly than judge if its design is good. To do it you need to carefully think about the code’s role in the system and how it fits with what’s already there. Tracking compatibility with good practises e.g. OOP takes lots of focus.
How it should work:
On one hand, you should check if new components or changes meet high standards and reflect good practises. Are they well designed? Next step is going to the higher level - the impact on the architecture. Most of the changes won’t influence it, but the ones that will are crucial and need careful consideration. It’s best to consult them with other team members.
6. Too big changesets
PR titled “Major system refactoring” and statistics like “60 files changed, 1740 insertions(+), 1202 deletions(-)” is a very bad idea. Even if you can see yourself going through all the changes, it will be veeery hard to put them in a broader context e.g. business processes or application. The quality of such a review is pretty low. With this many changes you can focus on syntax at most.
How it should work:
It’s very important to split your work to smaller pieces. First of all, it will give you more control over progress. Second, it will facilitate the review. Of course, a smart PM should pay attention to this problem.
A big changeset can be created when moving to a newer version of a basic library (e.g. a framework) and sometimes you can’t avoid it, especially in bigger apps. In that case it’s best to minimize the extent of changes and don’t give in to update-mania. If there are many changes, you can also start the review earlier and work through them gradually - it can help you to avoid repeating the same mistakes. Looking over the final version will be necessary anyway.
7. Unclear comments
Commenting a line you don’t like with a “Fix plz” is not helpful. How can anyone know what you meant? I don’t think they can read your mind. Maybe they will figure it out, maybe they don’t have sufficient knowledge to fix it, maybe they don’t think there’s anything to fix. You won’t know for sure until you clearly communicate what is wrong with a given fragment.
How it should work:
Code review is also a place for confronting various ideas about the code. Different doesn’t always mean wrong. To create some space for dialogue, you need to define problems and propose some solutions. Without them there are no grounds for discussion or change of mind. Additionally, thanks to broader descriptions code review can become a place for programmers to learn from one another.
Code review is an excellent tool, one of the most useful ones. You can use it to improve the quality of the code, confront ideas or pass on your knowledge. It can also be a process for the sake of it. I think it’s worth trying to get the most out of it.
