Good and bad Code Review examples
See, with specific examples in the code, what a well-done Code Review looks like and how to avoid bad code inspection.
In the last article of our Tech Stories series, we devoted a lot of attention to code review good practices [LINK] - what is worth paying attention to when inspecting the code, what tools to use and what procedures in the team should be implemented in order to actually both the author of the code and the programmer checking it, understood their roles perfectly and were able to communicate. Therefore, now we will deal with specific case studies of the code review, starting with the bad ones, because it's best to learn from mistakes.
Bad Code Review - Examples
There is a myth that a bad code review is one where someone urges us to change our code even though what we wrote is correct. Worse, the person's proposed changes will result in mistakes. Such situations, however, occur extremely rarely - they are the result of a mistake or ignorance of the issue, and most often they happen to newcomers who are relatively easy to explain why their comment was badly wrong.
In practice, the greatest risk that comes during the code reviews is due to rush or when we are already tired. It is then that we do not notice errors that we would otherwise notice immediately.
Unfortunately, the most overlooked bugs in the code are not those that cause little damage to the overall design that are easy to fix, but:
- errors related to breaking the generally accepted architecture,
- bugs consisting in incorrect handling of all edge cases
- errors in the logic of our application (incompatible application behavior).
Inconsistency / systematic error
One of the common problems is inconsistency with the person checking the code. As an example, let's take the code below:
The programmer responsible for code review aptly noticed that in the doSomething method, the code author did not correctly declare the types for the input parameter and the return value. However, he didn't notice the same for the internalMethod.
Such errors result from a lack of systematic checking of the code and they can be easily dealt with by adding to your code review checklist:
Do all methods and functions have declared input parameters and a return value?
The lack of consistency in code reviews manifests itself in many forms - it may concern checking coding standards, correct declaration of types or verification of variable and method names.
At first glance, these are not very large and persistent errors, but their systematic accumulation may have serious consequences in the future.
Unresolved conflicts error
During code review, disputes often arise between the person checking the code and the author of a given fragment. There is an exchange of views, but nothing comes of it. Let's look at the following code snippet:
The following exchange took place here:
- Checker: reports a problem with the name of the attempt method, suggesting to use getter or getAttempt (),
- Author: Disagrees with this suggestion, stating that getter is used when a process actually fetches data from another source rather than directly returning data from an object.
- Checker: inserts links to pages presenting examples of using the getAttempt () methods in this case,
- Author: replies with links to sources confirming his approach.
After exchanging a dozen or so comments, the situation is not clarified in any way. It is not known which approach is more appropriate. The tension between people begins to feel dangerously perceptible.
Such situations arise when we did not include a stalemate resolution strategy in our code review process. In order for the team to operate efficiently in the event of divergent views, we should have such a process that allows us to make the final decision in advance. It may be consulting another person or, in extreme cases, gathering more of the team to determine together what our code should look like.
A good example of resolving the same CR conflict: setting standards together
The above situation could be resolved in an amicable manner, e.g .:
- Checker: reports a problem with the name of the attempt method, suggesting to use getter or getAttempt (),
- Author: disagrees with this suggestion, stating that getter is used when a process actually fetches data from another source, rather than directly returning data from an object.
- Checker: agrees with the author of the code, but points out that it is better to name such methods so that they indicate the actual functionality. This way, eventually the method name is changed to numberOfAttepmts ().
Good Code Review - examples
Spreading knowledge
Code review creates excellent conditions for the exchange of knowledge about the programming language and certain technical dependencies that cannot be written directly into the code, but result from experience. Let's look at the following code overview example:
For a class defined in this way, the person conducting the code inspection began to wonder about the sense of using the sleep () function here. What raised her/his doubts was the impact of this feature on the performance of the solution.
The reviewer raised his/her doubts, and the author of the code explained that it was a deliberate procedure that results from the need to maintain a small number of API calls that take place further in the code and that could lead to instability of the solution.
Excluding potential errors
One of the biggest advantages of code review is catching potential errors at a very early stage of product development. It is much less expensive than when the error is only found on the production server. Let's take the following code as an example:
The first thing that we can observe here and already reported in code review is the lack of declaration of input parameter types and the return value of the doSomeFormatting method.
After reporting it to the author, he can inform us that the method declaration should look like this:
If we are an astute programmer, we will immediately notice that such a type declaration causes our code to return the wrong result on line 13.
By analyzing the code further, we can conclude that most of the body of this method is redundant and may cause unnecessary errors.
The whole thing, after properly conducted code review, may look like this:
The method written in this way is much simpler and does not carry the risk of an error related to calling it with an incorrect type of the output parameter or errors in handling the returned value.
Summary
There is no need to convince any programmer that it is worth doing code review. It is worth remembering, however, that just as programming is a skill that we constantly develop, good conduct of code inspection also requires from us constant improvement.
When we review the code correctly, we not only improve the quality of our code, but also the knowledge and skills of programmers.
Author of the article:
Mateusz Antkowiak | PHP developer Transparent Data
Professional programmer for almost a decade. He began his journeys with code from such exotic destinations as Lazarus Pascal or Delphi, and now he is drawn into the stable waters of object-oriented programming. He has extensive experience in process automation, which is useful in his current RegTech tools in Transparent Data.
You might also like:
