This week's book giveaway is in the OO, Patterns, UML and Refactoring forum. We're giving away four copies of Refactoring for Software Design Smells: Managing Technical Debt and have Girish Suryanarayana, Ganesh Samarthyam & Tushar Sharma on-line! See this thread for details.
Roy's post got me thinking about the difference between code review and static analysis.
The static analysis vendors say static analysis is a type of code review. I use static analysis for two purposes:
1) To get me looking at potentially troublesome code. Bad practices tend to cluster. As a human, this code is worth more time.
2) To ensure certain practices never make it into the codebase. Why should a human need to look for a DateFormat object as an instance variable.
I certainly don't think static analysis replaces code reviews of course. I'm curious what others think on this. Both the people from SmartBear and more broadly.
At Smart Bear, we use both FindBugs and PMD for static analysis and we still review every line of code. There are certain things that static analysis tools are great at and some things that they just cannot do. They can detect most bad idioms (like your DateFormat example). They can detect inconsistent code (you checked for null before dereferencing a parameter once, but not every time within a method), which probably indicates a bug. They can detect complex code (by computing cyclomatic complexity) and flag it for further review or refactoring.
They cannot reason about whether the code works, conforms to the spec, and is maintainable. In my mind, this is the primary function of code review. Here are some questions that go on my mental code review checklist:
1) Do the unit tests validate the specified behavior?
2) Are there missing test cases?
3) Did the unit tests change? If so, was this an intentional change or a "get the unit tests to pass" change?
4) Can I understand the code?
5) Are variables well-named?
6) Are the comments sufficient?
7) Is there a better way? Especially a better way that makes use of some common code. Static analysis can detect duplicate code, but that's not exactly what I'm looking for here. I'm considering alternative approaches that would simplify the code by using some common library code.
The phrase "code review" is typically used to describe a human process, not a "computing" process. In reality, it most likely has different meanings for different organizations. In my experience, "code review" is a meeting of one or more engineers, one or more programmers, and a project manager or business analyst. The purpose of the meeting is to review the "design" of a software application or service.
Static analysis of code is a "computing" process and can be used to create documentation to be reviewed in a code review meeting. Or, it can be one of the steps that occurs before or after a code review, and maybe depend upon how well the code is written.
Basically, you can use static analysis and code review together.
Static analysis checks are only as good as the rules that the analysis uses to find stuff. Code Review encompasses many other types of "checks" that are not or even could not be codified into an automated rule. They may include reviewing code for functional integrity - something that static analysis does not typically focus on. Static analysis usually finds flaws in coding practices, such as logic problems that suggest the code is incorrect or possible exceptions in the code and doesn't understand features.
Static analysis does do a subset of code review and can do it on a much larger scale (even through most every path in the code). It's a good idea to use both static analysis and manual code review together.