On Jul 10, 2012, at 11:09 AM, Vincent Massol wrote: > > On Jul 10, 2012, at 10:56 AM, Denis Gervalle wrote: > >> On Tue, Jul 10, 2012 at 8:08 AM, Marius Dumitru Florea < >> [email protected]> wrote: >> >>> 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. >>> >> >> Well, it seems you are thinking about this value of 20 as the perfect >> threshold to detect bad design. Could you explain why 20 is better than 25, >> and why it is not worse than 15 ? > > FYI the value 20 comes from checkstyle and I don't know why they picked this > value but I'm definitely sure they've had countless discussions about it ;) > > But that's beyond the point :) The point is: we have a value. We suggest to > change it. We need to prove it's better. I'm not against changing it. I'm > against changing it at random (which btw is exactly your point :)). > >> If Sergiu had proposed to double it, I >> would have been against of course. I think I may say that those voting +1 >> here has experienced a minor excess while their code design where not so >> bad or may not be really better. I remember removing a call to a utility >> function from apache-common, just to lower it, and this has finally nothing >> to do with improving my code design. > > Yes and your solution was probably bad (to remove a utility function). That's > not the way to fix a class fan out IMO. > >> Do you think this single line calling >> a utility function, is a good measure of my code design ? > >> Here, all Sergui >> is just proposing is to not account for the most usual imports we always do >> in almost any components. > > Which is exactly the same as saying we don't care about class fan out. Those > imports are as important as any import. > > Again, there's no point discussing in the void; we can argue either way > indefinitely. > > We need a real world example and then we can have a very interesting and > useful discussion on design.
BTW I would prefer to say that we don't care about class fan out and remove it from the checkstyle check rather than change arbitrarily its value. My only concern if we exclude this check is whether this will have an impact in our code and what other solution we have to ensure we keep having small classes (which means good OO design). Well, we do have this limit on # of lines in a class which is also a way to ensure we write OO code. Right now the FileLength check we have is 2000 lines which is too long IMO. I think a good class shouldn't be longer than around 500-1000. 500 is best and 1000 means refactoring is needed. But class fan out brings more to the table and is also a good way to ensure our unit test fixtures remain small since we don't have a check on the size of our unit test fixtures. That would be a good way to know the design is good. ATM I feel that class fan out is a good metric. Thanks -Vincent _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
