Automated Code Review is Not Enough

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

Static Code Analysis is just tip of the Iceberg!

This post is also present on the blog site of my current employer and has also been discussed on InfoQ.

 

Most of the times we are content that our code is of the right quality, if somehow, we manage to get the Static Code Analysis (SCA) tools like Checkstyle, PMD etc. report less number of severe violations. As an example if we see that the class is big in size then we conveniently split it into two or more classes to get rid of the violation. The tool is happy and so are we and most of the times that is the end of the story.

 

However more frequently than not getting an SCA violation is the start of the story. If you start associating the question “Why’ with every SCA violation found then the real reasons start unfolding.

 

This is similar to the way we resolve impediments on an Scrum project. The impediments rarely represent the isolated incidences of inefficiency. Rather, most of the times they are a part of a larger problem. The way to work out an impediment is fix it so that the team can work effectively and then to look at the root cause which caused the impediment so that the main cause can be fixed. This is called “Bottom-up process re-engineering.”

 

Similarly the way to work out an SCA violation is to remove it so that the code looks clean and good and then to hunt for the real cause.