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

Continuous Integration in a Huge Multi-Module Project

The benefits of Continuous Integration cannot be discounted in any environment whether you are working on a small project or a huge one. The difference being in the set up and configuration of continuous integration in a multi-module project.

Multiple modules bring in additional complexity in terms of versions, teams collaborating, release cycles, separate sprints etc etc. In an ideal world all this would not be necessary and we would have just one module for the entire system. With every developer check-in the CI build would trigger. Of course the CI box would have unlimited processing power, load distributing capability and unlimited storage to still build the system of any size under 10 minutes.

The magic number of 10 minutes is an XP practice for CI where it is correctly stated that if a build is taking more than 10 minutes then the value of CI is lost. If the time is more developers would check in less frequently because who wants to wait for say 2 hours to get the status, they would rather check-in their code end of the day and even worse not wait for the build to complete. A broken build, which would happen many times would be fixed the next day. Now just assume that the team is distributed across time zones and are struggling with a broken build when they step into the office.

Let us switch the real world and agree that having a huge system build under 10 minutes is a far fetched dream. If you are working in a huge enterprise, where various complex systems are collaborating together then you would agree to my point.

So the best way to build a huge system is by distributing it into multiple smaller systems or modules and then building each module separately so that each module can build under 10 minutes.

Taking a simplistic view of the real world let us assume that we successfully divided our system into 3 modules.

System = Ʃ modules (A,B,C)

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.