RFR: 8283269: Improve definition and use of jdk.javadoc.internal.doclets.toolkit.Content

Fri Mar 25 14:09:29 UTC 2022

On Wed, 16 Mar 2022 16:32:09 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

> This PR started in a draft mode as a refactoring effort. After a few commits and chats with Jonathan Gibbons, I decided that further refactoring belongs to follow-up PRs and that this PR could be marked as "Ready for review".
> 
> The motivation for the initial refactoring effort was as follows. The word "tree" is heavily overloaded in the JavaDoc codebase, both in comments and code. Here are some of the contexts this word appears in:  
> 
>   * Hierarchy Tree (i.e. overview-tree.html and package-tree.html)
>   * DocTree (AST nodes)
>   * ClassTree, *TreeWriter (data structures and entities related to supertype-subtype relationship)
>   * HtmlTree (HTML nodes) and specifically UL/OL elements which are nested lists
> 
> Sometimes contexts overlap, making the word "tree" ambiguous. To reduce this ambiguity, the word "tree" should be dropped in favor of a more specific word of phrase where possible.
> 
> In the case where the context is `jdk.javadoc.internal.doclets.toolkit.Content`, the programmer is already aware that they are dealing with trees in the sense that `Content` objects support recursive composition. There's no need to have the phrase "content tree" where "content" would do. Moreover, in the context that is exclusively about `Content` objects, the word "content" can be dropped too, since the type information is assumed.
> 
> As an example of content overlap, have a look at the source of `jdk.javadoc.internal.doclets.toolkit.builders.MemberSummaryBuilder::buildSummary`. This method used to needlessly mix DocTree with Content tree.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java line 227:

> 225:      */
> 226:     protected Content getParameters(ExecutableElement member, boolean includeAnnotations) {
> 227:         Content paramContent = new ContentBuilder();

Continuing the theme of using more specific words than `content`, I've started using the word `result` for those cases where the code is building up an object that will be the result of the method.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java line 280:

> 278: 
> 279:     /**
> 280:      * Get a content containing the exception information for the executable member.

Suggest "Returns content containing ..."

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 96:

> 94:      * @param content the content to which the label will be added
> 95:      */
> 96:     public abstract void addSummaryLabel(Content content);

Earlier in this review you used `target` for such an item

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 179:

> 177:      * @param member the member being linked to
> 178:      *
> 179:      * @return a content representing the link

you could either just drop the initial  "a" or even the initial phrase and reduce it to `@return the link`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 275:

> 273:      *
> 274:      * @param member  the member being documented.
> 275:      * @param content the content to which the comment will be added.

remove trailing periods

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 298:

> 296: 
> 297:     /**
> 298:      * Adds use information to the documentation.

An alternative would be one of
* Adds use information to the given content.
* Adds use information to the specified content.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 404:

> 402:     @Override
> 403:     public Content getMember(Content memberContent) {
> 404:         return writer.getMember(memberContent);

As a comment on the underlying API (not your changes), this method is confusing or under-specified or both.   

I think this method name is too short.  In an `AbstractMemberWriter`, the member might reasonably be expected to be the underlying `Element`.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java line 70:

> 68:         addConfigurationTitle(target);
> 69:         addOverviewComment(target);
> 70:         addOverviewTags(target);

Interesting. The name `main` was used to evoke the intent that the argument was an HTML `<main>` element.

Given this method does not override anything in `toolkit`, I think you could reasonably change the parameter type to `HtmlTree` and even (*cough*) add an assert that the argument has a `Tag.Name.MAIN`. On the other hand, that would be inconsistent with other targets.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java line 77:

> 75:      * "-overview" option on the command line.
> 76:      *
> 77:      * @param content the documentation to which the overview comment will

If the parameter name was `target`, you could replace `documentation` with `content`.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AllPackagesIndexWriter.java line 92:

> 90: 
> 91:     /**
> 92:      * Add all the packages to the content.

Should be `adds`

regarding `the content` ... it seems strange to see `the content` used in a way that does not specify _which_ content. That's why I often use `the given content` or `the specified content` in summary sentences. Then, later on, in the context of a `@param` it works to give the shorter form of `the content`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeMemberWriterImpl.java line 94:

> 92: 
> 93:     @Override
> 94:     public Content getMemberSummaryHeader(TypeElement typeElement,

OK, this is a weird schizophrenic method ... is it `get` or is it `add` ?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassUseWriter.java line 231:

> 229: 
> 230:     /**
> 231:      * Add the class use documentation.

I have a pair of classes I use in small utilities:
1. DeclScanner ... find accessible declarations in a `CompilationUnitTree`
2. (subtype of) DocTreeScanner ... analyze doc comments on those accessible declarations.

Whilst I know you are not a big fan of doc comments on internal API, we could still use them to find doc comments whose first sentence does not begin with 3rd person declarative form (the ...s form) of a verb.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassUseWriter.java line 246:

> 244: 
> 245:     /**
> 246:      * Add the packages elements that use the given class.

Pedantic: `Adds the package elements`  (note: package not packages)

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java line 222:

> 220: 
> 221:     /**
> 222:      * Get the class inheritance tree for the given class.

* Gets

"class inheritance" suggests superclass, which is not a tree. Depending on functional semantics[1], maybe just "class hierarchy", since superclasses do not form a tree and interfaces form an acyclic graph.

[1] impl looks like it is just going up the superclass hierarchy

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ConstantsSummaryWriterImpl.java line 144:

> 142: 
> 143:     @Override
> 144:     public void addPackageName(PackageElement pkg, Content toContent, boolean first) {

This is not your average `add` method.

A different cleanup would be to review methods that have non-local side effects, which seems to be the case here (i.e. `summaryTree`)

The use of `first` looks weird too.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 346:

> 344:      *
> 345:      * @param key the key for the desired string
> 346:      * @return a content for the string

Rather than `a content for ...`, consider `content containing ...`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/EnumConstantWriterImpl.java line 79:

> 77:                                                  Content memberDetails) {
> 78:         memberDetails.add(MarkerComments.START_OF_ENUM_CONSTANT_DETAILS);
> 79:         Content enumConstantsDetailsContent = new ContentBuilder();

This is one of those cases where a short, more generic name like `result` might work better

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

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