Automated Code Review is Not Enough

Posted on Tuesday, December 9, 2008

4


Code Review is considered to be the activity of verifying source code and rectifying mistakes in order to improve the quality of the code. Many times code review is associated with the painful process of painstakingly verifying code line by line and talk about a lot of mundane stuff. As a result of this many times code review becomes a mere eye wash when it is done manually.

Hence, came the tools like Checkstyle and PMD and many more both in the arena of open source and commercial softwares which helps the team write better code and check that automatically. Most of these tools have IDE integrations so that the code which is churned out is corrected at the source. Even if the developer wishes to check in code with checkstyle errors then there could be a gate just before the check-in process using pre-commit and post-commit handlers of version control systems like SVN and CVS.

{Sidenote – SVN allows atomic checkins hence this check can be easily applied there however CVS does not provide atomic check-in hence the pre-commit check is a little dangerous there.}

Anyway, back to main discussion

Automated code reviews can take care of mundane checks like code formatting etc. they can in general provide the following level of checks (from PMD)

  • Possible bugs – empty try/catch/finally/switch statements
  • Dead code – unused local variables, parameters and private methods
  • Suboptimal code – wasteful String/StringBuffer usage
  • Overcomplicated expressions – unnecessary if statements, for loops that could be while loops
  • Duplicate code – copied/pasted code means copied/pasted bugs

So, in a nutshell the code reviewer would be absolved of the trouble of going through the above checks. However, if you notice then there are many things which would still need to be covered depending on the enterprise standards.

For example, one would have to manually check for details like

  1. Algorithm implementation – Is this the most optimal way to code business logic? Agreed that it works and the unit tests work fine too however is the implementation the best possible one?
  2. Design Sync – Is the code in sync with the design which was discussed?
  3. Does the code have the required tags? – Sometimes there might be scenarios where you would like to include ‘usage metrics’ tags for your website. These tags get included in the JSPs/ASPs. Somehow these need to be verified.
  4. Exception handling / Recovery routine - This becomes primarily important for Availability and Reliability needs. Not only the system needs to be available however the system should be able to log exceptions such that the recovery from error is fast and hence the system is more available. Similarly fault tolerance needs to be built into the system so that it can ensure failure free operations for a specified period of time in a specific environment.
  5. Sometimes depending on the business criticality of the component you might have different quality standards. For example, a business critical component is considered done when there is a code coverage of more than 80%, likewise for a non business critical component the code coverage of 60% could be fine.
  6. Performance requirements might require that the code should be profiled to ensure that there are no memory leaks. Agreed some tools might predict some scenarios which lead to memory leaks but somehow it needs to be ensured that profiling has been done for business critical components.

This is just a representative list, there could be many more scenarios where there would be a need for manual code review on top of an automated code review.

One might argue that pair programming as advocated by XP would take care of many of these. Yes, it would. If you have a team of talented developers who know what to do and what to look at then you need not care about manual code reviews. But, if you feel that you have only a handful of those talented developers who should be able to review and mentor others then there is no escape from manual reviews.

Thus, though automated tools would take away some pains from the code review process however there are many other things which need to be checked in an enterprise and for that manual code review might be needed. The trick here is to do sampling of code. For example pick the pieces of code depending on their complexity and business critically. Apply the 80:20 rule and you should be good.

About these ads