Code Review Tips for Every Level of Detail

Ensuring Code Quality Through Comprehensive Inspection

Code reviews are more than a mandatory task to make code contributions more formal. They are opportunities to collaborate on maintainable and robust code implementation, which everyone can commit.

This article lists the tips and main points for performing code reviews at every level of detail, from catching minor bugs to assessing overall code architecture.

It is good to note, that every team should work in a way, they find suitable for them and do not force techniques, which would create redundant work for not existing benefits. It is also highly dependent on the actual language and used framework.

I have a checklist through which I go through every review and it goes as follows:

1. Architecture

First of all you have to ask yourself these questions. Do the changes:

• fulfill the milestones, which are currently set?
• follow the architecture that is set up for the project.
• adhere to processes set in the project?
• provide safe and reliable solutions?

If any of the questions have no appropriate answer, then it is good idea to rework whole contribution, so it does not potentially break anything at the project.

Examples of such contributions could be:

• adding redundant/unverified libraries to the project
• following other architectural concepts instead of following the project consensus
• breaking changes before the imminent release
• exposing sensitive information in logs

There is always an established way to do things, and it must be respected, but embracing change is also valid.

Changes compliant with the project philosophy diminish the required mental capacity to understand them.

If the architecture is completely off from the common implementation patterns, then it is worth investigating, why the developer did it this way. It should not happen in the future that the developer will need to rewrite the whole solution.

Moreover, the author of code should not try to rewrite important parts of the code, if they are not needed.

2. Functionality

Secondly, the code should do the job for which it is designed. If there is a flaw in it, it must be fixed.

Couple of questions, which you can ask yourself:

• Is it possible that it works? Is the implementation dubious in some way?
• Are any possible erroneous scenarios covered?
• Are there possible race conditions or deadlocks?
• Does the code introduce some vulnerability/weakness into the design?

If you have doubts, the best idea is to ask the developer to explain the changes or ask for a demo. It is the fastest way to resolve miscommunication or misunderstanding.

3. Complexity

Over-engineering and over-optimisation are double-edged swords in any kind of engineering. They can lead to error-prone or slow code in the long run, if more adaptations are added.

Simplicity is the ultimate sophistication — Leonardo da Vinci.

What I search for:

• If you can read the class and understand it, it is fine. If you find yourself running around the project, it might have too many dependencies and should be simplified/generalized.
• If you see the easier way, it should be taken - it is a learning experience for more junior developers
• Is it speculation or a solution? If there is no sign of an issue, then the part of the code can be eliminated.
• If you see spawning of threads and routines to do simple tasks - it should be removed, because it can make code slower. E.g. parsing small JSON does not need separated thread every time.
• If it works and does not slow down the user, then you do not need to optimize further.

Grinding the software solution to make it a couple of milliseconds faster can be counterproductive and brings no value (depending on the use case), so if it works, it works. Do not touch it! Do not improve it!

4. Testing

Tests are part of any development and not something extra. They should be treated as part of the main codebase. Do not compromise on the quality of code here too. The vastness of added tests should be related to the size of the introduced changes.

• Are the tests valid? Do they test implemented functionality?
• Do they cover all the reasonable scenarios which can happen?
  • too many tests can be considered wrong practice too - they can cause false positives in future and create more of burden than help
• Are they consistent with the whole test set? Do they follow established patterns?
• Are they properly implemented and independent from each other?
• is CI/CD pipeline green after running the test with changes?
• Are factories for the classes reusable in other tests too? (to learn more, you can read my article about this here)

5. Code Styling

All points above are more important than the actual style of the code because if any of the points above are lacking in some ways, it can result in a whole rewrite and the naming of the variable goes out of the window.

Even tests are above it because if you are not able to write tests, then there is something wrong with the architecture and you should rethink your strategy.

If everything seems fine, you can start digging into more details like:

• explicit and intentional naming of variables
• comments explaining more why this code is here than what it is doing
• applied styling by IDE (every language has its way, how it should be formatted)
• FIXME and TODO comments
• description of tests and their arrangement
• redundant changes in code with no benefit
• fits the design of the project

”I will do it later”

Never fall for this trap, if someone says they will do it later. If there is no urgency, do it now and you will not hit hidden issues in the future.

Now is the right time to do it.

If there is no time for a change, create an improvement ticket or write at least a TODO, so the change can be in the queue.

Code of behaviour:

• be polite
• be helpful
• be reasonable
• leave actionable feedback
• guide junior developers, so they can grow to your level
• technical fact overrules the opinion
• invite other developers to share knowledge or make them familiar with large changes
• do reviews promptly

Conclusion

Remember - by reviewing the code you take responsibility for the code on par with the developer who programmed it, so do not be sloppy about it. You can be the one fixing it some time later and getting annoyed about the quality of it.

If you do not understand the code, at least ask what is meant by it. You will find out more about it and can decide how to make it better, if it was not clear at the beginning. Not understanding the code is the start of future bugs.

Hopefully, these tips and points will make your review process more structured and help you find and uncover issues before they happen in the production system.

Thanks for reading and follow for more!

Subscribe for more