Code review chopped up
Many approaches can be taken during the code review process
The most popular way of reviewing code that I was facing when working with various companies was just focusing on details. Especially when we don’t know the context.
I’d like to stop here for a moment. Let’s consider the following situation. Do you remember the time when you were reviewing a piece of code and found a perfect fit for one of the “Gang of Four” (design Patterns: Elements of Reusable Object-Oriented Software (1994)) patterns? Did you try to understand why someone didn’t use it? Or you felt that catchy moment and the finger-pointing comment land right away on the pull request? There could be a reason why that obvious pattern wasn’t used there, but you couldn’t know it because you didn’t know the context.
Knowing it can also let you understand better naming, grouping, and other decisions made by previous engineers. On the other hand, we have to remember that context is a double-edged sword and may close our minds. If we know the context, everything is clear and readable so we don’t see many problems that we would catch not knowing it. Keep that in mind and always try to put on newcomer’s shoes when figuring out the best class or variable name.
Don’t get me wrong but thinking about code review only from the “clean code” level can be just polishing on the surface. Pointing out issues with nested conditionals or multiple params in the method is helpful and we should keep doing it. But honestly, it’s not the most valuable outcome of code review. Even with such mistakes included, the software will work as expected. And truth is that it can be maintained by an experienced programmer without issues. And further, a couple of nested “if” statements are more readable and easier to understand for the average programmer than the decorator pattern. Our main focus should be a context in which the reviewed piece of code is placed and look for possible problems there.
Software architecture supposes to reflect business logic and is the most crucial part of software which we should focus on. Also and especially during code review. If we identify issues in this core and quickly respond to them, it will bring the biggest value to the product.
We definitely shouldn’t review the code with an aggressive stance. Wishing to find as many mistakes as possible and pointing them out loud in the comments is what usually junior programmers are doing. Or the programmers that look for “revenge”. This can make us feel sad if we don’t find anything wrong at the end :). Taking such an approach tunnels us to think about small details. We are losing focus on the bigger picture of the architecture where this piece of code will be placed. It’s always better to start a discussion about possible use cases, problems, and needs of scaling in the concrete scenarios rather than imposing design pattern because it matches there.
However, even poor quality code review is better than none
There are many companies over the world which are not reviewing their code before merging. Sometimes in such places are happening fast actions where one (probably new) team member sets up a code review tool and announces the initiative of doing code review. Starting from Today! And it works. For a month. Then everybody forgets about doing the code review, because of lack of time, lack of resources, and other excuses you may think of. You can always find an excuse. The code review process will stay as it was, but won’t be anything else but just collecting approves. No code review leads to producing low-quality software. Software companies and engineers know the consequences of it.
Code reviews habits
It’s worth to mention about differences between code reviews habits through teams. It will be different because of the experience diversity of the team members. We may see some constructive comments and catches which bring a lot of value to the code. Through the interaction of a few junior and senior engineers, it’s very possible to have a long discussion about the problem.
It’s slightly different when it’s a team of juniors guided by one senior. Code review in this combination will look like guidance.
An interesting case is when code review is happening in the team of senior engineers. Sometimes in such setup, each of the proposed solutions is not better than the other. It’s more about preference and learned guides. Most senior engineers have their habits and patterns to follow when writing the code. Trying to change it can be difficult. They will fight for their “rights” with very reasonable and true arguments. A very important ability is to give up and find a compromise. This is what distinguishes seniors from other programmers.
A common issue with software engineers is that they feel a special “connection” with the code. Because of this, they may take every comment personally. Reviewers have to be very careful in commenting and proposing changes. Those can cause discomfort that may set a fence between the author and a reviewer.
Key code reviewers
Knowing the architecture and the specifics of the project is the responsibility of most involved and experienced engineers. And they are the key code reviewers. They can easily find low cohesion, violation of SOLID rules, layers nomenclature leaks, and other possible mistakes that may bite our tongue in the future. They can propose splitting, extracting, reusing, or moving parts of the code for easier maintenance and readability. And the whole rest of the benefits it can give us. Foreseeing consequences of the proposed solution, considering possible code misplacement or duplication is what they can do best.
Those are the things that should be at the top of our minds when reviewing the code.
For sure architecture violations bring the worst consequences. Worst than a couple of arguments in one method or too many lines of the class.
Three levels of code review
Considering all of the mentioned rules and ways of doing code review brought to my mind splitting code review in a hierarchical order based on its importance and effect.
Architecture
First and the most important should be an architecture focused code review. Here we should consider project specifics and agreements we made before. This level of code review should be done mainly by the most experienced programmers who understand the architecture of the project. Juniors can try to do this level code review as well and it can be a good exercise. However, they don’t have to stretch their minds too much if they don’t feel strong enough to do this.
At this stage, it’s very important to recognize possible nomenclature leaks and violations. There may be a rule that is clear to you because you know the project, but not necessarily everyone is aware of this or it may be transparent for them. This is a good point to start a discussion and figure out the motivation of the author.
Architects and engineers on the architectural level code review should also keep in mind the needs of scaling and reusability of a proposed pull request. Knowing this can unconditionally change the implementation.
Application security is also one of the key factors to keep the focus on here.
There are other architectural things that we can consider doing a code review. I think you’ve already known how valuable it can be to find an issue on this level.
Code clearance
In the second phase of code review, we can dive deeper into the code itself and be more detail-focused. It can be called a code clearance review.
Now we can argue on cohesion levels and class composition. It’s beneficial to discuss all of the doubts and confusions to better understand the thinking process behind the code. We always should ask first before blaming someone. It’s maybe something as clever that we couldn’t understand it.
Speaking of “clean code” (claps to Uncle Bob Marting and his “clean code” book) we can’t forget about SOLID rules and testing. If any of these rules are violated, we should quickly figure out why. There could be a reason why an engineer decided to not follow this rule, but it could be a mistake as well. Ah, good catch!
One of the hardest parts of programming is naming. It’s difficult to name variables, methods, functions, classes, and data structures in a proper way. Some names may be so obvious to you and totally unacceptable for others. Even that we should always try to find the best “names”, second-level code review is a place to start discussing it.
Polishing on the surface
Third and the last phase would be polishing on the surface code review. Here we can point out things that usually can be automatically caught by linters or compilers as code formatting, missing semicolons, and other trivial issues.
It’s also time for small sorting/grouping updates, complaining about used data structures, whether it’s a List<> or Set<> and so on.
It doesn’t matter who opened the pull request. Code review should be done always with the same rigor. Even experienced software engineers don’t know everything. Even about the product, they are working on. In the end, we are all people and we make mistakes.
