On 09/10/2012 12:17 PM, Vincent Massol wrote:
With this finding writing a macro requires between 5 to 6 Classes:
* DefaultContentDescriptor (only if there's content)
* XXXMacroParameters
* List
* Block
* MacroTransformationContext
* MacroExecutionException
Our checkstyle config fails above 20 which means Macro classes have about 14
additional deps on external classes allowed before they break. Which should
normally be more than enough.
My understanding is that reading the code is hard when there are too many
references to external classes (incidentally it also makes the code more
brittle as there are more chances it'll break due to an issue with the used
classes). Apparently 7 (+/- 2) is the magic number (see
http://en.wikipedia.org/wiki/The_Magical_Number_Seven,_Plus_or_Minus_Two).
That magical number doesn't really work well for software, IMO.
First, does everybody agree that a Block is that much different from
List<Block> to take up two slots? Same question for SectionBlock versus
GroupBlock, or BulletedListBlock versus NumberedListBlock. Yes, they are
all different, but I don't think that they really make it so hard for a
good software developer to work with several types of blocks, so that
each one is considered a different entity that must be counted. I'd
rather count types of classes than classes directly.
Then rarely does one need to understand the whole class and nothing but
the class. I need to keep in memory a code path at a time, and that
could go through several classes, but rarely through an entire class at
a time. So if we have to limit the number of different "entities" that
interact at a point, the class is not the best unit to investigate.
Third, good software developers are good especially because their brains
cope well with abstract thinking, and can hold entire algorithms in
memory. 7 is the average number observed for average people that don't
necessarily work in software development. I'd be interested in some
similar statistics for software developers.
I'm not arguing that we shouldn't limit the CFOC, I'm just saying that
we shouldn't bring the wrong arguments for a decision (don't use the
Chewbacca defense).
A kind of conclusion is that it's a real pain to work with XDOM.
So IMO we have 2 options that could make sense (ie we can rationalize them):
* Keep this default value of 20 total which includes some standard JDK classes.
14 deps for Macro should be enough, even if we count, say 2-4 more for standard
Java classes like Collection classes.That still gives us about 10 deps for
additional XWiki classes.
* Decide to exclude some classes that are basically part of the Java language
(such as Collection classes) since they don't increase the reading complexity
of the code since everyone knows what they do and we don't need to look their
source code or doc to understand them. However, if we do so then we should
reduce the allowed Fan out to 7 (+/- 2) so let's say to 9. So that gives us
only 9 deps for XWiki classes, compared to about 15 ATM...
IMO the second option is much harder for us than the first one and that's why
I'd keep the first option...
BTW the reason I wrote about all this is because I made a change to the
ChartMacro and the fan out became 21… In the end I refactored the code and got
a slightly better design (and fixed a bug at the same time…).
Thanks
-Vincent
On Sep 10, 2012, at 4:59 PM, Vincent Massol <[email protected]> wrote:
Hi devs,
I wanted to understand how Checkstyle computes the Class Fan out so I debugged
it.
Here are my findings:
* Some classes are excluded by default:
mIgnoredClassNames.add("boolean");
mIgnoredClassNames.add("byte");
mIgnoredClassNames.add("char");
mIgnoredClassNames.add("double");
mIgnoredClassNames.add("float");
mIgnoredClassNames.add("int");
mIgnoredClassNames.add("long");
mIgnoredClassNames.add("short");
mIgnoredClassNames.add("void");
mIgnoredClassNames.add("Boolean");
mIgnoredClassNames.add("Byte");
mIgnoredClassNames.add("Character");
mIgnoredClassNames.add("Double");
mIgnoredClassNames.add("Float");
mIgnoredClassNames.add("Integer");
mIgnoredClassNames.add("Long");
mIgnoredClassNames.add("Object");
mIgnoredClassNames.add("Short");
mIgnoredClassNames.add("String");
mIgnoredClassNames.add("StringBuffer");
mIgnoredClassNames.add("Void");
mIgnoredClassNames.add("ArrayIndexOutOfBoundsException");
mIgnoredClassNames.add("Exception");
mIgnoredClassNames.add("RuntimeException");
mIgnoredClassNames.add("IllegalArgumentException");
mIgnoredClassNames.add("IllegalStateException");
mIgnoredClassNames.add("IndexOutOfBoundsException");
mIgnoredClassNames.add("NullPointerException");
mIgnoredClassNames.add("Throwable");
mIgnoredClassNames.add("SecurityException");
mIgnoredClassNames.add("UnsupportedOperationException");
The fact that so many exceptions are excluded by default makes me think
that most exceptions could be safely ignored. So I think that a better
rule would be "ignore descendants of Throwable" than to list some
classes. So here two different code quality aspects collide: making an
API that's clear about its failure cases, and keeping a low count on the
number of imported classes.
An exception to this would be badly designed code that uses exceptions
as normal code flow, but we should stay away from such code anyway.
* All classes in java.lang.* are excluded too
* Annotation classes are not counted
* Classes in the same package are counted (they won't appear in import since
it's in the same package so don't count imports to get class fan out)
* Static method calls are not counted. So for example StringUtils from Commons
Lang never counts for class Fan out
* Enums are not counted (no new XXX() done. That's why static method calls are
not counted too BTW)
* Classes used in class extend or implement are not counted too.
Hope it helps
-Vincent
--
Sergiu Dumitriu
http://purl.org/net/sergiu/
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
