CRAP is not all forms of crap
Jason Gorman, posted a critique on his blog about some code in the Crap4j codebase. He mentions one particularly long method, SystemCrapStats.toXml(). It was indeed quite a long method that was responsible for persisting the statistics gathered into the report.xml file. He asked the question:
Can I take it that Crap4J doesn’t test for long methods, then? Is this further evidence to support the theory that you get what you measure (or, in this case, you don’t get what you don’t measure)?
I answer, “Certainly.” Alberto expounded on our aims in his original blog posts over at Artima, but some of that bears repeating.
First, though, I would like to say that I fixed this method :-). It already delegated the project specific information and the method crap scores to their respective object’s toXml methods, but it was still doing quite a bit in this method. The new method is a composition of methods, that I think is much more comprehensible. It could certainly be even better, but we have other fish to fry and it is good enough. It looks like this:
public String toXml() {
MyStringBuilder s = new MyStringBuilder();
s.start("“);
crapProject.toXml(s);
writeStats(s);
writeMethods(s);
s.end(” “);
return s.toString();
}
Back to our goal with Crap4j and a discussion of Jason’s question. We were looking for an unequivocal characterization of crappy code. That is, if the tool said the project is CRAP, then it had better be crappy. We believe, based on decades of experience, a lot of physical examination of code and interviewing of developers, and study and reasoning over piles of research papers that two simple measures can be pretty successful at indicating a whole class of crappy projects.
One of the properties is Cyclomatic complexity. Really complicated methods are hard to understand, thus hard to implement correctly, and probably more importantly hard to maintain and modify correctly.
The second property is the existence of automated unit tests. Tests serve to verify correctness and to catch regressions that might occur in maintenance. Without tests, the developer is left to reason through each change, and all of its implications for the rest of the code base. Tests extend the developer’s attention in a mechanical way and they show (when done reasonably well) a certain attention to correctness initially that may generally be relied on to increase confidence in the code.
Nowhere do we look at long methods. However, long methods can indeed be problematic. A straight-line method may still be hard to understand by virtue of the numerous responsibilities it has. If the definition and assignment of variables happens far from where they are used, that creates more opportunities for bugs by virtue of being more things that must be kept in the developer’s head while they work in that method. But, some long straight-line methods can arguably be pretty simple to understand. So, it is not as convincingly crappy as methods with many paths and no tests.
I would say that right after reducing the CRAPpy method count to a tolerable level, then by all means, pursue these other aspects of code quality. Look at things like long methods or the Chidamber Kemerer metrics such as cohesion and coupling. However, keep in mind that they are much more subjective due to their contextual interpretation. We don’t want CRAP to be subjective. If it says it is crappy, then everyone should agree.
That said, we expect to evolve the metric as everyone gets more experience with it. So, here is a question. Has anyone found disagreement with a particular method that Crap4j has identified as crappy? We would really appreciate knowing about it.
