RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v3]

[Anthony Vanelverdinghe]

On Wed, 10 Nov 2021 09:43:58 GMT, Hendrik Schreiber <hschreiber at openjdk.org> wrote:

>>> When I see `<cleaner>`, I'm just wondering what those <> type operators are good for here...
>> 
>> What about just replacing `<cleaner>` with `...` then? The `State` constructor and its invocation also have an ellipsis.
>> 
>>> BUT, at least it's a working example and not some pseudo code.
>> 
>> The example is still not compilable due to the remaining ellipses.
>> 
>>> We do want to move to working example code long term, don't we?
>> 
>> I agree that examples should be compilable *in the raw Javadoc*. However, in the rendered Javadoc, using ellipses is a well-understood way to keep examples concise and devoid of irrelevant and/or usecase-dependent code. Moreover, when developers copy-paste the example, they'll immediately be pointed to all the places where they need to fill in the blanks, make a choice for a trade-off, etc. On the other hand, by hard-coding a (suboptimal) choice, developers who merely copy-paste the example are unlikely to reconsider the trade-off.
>> 
>>> The new example Cleaner instance _is_ shared, though on a pretty small scale (just among instances of CleaningExample).
>> 
>> True, but my point was that the comment says "shared *within a library*". So to me it's confusing to have a comment saying "it's preferred to do A", and then have the code do B on the next line.
>> 
>> So I propose to either:
>> * revert the current change & simply replace `<cleaner>` with `...`
>> * update the comment to say: "A cleaner (preferably one shared within a library, but for the sake of example, a new one is created here)"
>> 
>> Actually, to have the line be compilable, and at the same time (attempt to) force developers to consider the trade-off, it could be changed to something like:
>> 
>> 
>> private static final Cleaner cleaner = createCleaner();
>> 
>> private static Cleaner createCleaner() {
>>     // A cleaner, preferably one shared within a library
>>     throw new UnsupportedOperationException("TBD");
>> }
>
> This is getting too complicated...
> 
> It's a code *example* with a very clear comment that explains a best practice:
> 
> // A cleaner, preferably one shared within a library
> private static final Cleaner cleaner = Cleaner.create();
> 
> 
> We cannot really show the best practice in this example without making the example itself more complicated. IMHO, introducing an extra factory method here adds nothing but complexity and makes the example more difficult to understand (that aside, it should probably be something like `MyLibrary.getCleaner()` and not a `createXyz()` method).
> 
> I still much more prefer `cleaner = Cleaner.create();` over `cleaner = <cleaner>` (which really is no better in any way, shape or form and creates more questions than it provides answers) or `cleaner = ...`, which again does not answer the question of how to get a cleaner instance—something I asked myself when trying to use the API. In fact *how to get a cleaner instance* is not explained by the current javadocs *at all* and it's something one *must* know to use this API.
> 
> Here's our chance to show how it *can* be done, even if it's not ideal, because it does not demonstrate the one-cleaner per library recommendation (only mentions it).
> 
> And yes, those ellipsis in the `State` class code—I'd love for them to be gone, too. It would make the example less abstract and easier to understand (what's this state anyway? in most cases it's probably a reference to a state, e.g. a native pointer, i.e. a `long`). But, admittedly, that's difficult.
> 
> So, to summarize, there is always a tradeoff between making an example easy to understand, not too complex, but still conveying the most important information and best practices. In the end it's a matter of opinion. In this case, I will stick to my original code change suggestion, because it adds value (answers the question of how to get/create a cleaner instance).

In short: the current code


// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = <cleaner>;


is unhelpful to you, and the proposed code


// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = Cleaner.create();


is confusing to me.

Trying to find a compromise, I'm merely asking that the comment be clarified:


// A cleaner (preferably one shared within a library, but for the sake of example, a new one is created here)
private static final Cleaner cleaner = Cleaner.create();


What do you think?

-------------

PR: https://git.openjdk.java.net/jdk/pull/6076