Should static analysis be integrated with code review? [closed]

Question

I want to integrate various static analysis tools, and then add the results as comments on a file within code review tools, such as Stash or Review board.

I am exploring the feasibility of writing such a tool, I have a prototype working for python, using pylint and pep8. But have not integrated it with a code review tool yet.

Additionally it should be relatively easy to add support for other languages, for example C++, via something like CPPCheck, or any static analysis tool that can be run/outputs to the console.

Should it be integrated? Or is there a better place for the results of those tools?

Answer 1

The tools you list have nothing to do with code reviews. They are there exactly to let the reviewers to focus on more useful subjects.

Code reviews are for important things which cannot be automated: using a design pattern when another one matches better, over-documenting clear code or under-documenting unclear one, not using language features which can make code shorter and more readable. All those things are “too smart” for a machine: a program, no matter how sophisticated is it, won't be able to tell whether your application of abstract factory pattern is a good idea in a given context or not. That's why code reviews are done by persons, often more experienced developers.

Subjects mentioned during code reviews are also often subjective. One developer will tell that this specific method should be split in two. Another equally experienced one will consider that the method is fine and does exactly one thing. One reviewer will find a goto statement terrible; his colleague may find that in this particular case, goto is fine and can't be removed without leading to less readable code.

Style tools, on the other hand, are the archetype of automation of purely objective, basic, even sometimes dumb rules. Line 273 is too long. You've forgotten a capital letter. And here, there is a space missing. And here, I've found two empty lines instead of one. Boooring.

Don't waste anyone's time for that during code reviews. If there is a style guide in your project, use style checker (like pep8) but run it automatically through pre-commit hook. Space missing? No commit for you, sir. An empty line contains whitespace? Please remove it if you want your code in version control.

In fact:

• Since the rules are objective and straightforward, there is nothing to discuss during a code review. There is whitespace in an empty line. So remove it. What would you like to tell about it during a code review?

• Worse, it can lead to discussions which are all but constructive. For some, code reviews would become an opportunity to discuss the merits of their preferred style, because, frankly, everyone knows that two spaces are better than four, and now, the code review is focused on “Should we change our code style and rewrite our entire code base to use two spaces instead of four”. Avoid this at all costs.

The remaining—the stuff which is too complicated for a machine—is for code reviews, letting the reviewers to focus on things that actually matter.

Automated checking, would it be style checking, compiling, tests, static analysis or formal proof, should be, well, automated. Where should it be in continuous integration process is a different subject. Short and fast checks, such as style checking, can be put directly in a pre-commit hook to avoid any non-compliant code in the code base. Long checks, such as static analysis, will usually be done during the build.

Remember, if you have a subject which:

1. Doesn't have a straight answer, the answer being based on personal experience and educated guesses,
2. Results in non-binary "well, it can be A, but B can be accepted here, while C seems a bit problematic, but will be an excellent solution if this were that"-style answers,
3. Cannot possibly be automated even with most sophisticated applications,
4. Cannot easily be ported to another context,
5. Is focused on code quality, readability, expressiveness and maintainability,

code reviews may be a good place for it. On the other hand, if the subject:

1. Is purely objective and without ambiguity,
2. Results in a binary answer, often based on a metric,
3. Can be automated (sometimes requiring a lot of work and research),
4. Applies to every situation in the same group,
5. Is focused on code quality, readability, expressiveness and maintainability,

pre-commit hooks or the build are much better places than a code review.

Further reading:

• How to set up something like an integration server that measures the quality of code and reject the code if the score is below a certain number?

• Linters rock, but they are slow

Answer 2

This answer is based on two assumptions. First, you are using a static analysis tool that is more than a style checker (PMD and/or FindBugs instead of Checkstyle, for example). Second, the tool has been properly configured with regards to the errors detected and the severity of the error for the given process or product.

Yes, static analysis tools that are designed to find errors should be part of the code review process. More generally, a static analysis tool should be run and the results reviewed as part of a code review, before committing code into a branch that other developers use.

Prior to code review, the developer should execute and correct issues found by the static analysis tool, with any findings reviewed and any important findings corrected. Then, they should be rerun over the code to produce remaining errors. This list should be part of the code review. This will allow the human code reviewers to focus on issues that they can't find, or perhaps pay special attention to areas of the code flagged by the tool (double checking false positives).

Ideally, you have been using the tool since the inception of the project. If not, however, and there are a large number of findings, these would need to be reviewed and dispositioned, which would not be done as part of a code review. At this point, I would make the execution of the tool part of the build or continuous integration process and each build should reduce the number of errors. The developers should use this information to help leave the code they touch in a better state than they found it during refactoring or development. Of course, using the list during a code review would also be useful to identify potential problems that should be corrected to ensure the code is better than it was before.