Style Police (a rant)

[Eric Sosman]

In recent days I've encountered a tool called "Checkstyle," that
parses Java code and flags various departures from its baked-in rules.
Some of these are worthwhile: It will whine if you override equals()
without overriding hashCode(), it will shriek if a method's Javadoc
omits a @param or @throws, it will moan if a static final field isn't
ALL_CAPS, and so on. Some are less so: It insists that all method
parameters should be final, it forbids the ?: operator, it tut-tuts
at `x << 8' for using a "magic number."

But the subject of this rant is its complaint that a class is "not
designed for extension." You write a perfectly ordinary class with
methods foo() and bar(), and Checkstyle throws up its hands and gibbers
that your class is "not designed for extension." Say, what?

Well, it turns out that "not designed for extension" means that
your class has one or more methods that are not either final, abstract,
or empty. That's it, that's The Rule.

You can "design for extension" by having final methods, methods
that a subclass cannot override even if it needs to.

You can "design for extension" by having abstract methods, methods
with no implementation that a subclass must implement on its own.

Or you can "design for extension" by having methods that do
nothing at all. (A peculiar measure of productivity, it seems to me.)

BUT if you have a concrete non-final method that actually *does*
something, you have not "designed for extension." It's all in aid of
somebody's theory about programming style: You're all right as long as
you freeze your methods against overriding or ensure they're impotent.

I wonder what this Checkstyle tool would think of the concrete,
non-final, non-empty equals(), hashCode(), clone(), toString(), and
finalize() methods of ... java.lang.Object, the one class *no* Java
program can avoid extending. If java.lang.Object is "not designed for
extension," is there any hope left for the language?

Enforcing good style is difficult. I wish the purveyors of the
enforcement tools would realize it's beyond their powers to do it well.

 

[Robert Klemme]

In recent days I've encountered a tool called "Checkstyle," that
parses Java code and flags various departures from its baked-in rules.
Some of these are worthwhile: It will whine if you override equals()
without overriding hashCode(), it will shriek if a method's Javadoc
omits a @param or @throws, it will moan if a static final field isn't
ALL_CAPS, and so on. Some are less so: It insists that all method
parameters should be final, it forbids the ?: operator, it tut-tuts
at `x << 8' for using a "magic number."

But the subject of this rant is its complaint that a class is "not
designed for extension." You write a perfectly ordinary class with
methods foo() and bar(), and Checkstyle throws up its hands and gibbers
that your class is "not designed for extension." Say, what?

Well, it turns out that "not designed for extension" means that
your class has one or more methods that are not either final, abstract,
or empty. That's it, that's The Rule.
....

Enforcing good style is difficult. I wish the purveyors of the
enforcement tools would realize it's beyond their powers to do it well.

I couldn't agree more, especially since I have suffered badly from the
"method parameters must be final" style police of this tool (which has
issues of its own) when a company bought my company and tried to
introduce the checkstyle police on a ton of existing code. Luckily I
could avoid being forced to convert _all_ method parameters in the code
to the "new style" - and eventually working for the company altogether.

And it's not only the authors of such tools. There are managers for
whom it seems to be easier to enforce rules with tools like these than
to judge the quality of their workforce themselves. I can accept that
these programs are imperfect (and e.g. use them to deliver helpful
hints), but people making these programs a bible are worse.

Kind regards

robert

 

[Rajiv Gupta]

I wonder what this Checkstyle tool would think of the concrete,
non-final, non-empty equals(), hashCode(), clone(), toString(), and
finalize() methods of ... java.lang.Object, the one class *no* Java
program can avoid extending. If java.lang.Object is "not designed for
extension," is there any hope left for the language?

Enforcing good style is difficult. I wish the purveyors of the
enforcement tools would realize it's beyond their powers to do it well.

You are not forced to use the tool. These tool are often written by
the pedants and those who don't have much real world experience. The
correctness checking inbuilt into modern IDEs is sufficient anyway.

 

[Volker Borchert]

You are not forced to use the tool. These tool are often written by
the pedants and those who don't have much real world experience. The
correctness checking inbuilt into modern IDEs is sufficient anyway.

<advocacy>
I like findbugs. It integrates neatly into Eclipse, detectors can
be enabled or disabled one by one, and you could even add your own.
</advocacy>

 

[Jan Burse]

BUT if you have a concrete non-final method that actually *does*
something, you have not "designed for extension." It's all in aid of
somebody's theory about programming style: You're all right as long as

I have noticed that final methods sometimes execute faster
than non-final methods. The final modifier gives a hint to
the optimizers.

So I usually develop for a while without bothering about final,
then I run some of the style checks of the IDA, that also spot
missing final keywords. And I put the final keywords.

Bye

 

[Jan Burse]

I have noticed that final methods sometimes execute faster
than non-final methods. The final modifier gives a hint to
the optimizers.

So I usually develop for a while without bothering about final,
then I run some of the style checks of the IDA, that also spot
missing final keywords. And I put the final keywords.

Bye

The final keywords is found more then ten time (>10) on
the following page:
http://www.javaperformancetuning.com/tips/rawtips.shtml

Bye

 

[Tom Anderson]

In recent days I've encountered a tool called "Checkstyle," that
parses Java code and flags various departures from its baked-in rules.

You mean the rules you configure? Or by 'baked-in' do you mean the default
rule set?

I assume you are not making the mistake of assuming that Checkstyle's rule
set is not configurable.

Some of these are worthwhile [...] Some are less so

That has been our experience with it too. When we added it to our
integration build process, one of our guys had to spend quite a while
grubbing through the rules chucking out those we didn't subscribe to. We
also planned to revise the rule set as time went by and we found more
problems with it, although in practice we haven't had to do that much.

We also run PMD in the build process, similarly tuned:

http://pmd.sourceforge.net/

We have Checkstyle set up to primarily check non-functional style rules
(formatting, variable naming, and so on), and PMD to look for potential
bugs, because it's better at it.

Note that the authors of PMD have the good grace to recognise that some of
the rules they include are controversial:

http://pmd.sourceforge.net/rules/controversial.html

But the subject of this rant is its complaint that a class is "not
designed for extension." You write a perfectly ordinary class with
methods foo() and bar(), and Checkstyle throws up its hands and gibbers
that your class is "not designed for extension." Say, what?

I know what this rule is getting at. Hopefully Lew will chime in at some
point soon and explain it.

I wonder what this Checkstyle tool would think of the concrete,
non-final, non-empty equals(), hashCode(), clone(), toString(), and
finalize() methods of ... java.lang.Object, the one class *no* Java
program can avoid extending. If java.lang.Object is "not designed for
extension," is there any hope left for the language?

Well put!

tom

 

[Eric Sosman]

The final keywords is found more then ten time (>10) on
the following page:
http://www.javaperformancetuning.com/tips/rawtips.shtml

The page looks like a simple list of one-sentence "tips," just
collected without evaluation. "Collected" in the past tense, by
the way, meaning a decade or more ago. If you're tuning your 2011
Java code to get maximum performance from a 1998 JVM, I respecfully
submit you're probably making the wrong optimizations.

Besides, that's not my complaint. This Checkstyle tool wants
me to ensure that no actual executable code is ever overridden; it
is happy only if an overridable method is empty or abstract. On
that criterion, java.lang.Object is "not designed for extension,"
an assessment that strikes me as fundamentally fatuous.

 

[Lew]

How did you "notice" that - by what measurements under what conditions?

The final keywords is found more then ten time (>10) on
the following page:
http://www.javaperformancetuning.com/tips/rawtips.shtml

Really? You're citing performance optimization tips from 1999-2991?

You do realize that virtually no performance "tips" from that era apply anymore, right?

Even in 2002 the use of 'final' to help optimize was recognized as urban legend:
http://www.ibm.com/developerworks/java/library/j-jtp1029/index.html
"Like many myths about Java performance, the erroneous belief that declaring classes or methods as final results in better performance is widely held but rarely examined. The argument goes that declaring a method or class as final means that the compiler can inline method calls more aggressively, because it knows that at run time this is definitely the version of the method that's going to be called. But this is simply not true. Just because class X is compiled against final class Y doesn't mean that the same version ofclass Y will be loaded at run time. So the compiler cannot inline such cross-class method calls safely, final or not. Only if a method is private canthe compiler inline it freely, and in that case, the final keyword would be redundant.

"On the other hand, the run-time environment and JIT compiler have more information about what classes are actually loaded, and can make much better optimization decisions than the compiler can. If the run-time environment knows that no classes are loaded that extend Y, then it can safely inline calls to methods of Y, regardless of whether Y is final (as long as it can invalidate such JIT-compiled code if a subclass of Y is later loaded). So the reality is that while final might be a useful hint to a dumb run-time optimizer that doesn't perform any global dependency analysis, its use doesn't actually enable very many compile-time optimizations, and is not needed by asmart JIT to perform run-time optimizations."

 

[Lew]

Really? You're citing performance optimization tips from 1999-2991?

Oops, fat-fingered that one: "1999-2001"

 

[Lew]

In recent days I've encountered a tool called "Checkstyle," that
parses Java code and flags various departures from its baked-in rules.

You mean the rules you configure? Or by 'baked-in' do you mean the default
rule set?

I assume you are not making the mistake of assuming that Checkstyle's rule
set is not configurable.

Some of these are worthwhile [...] Some are less so

That has been our experience with it too. ...
[snip]

But the subject of this rant is its complaint that a class is "not
designed for extension." You write a perfectly ordinary class with
methods foo() and bar(), and Checkstyle throws up its hands and gibbers
that your class is "not designed for extension." Say, what?

I know what this rule is getting at. Hopefully Lew will chime in at some
point soon and explain it.

I defer to a more expert Bloch on this:
http://java.sun.com/docs/books/effective/
Item 17: Design and document for inheritance or else prohibit it

The nut of this tip is that code outlives the writing of it, a much-ignored truth to the detriment of the code. If you leave a door open, someone will walk through it regardless of the danger on the other side.

 

[Eric Sosman]

You mean the rules you configure? Or by 'baked-in' do you mean the
default rule set?

I assume you are not making the mistake of assuming that Checkstyle's
rule set is not configurable.

Yes, the rules are configurable. You can enable and disable them,
you can change triggering thresholds where it makes sense, you can
even write your own and extend the rule set to your liking. It was
not my purpose to document Checkstyle in a Usenet post. As advertised,
it was my purpose to rant.

Note that the authors of PMD have the good grace to recognise that some
of the rules they include are controversial:

They do, yes. It is, I suppose, the lot of every Lint to be on
the receiving end of rants like mine. People even rant at compilers
("Whaddya mean, no creating a generic array? Stupid compiler!"), so
what chance has a purposefully pickier tool to escape complaint?
It has to be said that Checkstyle's configurability is a large point
in its favor: If you dislike a rule (and if you can get your own
meatware build police to go along), you can turn it off or modify it.

Still: An occasional rant is good for the soul. YAAAARRRGGG!!!

 

[Jan Burse]

How did you "notice" that - by what measurements under what conditions?

It is around 1% - 10% in a crucial spot in my application.
Without the final keyword a JIT potentially needs to reoptimize
code, when classes are later loaded. Since although an analysis
might yield that a class method is actually not overridden,
it might still get overridden at run time.

It might get overridden by Class.forName or even more
massive by URLClassLoader.addURL. May application makes use
of the former, therefore I am always doing the final where
I can.

Bye

 

[Jan Burse]

Without the final keyword a JIT potentially needs to reoptimize
code, when classes are later loaded. Since although an analysis
might yield that a class method is actually not overridden,
it might still get overridden at run time.

Of course the problem is not so much there for JITs that do
call site specific PICs. So a method that is not overridden
will also have not multiple entries in the PICs.

But with the final keyword we can put a closure mark on the
PICs we don't need to bother of extending them at runtime
in case a new method implementation pops up at the call site.
And this could change the realization of the PICs.

So I guess this is not an urban legend. Unfortunately I did
document so much my findings that final has an impact. It
occurs to me once a while. But also I am switching the JDK
a couples of time, i.e. migrating form 32-bit to 64-bit.

With the 64-bit I noticed I different sensitivity profile.
Things that made the 32-bit JDK stumble don't have any impact
at all for the 64-bit. For example the garbage collection runs
very smooth with much less effort.

So not an urban legend but maybe yes a moving target.

Bye

 

[Jan Burse]

Of course the problem is not so much there for JITs that do
call site specific PICs. So a method that is not overridden
will also have not multiple entries in the PICs.

Definition of PICs: See here:
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.36.6379&rep=rep1&type=pdf

 

[Lew]

Of course the problem is not so much there for JITs that do
call site specific PICs. So a method that is not overridden
will also have not multiple entries in the PICs.

But with the final keyword we can put a closure mark on the
PICs we don't need to bother of extending them at runtime
in case a new method implementation pops up at the call site.
And this could change the realization of the PICs.

So I guess this is not an urban legend. Unfortunately I did
document so much my findings that final has an impact. It
occurs to me once a while. But also I am switching the JDK
a couples of time, i.e. migrating form 32-bit to 64-bit.

With the 64-bit I noticed I different sensitivity profile.
Things that made the 32-bit JDK stumble don't have any impact
at all for the 64-bit. For example the garbage collection runs
very smooth with much less effort.

So not an urban legend but maybe yes a moving target.

Thanks for the information.

Except in performance-critical hot spots in code, such as yours presumably was, the movability of the target militates against such strategies. More importantly, use of 'final' on classes and methods is a semantic restriction; you have to be responsible for the consequences to program logic. Fortunately for your use case, it's frequently the right thing to do.

As usual, the rule is to do the right thing for the logic first and foremost. The problem with your post on the optimization aspect is that it reads like a general rule when in fact it was a particular case for a particular environment with a (presumably) important effect on (presumably) critical performance. By your post cited here, it isn't even a durable effect. Again, the downside is mitigated by the likely semantic benefit of using 'final'in that context, but that seems more coincidental in this case than deliberate.

I notice that you didn't describe your performance-test methodology. HotSpot compilation has some interesting variation in its effects depending on run-time considerations like whether you've heated up the analyzer (run through the code x thousand times), other things in the JVM, changes in the underlying platform, brand of JVM (strictly speaking only Oracle's has "HotSpot"), and so on. In your case I'll presume your methodology was rigorous enough, although "Unfortunately I did document so much my findings that finalhas an impact" should make anyone nervous about that. But that does not mean that one should recommend 'final' as a way to encourage inlining as a general technique, particularly in the face of the many expert warnings against that very practice.

 

[Jan Burse]

I'll presume your methodology was rigorous enough, although "Unfortunately
I did document so much my findings that final has an impact"
should make anyone nervous

Well there was a typo in my post, it should read: I did NOT document

 

[Lew]

Well there was a typo in my post, it should read: I did NOT document

Oh - I actually read it wrong and thought you had said, "... did not document ...", and not documenting was what frightened.

 

[Jan Burse]

Oh - I actually read it wrong and thought you had said, "... did not document ...",
and not documenting was what frightened.

Well it would be frightening if life would depend on it,
but since the keyword final either does nothing or
does increase the speed only a little, I focused on
other stuff.

 

[Lew]

Well it would be frightening if life would depend on it,
but since the keyword final either does nothing or
does increase the speed only a little, I focused on
other stuff.

Fair enough, but I thought your thesis was that the 'final' keyword was a good thing for performance. I apologize for misunderstanding.

While the 'final' keyword "either does nothing or does increase the speed only a little" from a performance standpoint, as you say, the OP's rant related to the semantic impact. In that domain the 'final' keyword does a great deal.

Whether that impact is good or bad depends on how rigorous you are when youwrite classes. Josh Bloch's advice and the default enforcement by Checkstyle are grounded in a rigorous style of class design where you account for all corner cases and don't leave much, if anything, to chance for future users of the API. This is the style that, for example, favors checked exceptions. Some folks on the receiving side of such decisions get a little peeved - they feel put upon to catch exceptions, "Favor composition over inheritance" or "Design and document for inheritance or else prohibit it" [op cit..]. Well, that's why they're paid the big bucks. Such restrictions increase the safety and predictability of the code and systems built with it, which is their rationale.

Rigor comes at a cost, that is true. Intelligent design requires that you consider the tradeoffs, bearing in mind that you yourself might or might not directly suffer the consequences of your choices. Checkstyle errs on theside of rigor in its defaults.