[xwiki-devs] [VOTE] Change the Class Fan-Out Complexity maximum value in our checkstyle configuration
9 Jul
2012
9 Jul
'12
5:36 p.m.
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/
9 Jul
9 Jul
7:22 p.m.
+1
On Mon, Jul 9, 2012 at 5:36 PM, Sergiu Dumitriu <sergiu(a)xwiki.com> 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
devs(a)xwiki.org
http://lists.xwiki.org/**mailman/listinfo/devs<http://lists.xwiki.org/ma…
--
Denis Gervalle
SOFTEC sa - CEO
eGuilde sarl - CTO
7:26 p.m.
+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.
7:39 p.m.
+1
On Mon, Jul 9, 2012 at 5:36 PM, Sergiu Dumitriu <sergiu(a)xwiki.com> 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
devs(a)xwiki.org
http://lists.xwiki.org/**mailman/listinfo/devs<http://lists.xwiki.org/ma…
--
Jérôme Velociter
Winesquare
http://www.winesquare.net/
7:59 p.m.
New subject: [xwiki-devs] [VOTE] Change the Class Fan-Out Complexity maximum value in our checkstyle configuration
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
10 Jul
10 Jul
8:08 a.m.
On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol <vincent(a)massol.net> wrote:
On Jul 9, 2012, at 5:36 PM, Sergiu Dumitriu wrote:
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
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
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
10:56 a.m.
On Tue, Jul 10, 2012 at 8:08 AM, Marius Dumitru Florea <
mariusdumitru.florea(a)xwiki.com> wrote:
On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol
<vincent(a)massol.net> wrote:
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.
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
--
Denis Gervalle
SOFTEC sa - CEO
eGuilde sarl - CTO
Thanks
-Vincent
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
11:09 a.m.
New subject: [xwiki-devs] [VOTE] Change the Class Fan-Out Complexity maximum value in our checkstyle configuration
On Jul 10, 2012, at 10:56 AM, Denis Gervalle wrote:
On Tue, Jul 10, 2012 at 8:08 AM, Marius Dumitru Florea
<
mariusdumitru.florea(a)xwiki.com> wrote:
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 :)).
On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol
<vincent(a)massol.net> wrote:
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 ?
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.
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
11:18 a.m.
New subject: [xwiki-devs] [VOTE] Change the Class Fan-Out Complexity maximum value in our checkstyle configuration
On Jul 10, 2012, at 11:09 AM, Vincent Massol wrote:
On Jul 10, 2012, at 10:56 AM, Denis Gervalle wrote:
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 Tue, Jul 10, 2012 at 8:08 AM, Marius Dumitru
Florea <
mariusdumitru.florea(a)xwiki.com> wrote:
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 :)).
On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol
<vincent(a)massol.net> wrote:
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 ?
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.
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.
12:21 p.m.
New subject: [xwiki-devs] [VOTE] Change the Class Fan-Out Complexity maximum value in our checkstyle configuration
On Jul 10, 2012, at 11:18 AM, Vincent Massol wrote:
On Jul 10, 2012, at 11:09 AM, Vincent Massol wrote:
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 Jul 10, 2012, at 10:56 AM, Denis Gervalle wrote:
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. On Tue, Jul 10, 2012 at 8:08 AM, Marius Dumitru
Florea <
mariusdumitru.florea(a)xwiki.com> wrote:
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 :)).
On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol
<vincent(a)massol.net> 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.
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.
2:55 p.m.
On 07/10/2012 04:56 AM, Denis Gervalle wrote:
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/
On Tue, Jul 10, 2012 at 8:08 AM, Marius Dumitru Florea
<
mariusdumitru.florea(a)xwiki.com> wrote:
> On Mon, Jul 9, 2012 at 8:59 PM, Vincent Massol <vincent(a)massol.net> 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.
4451
days inactive
4452
days old
10 comments
6 participants
participants (6)
-
Caleb James DeLisle -
Denis Gervalle -
Jerome Velociter -
Marius Dumitru Florea -
Sergiu Dumitriu -
Vincent Massol