Software Quality Award comment for Checkstyle

ATTENTION: non of following should be treated as appellation or demand to reconsider review of Checkstyle project in Award. This post is targeted only to contributors who asking why Checkstyle missed so obvious points.

http://www.yegor256.com/award.html

> 83K Java LoC, 553K HoC

suseika/inflectible (5K LoC, 36K HoC) - winner
testinfected/molecule     (10K LoC, 43K HoC)
coala-analyzer/coala      (14K LoC, 160K HoC)
xvik/guice-persist-orient (17K LoC, 54K HoC)
raphw/byte-buddy      (84K LoC, 503K HoC)
citiususc/hipster   (5K LoC, 64K HoC)
checkstyle/checkstyle (83K LoC, 553K HoC)
kaitoy/pcap4j         (42K LoC, 122K HoC)

we are second biggest project in Final. But we are the oldest projects, we are almost 15 years old. That point will explain some design problems below.

> There are many ER-ending classes, like SeverityLevelCounter, Filter, and AbstractLoader (for example), which are anti-patterns.

That is controversial anti-pattern and I will explain why there is no damage this in separate post. No comments for this point, it is just philosophy of Award owner. 

> There is a whole bunch of utility classes, which are definitely a bad thing in OOP. They are even groupped into a special utils package, such a terrible idea.

Utility is good as it is stateless realization of algorithms. Mismatch of design philosophy between Author and us.

> Setters and getters are everywhere, together with immutable classes, which really are not an OOP thing, for example DetectorOptions.

Yes, that is effect of 15 years old project and numerous existing Checkstyle's integration and extensions that already exists in this world. We can not changes this - it will be huge compatibility damage. Nobody will benefit from such update. But we will already in progress to make Checkstyle more immutable.

Attention: getter/setter is very controversial anti-pattern (mutability is a concern, but there is not problem in naming), I will explain this in separate post.

> NULL is actively used, in many places — it's a serious anti-pattern

Yes, that problems comes from core Checkstyle - ANTLR2 parser.

> I've found five .java files with over 1000 lines in each of them, for example 2500+ in ParseTreeBuilder.java

Yes, and we do this on good reason. First of all https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/grammars/javadoc/ParseTreeBuilder.java is in test ares, secondly it is generated :).

There are exceptions in rule for big files:

1) Some files are not possible to slit - https://github.com/checkstyle/checkstyle/blob/master/config/suppressions.xml#L8

2) UTs are also in exclusion, as it is easier to find from class MyCheck.java --> MyCheckTest.java and not puzzle what  MyCheckSecondTest.java  mean (and how author decide when test should go second test file). So it is OK.

> There are direct commits to master made by different contributors and some of them are not linked back to any tickets. It's impossible to understand why they were made. Look at this for example: 7c50922. Was there a discussion involved? Who made a decision? Not clear at all.

Such commit is mine :). But we do have strict control of commit message, we are even more fanatic than all other projects. Whole idea is described - https://github.com/checkstyle/checkstyle/wiki/Release-notes-automation . There is no problem with direct commits, especially from the most experienced owners of it (see all details in wiki link).

> Releases are not documented at all.

It is first time I see that somebody pay attention to https://github.com/checkstyle/checkstyle/releases , we do have Release Notes in human friendly way on our main HTML site - http://checkstyle.sourceforge.net/releasenotes.html  (it is not a list of commits !!!! users do not need commits!!! )

> Release procedure is not automated. At least I didn't find any release script in the repository.

It is automated and done by standard maven procedure "mvn release" no need for special shell file and we are multi-OS project (Linux, Windows, MacOS). 

Release is not a binaries copy to artifact repository!  Here is detailed instructions on how to make a release - https://github.com/checkstyle/checkstyle/wiki/How-to-make-a-release with all details of how systems should be prepared and what should be updated after version bump. But I agree, I would be happy to automate it. For now it does not make sense to spend time on difficult automation, if we release ones a month.

Summary:

some quotes from Award page:

I'm a big fan of object-oriented programming in its purest form
....

Strict and visible principles of design.

We participated in Pure OOP contest. I do not share fanatic following of pure OOP designs, I am more in favor of changes to be more functional. Any re-factoring in favor of OOP is not possible without braking compatibility with plugins/extensions.

So 7th place is very good.