10 Sep
2012
10 Sep
'12
7:48 p.m.
On 09/10/2012 07:23 PM, Vincent Massol wrote:
Hi Jerome,
Hey I didn't really want to awaken this topic again ;) I already gave my opinion on
the other thread (I still need to reply to Sergiu's last mail BTW). I just wanted to
explain how checkstyle computes the class fan out and share that since it was useful for
me to fix one class fan out issue I had.
See below.
On Sep 10, 2012, at 7:06 PM, Jerome Velociter <jerome(a)velociter.fr> wrote:
Yes, this is a way better approach than trying to rationalize said
random numbers. That was exactly my point actually.
Jerome
On 09/10/2012 06:17 PM, Vincent Massol wrote:
Would be interesting to ask the checkstyle guy why they picked this value. My guess is
that they used something like 7 + standard java classes.
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)
So this
shows "20" was magic to begin with. How do you go from 7 +/-2 to 20 ? I find it a bit specious to apply the theories
exposed in the article to a program's class external classes without a pinch of salt.
It would be saying their is a good match between the notion of "object" as
described in the experiments (so in the sense of an item of input, like a word in a list
of words or a sound in a list of distinct sounds) and the notion of a class of objects in
programming.
Not my choice. All references to Class Fan Out say that the default
value should be around 7 and quote this article.
What I'm trying to say is that maybe we
should rather accept the fact 20 is magic to begin with and try and figure out if this
works for us and tweak it if necessary
I've already answered to this. I'm
-1 to change what we have in a random manner with another random value. What I said is to
show me some code and we can discuss on this basis. Sergiu has pointed out some code but I didn't
get the time to look at it yet. I need to do that and if the outcome is that we can't
make it pass with a good design then we can discuss either make an exclude for this
special case or decide that the case is generic.
--
Peace,
—Jerome
- rather than trying to rationalize it with
bridges to psychology thesis that are far from obvious (not that the thesis don't
apply to programming ; just that the impendance of an "input" or "unit of
thought" is IMHO not so easy to match with programming concepts without making blind
guesses). Maybe I'm wrong, it's just I find this kind of rationalization somehow
naïve.
Hope that made sense.
Well, there's not much point in saying that something is
wrong if you don't provide an alternative… If you're going against the established
standard then you better be an expert in the domain. I know I'm not. But even if we
agree that the study has no meaning, it doesn't change the fact that to change what we
have we need to have a good reason to do so and we need to define the new value while
still ensuring that the check still makes sense.
Also, a note about ignoring classes that
"don't require effort to understand" (or to look at their source code), they
would probably span more than "standard Java classes" like Collections, no ?
Yep, I mentioned that in my mail (I said "Collection" classes).
For example, what about common-langs
(StringUtils, etc.) ?
I wouldn't put those, not everyone know them and again
static method calls **don't count** for the class fan out …
Anyway ATM I don't agree about starting to ignore classes without at least one good
example where we have to do so to make the fan out check pass.
I write new code almost everyday and it's seldom that I have issues with class fan
out and most of the time when it happens it's because I've been lazy and I can fix
the issue in a few minutes. This makes me think that the current value is good. As
I've already said in the other thread for me it would be much better to agree to do a
checkstyle exclude of *only* the ClassFanOut check for a given class rather than change
the default for everyone.
Thanks
-Vincent
Jerome
> 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 <vincent(a)massol.net> 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");
>>
>> * 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
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs