On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol <[email protected]> wrote: > > 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.
I agree with Vincent. I've hit the limit myself a few times but I'm sure it was due to bad code design. I'm -0. Thanks, Marius > > Thanks > -Vincent > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
