in #16923 I already disagreed with Utility classes should not have public constructors (squid:S1118), because it makes code unnecessarily verbose, but I didn’t make myself clear: Classes with only “static” methods should not be instantiated (java:S2440) is the better rule, and that’s the only one that should exist. Which is what I request from the SonarQube developers.
I simply don’t consider it best practice to protect ourselves so hard from issues that simply never happen if we use common sense: if a class looks like a utility function, and behaves like a utility function then it is a utility function. This is duck typing, and works well in certain languages like Python. Unlike Kotlin, Java never had a good solution for this problem, but that doesn’t mean we should increase verbosity just because there is a very slight chance that someone may do the wrong thing. Instead, the wrong thing should be detected and reported as an issue, that’s what java:S2440 is about. I explained the details in the linked forum topic.
Also, there is a potential side effect of the rule:
You say, we can use Lombok to solve the problem too, well, it’s not necessarily a good library to use. It does come with downsides and risks, see for example:
I think the answer is going to be “If you don’t like the rule, don’t include it in your profile. But don’t try to kill it for the ones that do like it.”
But I’ll flag this for the language devs nonetheless.
Hi @rkrisztian,
Thanks for starting this discussion again.
As @ganncamp proposed, I think that if the rule stands in your way and you are confident that the measures you have taken prevent users of your code from instantiating the class, then I think you could drop the rule from your quality profile.
However, you are correct in pointing out that our suggestion for a fix to introduce a private constructor that throws an exception is not the right way to go since it negatively impacts coverage.
So I created 2 tickets:
SONARJAVA-5927 to replace the misleading compliant example in the docs
SONARJAVA-5928 to introduce a quickfix to avoid reproducing the old issue
Sorry that I did not respond for a long time, my situation has changed, then this thread got forgotten.
While the 2 tickets are certainly an improvement, and thank you for that, the S1118 rule still requires utility classes to be created with boilerplate code, when all an ideal linter should do is detect utility classes and prevent their accidental constructions, and in general: prevent misuse. A good linter should flag an unreferenced explicit constructor as unused.
I would like to emphasize again that this is some form of overly defensive code, because it shifts the work of linters into production code, which increases verbosity. But as we know, if a language feature does not back up this kind of work (e.g., object in Kotlin), then this strategy has several downsides, as we know from popular examples:
Yoda conditions instead of linters preventing accidental patterns.
PMD rule says, * method arguments could be made final, which makes method signatures harder to read. The right practice is S1226, which effectively treats arguments as final.
As Robert C. Martin said:
Code that protects the author at the expense of the reader is flawed code.
Therefore, as much as I love the improvements for taking a step towards the better direction, they are still setting up bad practices in my opinion.
I understand that having more options may be better than less. However, even though readability is subjective (or even if some say not as much), I always strive to think as much objectively about such problems as I can like the one we are discussing here. What I care about is optimizing code maintainability, here more specifically optimizing code readability by reducing boilerplate code. In this sense, my view is not about what rules people might like, but what makes their code go faster to production (see DORA metrics), and I think that’s what SonarQube should support. If you think I am wrong, and there are multiple ways to get there, like with the S1118 rule actually enabled, feel free to point out.
So first, opinions are great! Keep 'em coming. Just don’t expect us to always agree with you.
Second, I don’t normally engage in questions of how a particular language can/should be used or what the rules for it should/should not do. It’s been long enough since I was a hands-on coder that I only embarrass myself.
And…
You’re the one who called this “boilerplate” code. Which in my mind makes it code that is or can be auto-generated. So I’m not sure a “speed to production” argument is convincing here.
So you’d rather check every other class in the code base, and every class in every app that may use it as a library for improper instantiation, rather than simply prevent its being instantiable? That feels to me like refusing to put a lock on the bank door, and instead posting 24/7 security outside to make sure no one goes in.
Which is fine, I guess, if you want to go that route. It’s your bank. () But it seems like a lot of effort to avoid what could be a very simple solution.
Thank you for your reply. Of course we can always disagree. I have opened this thread to suggest an improvement to SonarQube, to make it better, but I also have no choice but to accept if we have different opinions on what is better.
You’re the one who called this “boilerplate” code. Which in my mind makes it code that is or can be auto-generated. So I’m not sure a “speed to production” argument is convincing here.
By definition, private constructors that are never called are still boilerplate code: they adds no functional logic to the application. Even if they can be generated, the readability of the code can still suffer, and reading code is a more frequent activity than writing it. Of course we’re only talking about one single rule here, and plenty of other things determine the actual production speed metrics, but private constructors do contribute to the overall volume of boilerplate in Java.
So you’d rather check every other class in the code base, and every class in every app that may use it as a library for improper instantiation, rather than simply prevent its being instantiable? That feels to me like refusing to put a lock on the bank door, and instead posting 24/7 security outside to make sure no one goes in.
SonarQube already checks every code, so I fail to see the difference here. Plus, As I mentioned, the real solution would be a dedicated language feature like Kotlin’s object. Therefore, I think a more fitting analogy would be, instead of putting on a door lock, we are putting on chain with a note saying “For authorized personnel only,” which then has to be read, which is slower than noticing the lock. However, until the proper solution becomes part of Java, a rule like java:S2440 helps.
One other consideration is the use of Javadoc and similar tools, which will display the public parameterless constructor the compiler generates if a class has no explicit constructor. I would say getting rid of that confusing constructor is well worth the extra line of code (private FooUtils() { }) and in such situations S1118 is useful.
Tools like Javadoc are typically used for “external” or widely used APIs; some teams also use them for exploring the internals of a code base.
Otherwise, I would say it is not that dangerous if someone creates a useless object by instantiating a utility class, so I can see why people would not want S1118.
Thanks, @jilles-sg, you are right, the duck typing idea does not work with Javadoc, because, as the JavaDoc documentation says:
Relying on the compiler ensures that the HTML output corresponds exactly with the actual implementation, which may rely on implicit, rather than explicit, source code. For example, the javadoc command documents default constructors that are present in the compiled class files but not in the source code.
This was an oversight by me, and I can’t suggest a SonarQube feature that breaks Javadoc, regardless if a project wants to use that tool or not.
I’m going to mark your answer as the solution and agree with @Dorian_Burihabwa’s tickets.
I am sorry that I didn’t see this incompatibility sooner.
) But it seems like a lot of effort to avoid what could be a very simple solution.