Code Reviews with CI

Question

What sorts of things do you look for in a code review when using continuous integration? It seems like a lot of the literature around for code reviews mentions things like coding style, spelling mistakes, resource usage, error handling, etc. (perhaps not in that order :-) However tools like FxCop and StyleCop seem to pick up a most of the trivial problems.

I would think that as many checks as possible should be pushed into CI to save time and ensure that those checks are done consistently. If you take this approach it seems like the main things to look for in a review are things like poor design, failing to use the existing code structure properly, subtle defects that may been missed in tests, etc

How does everyone else do reviews with CI in place? What other things do you look for in a review?

Answer 1

• Security issues
• Performance issues
• Usability for GUIs
• Scability
• Good commenting

Pretty much, whatever can't be automatically tested by a machine (except perhaps for performance/security issues, but they're a lot easier for humans to detect.)

Answer 2

In my experience code reviews are more about ensuring things have been done in the best way possible. Usually the outcome of a review will be some refactoring. If the code is a bit dodgy it might need to other things fixed up. Products like FxCop and ReSharper can help a lot with code quality.

Answer 3

A key aspect of a code review is to ensure that all of the horses are pulling in the same direction: i.e. that the code is consistent with the team philosophy. There are any number of small variations and debates where Option A or Option B are reasonable but should not really be mixed. e.g. unit testing getters/setters for 100% code coverage, or using constructor versus setter initialization for Spring. We can debate these, but once settled, a team should really stick to the decision for consistency.

Because the choices are reasonable (or vague), it is hard to test for violations in CI. It is much easier to get a sense of "peer pressure" via reviews.

I've blogged on this in the past, with respect to some specifics: code coverage, internationalization, and versioning of data. These are just some issues where coherence to a team philosophy is important.

Answer 4

Code readability and future maintainability is a huge problem that gets missed out from automated solutions. Undocumented side effects and requirements on methods increase dramatically. Methods called changeX() don't change X and methods called getFoo() return the value of foo and also secretly add 7 to the internal state of the object.

Answer 5

That the code is delivering the best business value possible. Additionally code reviews aren't just for the code that has been written, it's also to help shape the code that will be written in the future. With the tools that are available these days much of what we used to look for is already (at-least mostly) taken care of by the time code is committed to source control. At-least that's how I think it should be done. To elaborate here's a snippet of something I wrote before on this subject.

".Net code review tools have been improving over recent years and removing some of the traditional necessity of code review, but not the most valuable aspect. There are static analysis tools like FxCop to automatically review for all the code-deterministic industry standard guidance. There are source analysis tools like Microsoft’s recently released Source Analysis (a.k.a. StyleCop) to review all the source code style choices made. With Continuous Integration and TDD code is reviewed to make sure it builds and meets the expected business scenarios. Products like NDepend have been recently introduced to review code and provide statistical information to help determine if designs are following principles that lead to sustainability. All of these types of tools augment the ecosystem in which we review our software. However, none of them provide any help in one of the most critical aspects of code review - the answer to the question “does this code deliver the best value for the business domain?” Just because code passes FxCop, StyleCop, looks good in NDepend and passes automated tests doesn’t mean it provides the best business value. Those tools are part of a domainless code review ecosystem, they don’t have any insight into any of the businesses we create software for or how well our code serves that domain. The answer to the “best value” question is in some ways subjective, but the most capable people to provide the answer are the coders in that domain using the tacit technical and business knowledge that only they retain. Code reviews are the way to apply and share that tacit knowledge and find the answer to the “best value” question." - TeamReview - New Business Value From Code Review