This is an idea for replacing the rule Utility classes should not have public constructors (squid:S1118) with one that does not require us to add boilerplate code.
The problem I find with this rule is that:
-
Utility classes in general should rather be avoided, because they are not fully OO:
https://www.vojtechruzicka.com/avoid-utility-classes/ -
Even if we write utility classes for a good reason, they just contain a bunch of static methods, preferably with no (static) fields so they won’t cause us testability issues because of global states. But then, if someone looks at the overview of a class seeing a bunch of static methods, then what is the point of instantiating the class?
If you just want to use a static method, why would you ever instantiate its class? Common sense dictates you wouldn’t. It looks obvious that there is nothing to instantiate about that class because it only has static stuff in it. So what are we trying to protect ourselves from, seriously? Trying to be overly defensive about this feels like not trusting your developers having enough intelligence at all. Which sounds like a company having serious problems then. There are millions of ways code can go wrong without common sense, and I would rather spend my time on doing things that are actually worth doing.
And if you think that people can still make mistakes: there are millions of other ways we can make mistakes. Want to deal with all those too in the source code? How verbose would our classes be then? How about KISS? All those defensive code can raise WTFs in people’s head because they can’t imagine how such code lines could be useful.
And interestingly, I see this pattern everywhere:
- I think this pattern was mostly popularized by the book Effective Java (3rd Edition) by Joshua Bloch, going even further and also saying, make the class final, add an exception so really nothing can instantiate that class. But it’s seems crazy having to add all those boilerplate code to communicate intent about something that should be obvious in the first place. I don’t consider everything written in “Effective Java” as “Pragmatic Java,” partly because this book also advocates the Singleton Pattern, which not only breaks the Single Responsibility Principle (the class is also responsible for its own instantiation), but also makes testing its clients harder, because you cannot replace such singletons in your tests with a mock object. In the 3rd edition, at least the testability problem is now mentioned, but the pattern is still recommended. I already lost my willingness to continue reading that chapter when the Enum type was suggested to be abused as the perfect way of creating singletons (you inherit stuff that belong to enums, potentially breaking the Liskov substitution principle). By the way, it is rather the language that should be improved when patterns become too much boilerplate.
- Some StackOverflow articles also suggest this pattern, but without thinking of common sense, and there is always a guy who questions the necessity of this pattern with very valid questions, yet noone answers or cares:
So what I am proposing is:
Instead of forcing us to add this boilerplate code, add a rule that checks whether classes with only static members are instantiated.
In case you disagree:
- I would like to know why. I could be mistaken too, and then I would like to learn about this more.
- We could also consider adding better reasoning to our rules. Often what I miss in Sonar rule descriptions is adequate quantity of reasoning. Our developers questioning the rules all the time, will just get frustrated and that is a distraction from their work.