search

vavr-io / vavr

vʌvr (formerly called Javaslang) is a non-commercial, non-profit object-functional library that runs with Java 8+. It aims to reduce the lines of code and increase code quality.
https://vavr.io
Other
5.68k stars 631 forks source link

minor: make private class final with default constructor #2740

Closed Kevin222004 closed 1 year ago

Kevin222004 commented 1 year ago

minor: make private class final with default constructor

This is part of https://github.com/checkstyle/checkstyle/pull/12737

According to the https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.8.9 The default constructor has the same access modifier as the class. and according to the check https://checkstyle.org/config_design.html#FinalClass a class that has only private constructors and has no descendant classes is declared as final.

Kevin222004 commented 1 year ago

I have given information on this changes in description.

can we have your valuable feedback on this changes.

Kevin222004 commented 1 year ago

@danieldietrich thanks for reviewing but can give your feedback on this changes usually many people avoid this what is your opinion on this type of changes

ezoerner commented 1 year ago

I'll throw in my two cents. Personally and in general, I'm a fan of declaring "effectively final" entities as explicitly final, but I think this is most important for variables (for immutable data/references) and less important for classes. Also, I think it's even less important for test classes to be declared final, and yet again even less important for inner classes in tests to be declared final. So sure, I think the checkstyle Final Class check is somewhat useful and good to have, but I don't think its application to these particular test classes in vavr is very important at all—but it doesn't hurt anything, either.

danieldietrich commented 1 year ago

Yes, that was also my impression.

The test code does not need to be checkstyle conform. IMO people overdo checkstyle a bit in real world projects, the cost/income ratio isn't good. it leads to infinite team discussions about minor "improvements". there are rarely major or critical issues found.

During the years I got more relaxed on private implementation style. The public API and the overall design are important to me. Having general rules is good. Most of the time, looking at existing code and having proper code reviews is sufficient.

In the particular case of your change I also thought: "it does not hurt".

Thanks.

romani commented 1 year ago

Thanks a lot for feedback. This is why we created this PR to get feedback. I also had exactly same worries that over focus on placing final on all is too much. But we have big set of users who want to be super explicit and we try to reconcile user demands from checkstyle behavior by extra options to allow be strict on level that team needs. We are not asking user why they need such demands from code, we just highlighting to user what we were asked to do in config.