7 Code Review errors
Code review can greatly improve the quality of your code. There is only one condition-to use this potential, it must be done properly. Without further ado, we move on to the list of the most common mistakes that cause us to waste this opportunity.
Focus on style, syntax
This type of review I’ve seen many times: lots of comments around how something was written, and more relevant issues without a single comment. This error is made mainly because it is easiest to catch such things. The programmer’s eye is used to searching for unnecessary spaces, unnecessary new lines or indentation. We do it from a vending machine.
If a programmer does something with a vending machine, it means that the vending machine can do it even better. To do this, you need to set up a style convention in the team (it can be one of the publicly available ones) and use tools to automatically correct the code in this regard. This convention must also be reflected in the development environment of every developer from the company-regardless of whether they use Emacs, vim, visual studio or intelliJ. Static code analysis tools pick up a lot of code anomalies and do it much more efficiently than humans. Both enforcing style conventions as well as code analysis should be a prerequisite for code review.
Review should start with the level of syntactic simplifications that automatic tools cannot perform. Here, in turn, it is easy to focus on syntactic simplifications or typos. However, the purpose of code review is to understand the whole of the changes and improve their quality. That’s why, when we don’t understand, we don’t click approvebut we’re trying to figure out what’s going on.
You see a lot of tests in the review, you just fly through them with your eyes so that you can get to the implementation as soon as possible. Believe me-I understand you:) reviewing tests is boring. Still just setup, assert, teardown. This repeatability quickly lulls our vigilance and it’s easy to assume that since the first few tests are OK, the rest will be OK too. Unfortunately, this is often not the case. Where do these tests that do not pass once a month (usually 30 or 31) come from? And the ones depending on the result of the previous test? Or the ones that only check for trivial things? This theoretically should not go through code review.
Tests are also code and deserve a decent review. Here is a good chance to make sure that the code is well tested. This requires a lot of focus and careful consideration of the proposed change-so it is not easy. The whole team must pay attention to this and without the support of all team members it will be a battle with windmills. Think about what and how you want to test, what is key in your application. Require each other to fulfill these arrangements.
Looking only at the added code
Do you pay much more attention to the green lines than the red ones? Yes, in most cases, what you add is more important, but an inadvertently deleted line can make quite a mess, unless you have a well-tested system. What if the deleted method was still needed somewhere? What if the last call was deleted?
Any changes to the deleted lines will show the intent of the previous implementation. Thanks to this, you can compare new and old, ask where the differences came from-unless it is obvious.
In addition, if a method has been removed in the changeset, code Review is required to check (or query) that all calls have also been deleted. On the other hand, it is worth checking whether the deletion of the call was not the last use of the method in the code. Maybe you can delete it and clean the codebase.
Half an hour before the demo is a very bad time for code review. Usually it looks like, along with the notification about the review, you also get a message from the author: tap me this, because in 15 minutes we are doing deploy on demo. A decent review takes time and peace of mind, and a quick review will not provide quality. Such situations often also arise from poor management, which sometimes makes developers not feel responsible for it. No matter who is to blame, the lousy code has been put into the wrong branch and will probably stay there.
A good habit is closing tasks well before presentationrelease or any deadlin’. Only this will ensure that there will be enough time both for the review and-above all-for subsequent amendments.
Sometimes, however, there is a big landslide, and showing a new killer-feature is to be or not to be. Then, instead of rushing the review, it is better to build a demonstration package from a different branch than the one to which the reviewed code goes. You have to. think about this possibility in advance and prepare the construction or deployment process accordingly.
Not understanding design
Another error due to too shallow entry into the role of the reviewer. It is easier to determine that the code will work and there are no glaring flaws in it than to assess whether its design is good. This is because you need to think carefully about what role the new code plays in the system and whether it fits well with what is already written. Tracing compliance with good practices e.g. PPE requires a lot of focus.
On the one hand, it should be examined whether the new components / changes can withstand a collision with the principles and good practices. Are they themselves well designed? The next step is to go to a higher level-impact on architecture. Most of the changes will be completely indifferent to her. However, those that change architecture are very important, and you need to think carefully about whether this is the right direction. You can, and even should, consult more people about this change.
Too big changesets
PR with the title Major refactoring system and statistics in style
60 files changed, 1740 insertions(+), 1202 deletions(-) this is a very bad idea. While it is probably possible to look at the changes yourself with the right effort, putting these changes in a broader context – business processes or applications – will be very difficult. Therefore, the quality of such a review will be inherently quite low. With this many changes, the reviewer can focus on syntax issues at most.
It is very important to divide the work into small pieces. Firstly, it gives better control over the progress of the work, and secondly, it greatly facilitates the review. Of course, here can help a lot properly kumaty PM, who already when planning will pay attention to this problem.
A large changeset can result from moving to a newer version of the underlying library (e.g. a framework) and is often unavoidable in large applications. Here it is always worth minimizing the scope of changes and do not fall into the mania of updates. In addition, if there are a lot of changes, you can open the review earlier and do them gradually-this will probably avoid repeating some of the mistakes. In this case, however, a thorough review of the final version will still be needed.
Marking a line that does not fit us with the fix plz comment is a common mistake. How is the other person supposed to know what we’re talking about? I don’t think he can read it out of our minds. Maybe he’ll figure out how obvious it is, or maybe he doesn’t have the full knowledge to correct it, or he’s right in this particular case. You won’t know until you write down clearly what’s wrong with the passage.
Code review is also a place where you can confront different ideas about code. Finally not always different means wrong. However, in order to create a field for conversation, it is necessary to clearly define the problem, as well as to propose a solution. Without this, there is no basis for dialogue or change of mind. In addition, with the broader descriptions, code review also becomes a place where developers can learn from each other, and this is very valuable.
Code review is a great tool, one of the more useful ones. It can be used to significantly improve the quality of code, confront ideas or transfer knowledge. It can also be a process for a process. In my opinion, it’s worth trying to squeeze the maximum out of him.