On Jul 9, 2012, at 5:36 PM, Sergiu Dumitriu wrote:
Hi devs,
Short version:
I'd like to increase the allowed maximum value for the Class Fan-Out Complexity
checkstyle rule from 20 to 25, since a bare component already uses 1 to 4 imports, and the
20 rule was set before we had components.
Long version:
The Class Fan-Out Complexity metric measures how many other classes are used by a class.
Keeping this to a small value is a good goal, since it favors loose-coupling, small and
independent classes, and makes it harder to put more than one responsibility in a single
class.
However, there are several problems:
One is that this counts utility classes and interfaces, such as java.util.List, and a
fairly complex class uses more than one such utility class; they usually come in pairs
(the interface and the implementation). And more and more classes are using apache-commons
utilities like StringUtils or IOUtils, which are just shortcut helper methods that could
be implemented in a few lines of code, so we're trading one import for reduced code
complexity, which is a good thing, even though apparently it's increasing the Fan-Out
measure.
Another is that a bare Initializable component implementation will import:
- its component role interface
- Initializable and InitializationException
- Logger
And since good libraries also follow the "many small classes" paradigm, barely
using some of our dependencies will add a lot of imports. For example, the LucenePlugin
has 25 org.apache.lucene.* imports just for initializing the server and sending search
requests.
While the current maximum, 20, is enough for the majority of our classes and interfaces,
I've found myself often enough trying to refactor a class to get down from 23 or even
21 imports, and usually I find myself doing ugly tricks, keeping the actual code
complexity the same or even worse. Best case is that I extract an extra class that just
contains the non-public utility methods that were initially in the component
implementation, but that class is meaningless out of the context of its parent class, so
that is also a bad decision, IMHO.
So, I propose to increase the Fan-Out limit to 25, while keeping the requirement that
classes should be kept as small as possible.
I'd like to see first an example of a class that goes beyond the recommended value of
20 and then decide if the problem is in the class design or in this parameter threshold.
Re LucenePlugin; I hope you're not considering it as an example of good design… :)
The fact that we're using components has nothing to do with this. Whether you use
components or not is exactly the same. Quite the opposite actually since components favor
decoupling and thus reduces class fan out.
BTW if you find your code using too many components injected it's a very bad sign. You
have a design issue. Just write some unit tests using AbstractMockingComponentTestCase and
you'll understand you have a problem because you'll start needing to mock way too
many things.
When you find yourself doing ugly tricks to reduce the class fan out it's because
you're not thinking at the level of design. The problem has more to do with the fact
that you're usually working on fixing something and suddenly you execute the build and
you don't feel like redesigning the code just because of a class fan out issue and
thus we usually end up adding technical debt by removing it from the checkstyle check. IMO
this is the part that we need to fix but not the threshold.
For me this checkstyle check is probably one of the best we have. Most of the others are
syntactic sugar and are not important whereas this one is really about design. Of course
since it's about design it's also harder to fix but that doesn't remove its
value. Having a good design has always been hard. And not having a good design generates
technical debt.
Let's see the code first before doing something arbitrary.
I'm -1 ATM because I'd rather not rush blindly. Personally I've never had any
real issue with this class fan out except for old code I didn't write and for which I
brought some quick bugfix and I didn't want to spend the time to refactor it. That
doesn't make the rule bad.
Thanks
-Vincent