Practical tips for effective code inspection
Inspecting code is a very complex and demanding task that can consume a lot of resources. It is important to conduct an inspection responsibly and conduct it effectively. Effective - this means spending little time and finding a lot of defects. But how to increase efficiency? Below are some tips to help you with this.
Often, when modifying a code as part of a task, it comes to understanding that this code should be refactored first. It is not necessary to include refactoring and task implementation in one inspection. They should be divided into two different inspections. This is necessary to simplify the life of the inspector and increase the chance of finding an error by him, because if you combine in one inspection a module with potentially dangerous changes with another 100 modules that were modified using automated refactoring (for example, renamed a class), that is, a high probability that the inspector will skip the potentially dangerous changes.
In other words, it is not necessary to combine solutions of different tasks into one inspection. Refactoring is a separate task, even if it appeared as a result of work on another task.
Many tools that automate code inspection allow you to leave comments on the source code and defects (for example, Code Collaborator). In this case, heated discussions often arise when the inspector starts a defect, and the author begins to explain to him in the comments that this is, in fact, not a defect at all. As a result, a holy war begins, which can go on forever. Avoiding this is very simple: you do not need to leave comments at all during inspections. In this case, the inspection process proceeds as follows:
Before conducting an inspection, it is imperative to make sure that you accurately understand the problem being solved by the injected code. It sounds like Captain's next revelation. Evidence, but practice shows that often the inspector mechanically does his work, looks at compliance with formal stylistic requirements, without delving into the task. The code can be as “beautiful” as you like, have all the SOLID characteristics, handle all possible errors, but if it does not solve the task, then this code is suitable only for demonstration at the next interview.
Before conducting an inspection, while your eyes are not blurred, think about how you would solve the task? If you suddenly find that your decision is significantly better than that provided for inspection, do not hesitate to report it to the author. Better he fix it now, while it’s not so expensive and it has not been crutched.
When introducing defects, keep yourself in control and do not slide down to taste. Taste leads to conflict, lost time for holy wars and is not constructive. Avoiding it is very simple: before instituting a defect, you need to prepare a set of compelling arguments why this defect needs to be fixed. Arguments like “I like it better” or “so prettier” are not considered valid. If there are no other arguments left, then there is no need to start a defect, let the code remain in the "author" version, even if you wrote it differently.
Look for alternative scenarios. Often, the programmer is focused on the main thread of the program and forgets to correctly process the alternative ones. It is easier for you, as a person not immersed in the task, to replace such a puncture.
All defects have their own unique smells, which help to detect them great. The smells of architectural defects are well described in Martin Fowler's book “Refactoring,” which is required to be read by any code inspector. But not only architectural defects smell, their algorithmic ones also have their notes. For example, if a multi-threaded code in a lock accesses another code from which a callback can be made, or several synchronization objects are used, then it smells of deadlock. Smells of algorithmic defects are a good topic for a separate article.
PS: A very big request to share your comments on code inspection in the comments. What would you recommend for an effective inspection?
Cutlets separately, flies separately
Often, when modifying a code as part of a task, it comes to understanding that this code should be refactored first. It is not necessary to include refactoring and task implementation in one inspection. They should be divided into two different inspections. This is necessary to simplify the life of the inspector and increase the chance of finding an error by him, because if you combine in one inspection a module with potentially dangerous changes with another 100 modules that were modified using automated refactoring (for example, renamed a class), that is, a high probability that the inspector will skip the potentially dangerous changes.
In other words, it is not necessary to combine solutions of different tasks into one inspection. Refactoring is a separate task, even if it appeared as a result of work on another task.
Correspondence Boxing
Many tools that automate code inspection allow you to leave comments on the source code and defects (for example, Code Collaborator). In this case, heated discussions often arise when the inspector starts a defect, and the author begins to explain to him in the comments that this is, in fact, not a defect at all. As a result, a holy war begins, which can go on forever. Avoiding this is very simple: you do not need to leave comments at all during inspections. In this case, the inspection process proceeds as follows:
- Inspector starts a defect.
- If the author agrees with the defect, he corrects it, after which the inspector closes the defect.
- If the author does not agree with the defect, then he verbally discusses this defect with the inspector. No comments are added to the defect.
- If the author has successfully agreed with the inspector, then, depending on the agreements, either the author corrects the defect or the inspector removes it.
- If a holy war has begun, in which neither side wants to give in, then an arbiter is called to help, who will judge the inspector and the author.
What are you doing here?
Before conducting an inspection, it is imperative to make sure that you accurately understand the problem being solved by the injected code. It sounds like Captain's next revelation. Evidence, but practice shows that often the inspector mechanically does his work, looks at compliance with formal stylistic requirements, without delving into the task. The code can be as “beautiful” as you like, have all the SOLID characteristics, handle all possible errors, but if it does not solve the task, then this code is suitable only for demonstration at the next interview.
What muck is this your fish in fish!
Before conducting an inspection, while your eyes are not blurred, think about how you would solve the task? If you suddenly find that your decision is significantly better than that provided for inspection, do not hesitate to report it to the author. Better he fix it now, while it’s not so expensive and it has not been crutched.
All markers are different in taste and color.
When introducing defects, keep yourself in control and do not slide down to taste. Taste leads to conflict, lost time for holy wars and is not constructive. Avoiding it is very simple: before instituting a defect, you need to prepare a set of compelling arguments why this defect needs to be fixed. Arguments like “I like it better” or “so prettier” are not considered valid. If there are no other arguments left, then there is no need to start a defect, let the code remain in the "author" version, even if you wrote it differently.
And if he had a grenade in his pocket?
Look for alternative scenarios. Often, the programmer is focused on the main thread of the program and forgets to correctly process the alternative ones. It is easier for you, as a person not immersed in the task, to replace such a puncture.
About the benefits of smell
All defects have their own unique smells, which help to detect them great. The smells of architectural defects are well described in Martin Fowler's book “Refactoring,” which is required to be read by any code inspector. But not only architectural defects smell, their algorithmic ones also have their notes. For example, if a multi-threaded code in a lock accesses another code from which a callback can be made, or several synchronization objects are used, then it smells of deadlock. Smells of algorithmic defects are a good topic for a separate article.
PS: A very big request to share your comments on code inspection in the comments. What would you recommend for an effective inspection?