[xwiki-devs] [VOTE] Change the Class Fan-Out Complexity maximum value in our checkstyle configuration
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. -- Sergiu Dumitriu http://purl.org/net/sergiu/
+1 On Mon, Jul 9, 2012 at 5:36 PM, Sergiu Dumitriu <[email protected]> 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. -- Sergiu Dumitriu http://purl.org/net/sergiu/
______________________________**_________________ devs mailing list [email protected] http://lists.xwiki.org/**mailman/listinfo/devs<http://lists.xwiki.org/mailman/listinfo/devs>
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
+1 I've seen many of the same problems, unfortunately strict rules and automation can't always tell good designs from bad. Caleb On 07/09/2012 11:36 AM, 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.
+1 On Mon, Jul 9, 2012 at 5:36 PM, Sergiu Dumitriu <[email protected]> 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. -- Sergiu Dumitriu http://purl.org/net/sergiu/
______________________________**_________________ devs mailing list [email protected] http://lists.xwiki.org/**mailman/listinfo/devs<http://lists.xwiki.org/mailman/listinfo/devs>
-- Jérôme Velociter Winesquare http://www.winesquare.net/
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
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
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 ? 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. 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.
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
-- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
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. Thanks -Vincent
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
On Jul 10, 2012, at 11:18 AM, Vincent Massol wrote:
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.
IMO the main use case we need to fix is this one: * Someone fixes a bug in a class that he hasn't coded and suddenly the fan out is beyond the threshold. Ideally the code should be refactored but the person may not have the time to do so at this point and it's better to have a bug fixed than waiting for the refactoring before fixing the bug. Even though this introduces a technical debt (possibly caused by a bad design of whoever coded the code in the first place), we might want to allow to exclude from checkstyle this class for this check only (this is done using checkstyle-suppressions.xml, see the tools-standard-validator module which has such a file). Ideally a comment in this file explaining the reason for the exclude and any idea on how to fix the check, would be a good thing. WDYT? Sergiu, would that solve the main issue you have? Thanks -Vincent
On 07/10/2012 04: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.
While searching to find out which classes are counted by the CFOC check, my "social" Google put this on the fourth position: https://twitter.com/vmassol/status/2371261254 Can you explain me where is the bad design decision in there? I remember now that writing rendering macros is where I had the most problems myself, and the bad design wasn't in my code, it was in the rendering module which defines a new class for every little piece of data. I see 17 rendering imports in your class. And the way I fix this kind of problem is to change a few exact classes with a common superclass that still has the right methods that I need. Any well designed data library will have a verbose set of classes modeling the data, and any moderate usage of that library is going to import many of them.
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 ? 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. 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.
Actually, this is an interesting subject. The real problem is that the CFOC is supposed to measure how connected a class is, but some utility classes IMHO are just as natural as those in the java.lang package, and those aren't counted. So a better solution would be to change the CFOC check so that it ignores some of our standard classes. Is using StringUtils really an extra point for bad design? Is logging a bad design? Is making a component Initializable worse than making it Serializable? Is working with a Map worse than working with Numbers? Unfortunately, I don't think that's easy to do unless we provide our own implementation of CFOC. -- Sergiu Dumitriu http://purl.org/net/sergiu/
participants (6)
-
Caleb James DeLisle -
Denis Gervalle -
Jerome Velociter -
Marius Dumitru Florea -
Sergiu Dumitriu -
Vincent Massol