ClassFanOutComplexity metric

Development
#1

Hi everyone,

so I’m opening this as a brainstorming topic on a maybe sensitive subject which is the ClassFanOutComplexity metric that we use with Checkstyle. Right now the CFOC threshold is 20 and I’m wondering if:

  1. we should keep this metric
  2. we should keep this threshold

I’m starting this discussion because that’s the checkstyle issue I hit the most often when I develop new stuff in XWiki, and the solution to fix those are generally not obvious to avoid adding meaningless classes just there to pass the checkstyle. Another reason is that I’ve just seen this issue: https://jira.xwiki.org/browse/XCOMMONS-2294 which appears related to a fix of checkstyle which was badly counting the CFOC. When they will solve it we might discover new issues in our existing code.
Finally a last reason, is that I googled “how to reduce class fan out complexity” (don’t judge me :)) and I surprisingly found first this XWiki markmail archive: http://markmail.org/message/snun6avp3qlkg54k

So clearly not the first time the subject is on the table, but just to add some information, after reading it I checked and apparently Checkstyle itself doesn’t use a threshold of 20 for CFOC, they use 25: https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle_checks.xml#L647

So I’m not proposing right now that we increase our value, even if I’d like it, just wondering what do you think about this metric nowadays? I know that on my side I really don’t like it and I don’t find it very useful personally. We could check but I think most of the checkstyle problems we ignore are actually related to this one.

So, sorry for this hot topic, hope it won’t become a flamewar but would be interesting to know your opinion.

Thanks.

#2

Reminder:
Class fan-out complexity : The number of other classes a given class relies on.

Note that we already covered this topic in the past:

My POV is that 20 is a good value and we shouldn’t change it. Above that would most likely means that the design is not good and too much different domains are put in the same class. That’s the reason these errors are hard to fix; they require hard design thinking.

What we did in the past and what we could check/review is to add some known classes that don’t increase the mental complexity because they are well known base classes (such as the java collections), if we find some that are not already in our ignore list (we already have quite a lot but it’s possible we missed some: https://github.com/xwiki/xwiki-commons/blob/05afe1a2f9b07784c3d4e136a1ea5b4bfc27d075/xwiki-commons-tools/xwiki-commons-tool-verification-resources/src/main/resources/checkstyle.xml#L109-L120 ).

I still agree with what I said in the past: let’s see some code that fails and brainstorm about how this code should be written for the best readability/design.

Personally I don’t fall on CFOC often. Happens from time to time. Sometimes it’s a bit painful and I have to rack my brain to find a solution and sometimes I even don’t find a good solution but most of the time the resulting code is simpler to read.

#3

Hi everyone,

I’m reviving this brainstorming: this metric is still often bothering me, and I’m seeing that it’s generally the first reason why we insert checkstyle exceptions in the project, see:

I still think that increasing this value a little (e.g. using checkstyle’s default) could help to avoid introducing those exception. We already added a bit more class to ignore in it (with Allowing more Excluded Classes for ClassFanOutComplexity) but not sure it’s enough: I have in particular the problem with some APIs like everything’s related to rendering and XWiki’s email API which involve a lot of different classes.

#4

Make sense, +1.

#5

Default is indicated to be 20, see checkstyle – ClassFanOutComplexity

Checking code source now.

#6

I don’t know if this file is the one used or if it’s just an example but it’s indeed using 25: https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle-checks.xml#L721

I guess it’s easy to try locally, will do.

EDIT: In any case, I’m fine to use the default value.

#7

errr we’re already using the default it seems: https://github.com/xwiki/xwiki-commons/blob/92cf353c08397ed6419b729f8a28655714829259/xwiki-commons-tools/xwiki-commons-tool-verification-resources/src/main/resources/checkstyle.xml#L103-L125

#8

Hmm so it’s actually the value used for the checkstyle project itself it seems. The value was provided as part of Fix violations reported by Checkstyle checks · Issue #1566 · checkstyle/checkstyle · GitHub