On Wed, 10 Nov 2021 09:43:58 GMT, Hendrik Schreiber <hschrei...@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
