What is Code Review? - Guidelines and Best Practices - NDepend

Code review is the process of mandating systematically one or several developers to review the code written by another developer in other to detects defect and to improve it. Code review is more often performed by an experienced developer considering the various aspects including the quality and security of code, sharing the knowledge, enabling better collaboration, building a review culture, building confidence in the Team on the code they are working on.

Code Review Costs

In addition to code reviewing, there are two other well-known practices to streamline the development process.

• Writing tests to exercise often and automatically the code written and refactored.
• Using tools that inspect the code to automatically find code smells and bugs.

Unlike tests and analysis tools, code reviewing made by human can be seen as a smoke task. The word smoke is used per analogy with smoke testing i.e manually performed testing to quickly validate a particular point. Such test is deemed as smoke because when the code changes, it must be performed again, exactly like code review. For that reason code review is a costly task because it consumes human resources and it must be repeated again and again, after each code change.

Code Review Benefits

Code review cost is high. On the other hand code review benefits are real.

• Education: With code review, junior get advices from senior programmers within the context of their own code. There is no better way to learn the right practices and promote the company’s code guidelines and requirements.
• Communication: With code review programmers necessarily talk to each other and share opinions. Doing so fosters companionship and improve the team cohesion. 
• Knowledge spread: Every team should struggle against code ownership i.e when one person owns a code base or a component. Code ownership is provoking friction and stress, both for the owner and for his/her colleagues. The owner doesn’t learn anymore from others and can become lazy since its work is not assessed by others. On the other hand the others can be let with a massive problem when the owner is absent or if he/she leaves the team. Code review is the best practice to spread knowledge and prevent code ownership.
• Motivation to progress: It is clear that if you keep in mind that your code will be reviewed by some colleagues, you will put more care into it. You will take the time to properly name identifiers, write tests, introduce well fitted abstractions and use carefully the enterprise wide patterns.
• Better code: Because the developer intelligence, experience and attention is at the core of code reviewing, it can help detect early defects that cannot be spotted through testing and tooling.

With code review not only the resulting code gets better but everyone improves as a developer. Those human-related gains cannot be obtained through another practice. However they can make the difference between a successful project and a failure. Thus you have to experience systematic code review seriously to notice that its (ROI) Return on Investment is favorable.

How To Perform Code Review

Notifying that some code is ready for being reviewed

Often code review is conducted the informal way. When a developer ends up a coding session he notifies one or several team members to review it when they can. This is a start but doing so is not systematic, not traceable nor measurable.

Over the Shoulder Review

Unlike the previous notifying way, over-the-shoulder reviews involves a synchronous review. It consists in asking one or few developers to join to present them the work done and gather comments. Ideally they join physically but nowadays, many developer work from home and such review can be made long distance. Over-the-shoulder review suffers from the same informal cons of the previous notifying way. However it encourages developer to engage a real discussion and not just another mail thread, which is good for team building.

Pair Programming

Pair programming is an Agile software development approach that comes from XP (Extreme Programming). Pair programming consists of two developers team together on one computer. They join their efforts to write code and tests. This is a form of code review since there is a single keyboard: one is reviewing the code written by the other.

Pair programming has several advantages. This is a great way to mentor a junior developer and to catch some defects as early as possible.

On the other hand often developers reach their productivity peak when they are alone, in the flow zone and not interrupted. Systematic pair programming can thus impairs the productivity and make developers less enthusiast. Code written by a single developer in-the-flow-zone can be still reviewed and improved later by others.

Tooling to help for Code Review

Code review tooling helps a lot to improve the reviewing process. With tooling reviews can become systematic. They can be traced and some metrics can be inferred to improve the process. Github’s pull requests are now popular to conduct most review needs. There are also more sophisticated tools like SmartBear Collaborator with a wide range of SCM (Source Control Management) supported.

However the downside of code review is its high cost because it requires a lot of human effort. This is why the team must struggle to automate review to a large extend to narrow human intervention to essential points. To do so the tool NDepend analyzes many dimensions of the code like naming, complexity, design, code smells, code coverage by tests… and produces snapshots. Two snapshots can be compared. Some C# LINQ based queries make it easy to query any dimension within the delta between the two snapshots compared. Here are some sample queries.

Prior to any human review developers can be forced to avoid making already complex methods even more complex thanks to the query below. Notice the prefix warnif count > 0 that transforms the query into a rule. This way the matches of the query become issues to fix:

A rule can be easily written to force 100% code coverage by tests on all new classes written. But certainly developers will find it a bit extremist because some code like UI code can be hardly tested.

However they will certainly accept this rule: Types that used to be 100% covered by tests should still be 100% covered. We can be proud when a class is 100% covered because it means that it is well designed since it is fully testable. 100% coverage also means that if the class contains a bug, it is easy to adapt unit tests to reproduce it in order to fix it. Once the bug is fixed, with full coverage it is easy to write one or several tests to assert the fix. However we need a way to prevent new bugs to be introduced in a 100% covered class. And this rule helps a lot because it spots new hole in the class coverage. Experience shows that new hole in testing coverage almost always shed light on some interesting facts – if not bugs – that require the developer attention.

Typically the baseline chosen to pass the rules is a snapshot made on the last release in production. Many more such rules on the delta since a baseline can be written to streamline significantly code review. Some related default rules categories are:

Also NDepend can help focus on news issues introduced since the baseline to automate more aspects of the review like naming, Object-Oriented usage or code structure. This is important because it is not practicable to fix all problems spotted by a tool. The right way to use a static analysis tool is to focus on new problems introduced since a baseline.

What to look for in a code review

We just explained that with a tool like NDepend, a lot of aspects of review can be automated. However human skills and experience remains essential to detect important improvements:

• Requirement mistake: A developer might have misunderstood some requirements. His code might be well 100% tested but if both the code and tests written are based upon wrong assumptions the change cannot be accepted.
• UI changes: Designing proper user stories that will lead to a smooth UI that will make users productive is one of the most difficult development task. Code review and UI review by the team experts is mandatory to make sure the product evolution goes into the right direction.
• Hard to detect bugs: Bugs that are related to concurrent programming (deadlock, race-condition…) are hard to detect, hard to reproduce and hard to fix. Junior developers often produce such bugs without even suspecting it could happen. On the other hand, when reviewing code executed concurrently, a seasoned programmer will be able to assess if a bug is possible or not. He will check for state mutability and method purity for example. Btw, NDepend provides some rules to enforce these sort of constraints. During code review some other tricky bugs can be spotted including memory leak and performance smells, like the infamous N+1 selects problem.
• Vulnerabilities: There are a wide range of potential security pitfalls that require an expertise. Some tools like Semmle can automatically detect many of these pitfalls. But if security is a concern for you (and it should be) regular review made by a security expert is a must.
• Naming: Code rule can be written to enforce simple naming requirements like an interface name should become with an I. Tooling can go further to force the usage of a predefined core domain terminology. This is know as Domain Driven Design (DDD) Ubiquitous Language Check. For that NDepend provides a rule customizable with the domain vocabulary. However proper naming is sensitive, especially for API identifiers that will be made public. This is why careful human review is required.
• Testing: As we saw testing can be measured and passed automatically to a large extend. But code coverage ratio and number of tests passed are not enough. Human must review the tests to make sure that the proper conditions are asserted and that tests are not overly complex.
• Consistency: Often juniors in a team are re-inventing the wheel because they are not aware of enterprise-wide patterns and libraries. Some automatic rules can be written to force the usage of some particular classes in certain situations. But code reviews represent a great way to mentor about the company culture and knowledges.
• Comment: It is easy to enforce a certain ratio of comment but this is not the point with commenting. Commenting must be written carefully in proper English to explain why the code was needed in the first place and why the code was written this way. For example, when some non-trivial code is adapted from a stackoverflow.com response, it is worth mentioning the response url and the context. On the other hand, code should be readable enough to avoid having comment that explains what the code do.
• Documentation: Tools can easily detect missing documentation. An IDE like Visual Studio can easily generate the skeleton of a class or a method documentation. But documentation is written by human and for human. A proper human review is needed to ensure that the documentation is really informative. We -as developers- are regularly frustrated by shallow and outdated documentation that doesn’t really explain why an API is needed, how it should be used and what you can expect from it. Proper documentation is an excellent way to reduce both support cost and users frustration. It deserves careful review.

No tool will be able to detect the kind of defects just listed. At least not until Artificial Intelligence make some tremendous progresses in code understanding. This will necessarily happen but in 2021, it still remains science-fiction. More on this subject can be read in this post Is Artificial Intelligence Assisted Coding the Next Developer Productivity Silver Bullet?

Conclusion

Code review benefits are widely acclaimed by the community and if you are not practicing it in your team, you have a serious room for improvement. However, because code review is consuming a lot of precious human time, it is essential to use the right tools to streamline the reviews coordination and approvals and to enforce facts that can be automatically verified from the code itself. This way reviewers deal with clean enough code and can focus on points that really require their expertise.