You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Jing Ge <ji...@ververica.com> on 2021/10/14 12:24:48 UTC
[Discussion] Avoid redundancy of Javadoc @return tag comment
Hi Flink developers,
It might be a good idea to avoid the redundant javadoc comment found in
some classes, e.g. org.apache.flink.core.fs.Path w.r.t. the @Return tag
comment on some methods.
To make the discussion clear, let's focus on a concrete example(there are
many more):
> /**
> * Returns the FileSystem that owns this Path.
> *
> * @return the FileSystem that owns this Path
> * @throws IOException thrown if the file system could not be retrieved
> */
> public FileSystem getFileSystem() throws IOException {
> return FileSystem.get(this.toUri());
> }
>
>
In order to remove the redundancy, there are two options:
option 1: keep the description and remove the @Return tag comment:
> /**
> * Returns the FileSystem that owns this Path.
> *
> * @throws IOException thrown if the file system could not be retrieved
> */
> public FileSystem getFileSystem() throws IOException {
> return FileSystem.get(this.toUri());
> }
>
> option 2: keep the @return tag comment and remove the duplicated
description:
> /**
> * @return the FileSystem that owns this Path
> * @throws IOException thrown if the file system could not be retrieved
> */
> public FileSystem getFileSystem() throws IOException {
> return FileSystem.get(this.toUri());
> }
>
> It looks like these two options are similar. From the developer's
perspective, I would prefer using @return tag comment, i.e. option 2.
Having an explicit @return tag makes it easier for me to find the return
value quickly.
This issue is very common, it has been used as a Noise Comments example in
Uncle Bob's famous "Clean Code" book on page 64 but unfortunately without
giving any clear recommendation about how to solve it. From Stackoverflow,
we can find an interesting discussion about this issue and developer's
thoughts behind it:
https://stackoverflow.com/questions/10088311/javadoc-return-tag-comment-duplication-necessary.
Javadoc 16 provides even a new feature to solve this common issue.
Since @return is recommended to use for the javadoc, I would suggest Flink
community following it and therefore open this discussion to know your
thoughts. Hopefully we can come to an agreement about this. Many thanks.
Best regards
Jing
Re: [Discussion] Avoid redundancy of Javadoc @return tag comment
Posted by Chesnay Schepler <ch...@apache.org>.
Attachments don't work on this mailing list.
I don't think the redundancy really hurts anyone. In particular because
this often happens for getters, where no one expects interesting
documentation in the first place.
Even if were to remove something:
- Removing the description would make the online Javadocs a less
readable because it is used in the method summary.
- Removing the @return tag is problematic because it would be difficult
(read: impossible) to enforce anything if there were optional.
- The Javadocs structure becomes intentionally inconsistent which to me
is even worse from a professionalism point-of-view.
All in all I wouldn't change anything.
On 15/10/2021 12:03, Jing Ge wrote:
> Since images do not work with pony mail, enclosed please find them as
> attachments.
>
> Best regards
> Jing
>
> On Thu, Oct 14, 2021 at 6:43 PM Jing Ge <ji...@ververica.com> wrote:
>
> Agree. If the description contains more information than the
> return tag comment. Part of content overlap is acceptable.
> Otherwise I think it depends on the visibility and influence of
> the API. Agree again, it is not a big issue for internal
> interfaces and classes. But for some core public APIs that will be
> used by many application developers, such redundancy looks
> unprofessional and sometimes even confused, e.g. using read mode
> in Intellij Idea, it will look like:
>
> image.png
> image.png
>
> When I read these for the first time, I was confused, why again
> and again? Is there anything wrong with my Intellij idea, maybe
> the rendering has a bug? After I realized it was the"paragon of
> redundancy" - word from Robert C. Martin, you could imagine my
> feeling about the code quality at that time :-).
>
> I would suggest doing clean-up for some core public APIs that have
> an impact on application developers.
>
> best regards
> Jing
>
> On Thu, Oct 14, 2021 at 3:24 PM Piotr Nowojski
> <pn...@apache.org> wrote:
>
> +1 for trying to clean this up, but I'm not entirely sure in which
> direction. Usually I was fixing this towards option 1. I
> agree, in your
> example option 2. looks much better, but usually the javadoc
> contains a bit
> more information, not only copy/pasted @return text. For example:
>
> /**
> * Send out a request to a specified coordinator and
> return the
> response.
> *
> * @param operatorId specifies which coordinator to
> receive the request
> * @param request the request to send
> * @return the response from the coordinator
> */
> CompletableFuture<CoordinationResponse>
> sendCoordinationRequest
>
> Sometimes this can be split quite nicely between @return and
> main java doc:
>
> /**
> * Try to transition the execution state from the current
> state to the
> new state.
> *
> * @param currentState of the execution
> * @param newState of the execution
> * @return true if the transition was successful,
> otherwise false
> */
> private boolean transitionState(ExecutionState currentState,
> ExecutionState newState);
>
> but that's not always the case.
>
> At the same time I don't have hard feelings either direction.
> After all it
> doesn't seem to be that big of an issue even if we leave it as is.
>
> Best,
> Piotrek
>
> czw., 14 paź 2021 o 14:25 Jing Ge <ji...@ververica.com> napisał(a):
>
> > Hi Flink developers,
> >
> > It might be a good idea to avoid the redundant javadoc
> comment found in
> > some classes, e.g. org.apache.flink.core.fs.Path w.r.t. the
> @Return tag
> > comment on some methods.
> >
> > To make the discussion clear, let's focus on a concrete
> example(there are
> > many more):
> >
> > > /**
> > > * Returns the FileSystem that owns this Path.
> > > *
> > > * @return the FileSystem that owns this Path
> > > * @throws IOException thrown if the file system could not
> be retrieved
> > > */
> > > public FileSystem getFileSystem() throws IOException {
> > > return FileSystem.get(this.toUri());
> > > }
> > >
> > >
> > In order to remove the redundancy, there are two options:
> >
> > option 1: keep the description and remove the @Return tag
> comment:
> >
> > > /**
> > > * Returns the FileSystem that owns this Path.
> > > *
> > > * @throws IOException thrown if the file system could not
> be retrieved
> > > */
> > > public FileSystem getFileSystem() throws IOException {
> > > return FileSystem.get(this.toUri());
> > > }
> > >
> > > option 2: keep the @return tag comment and remove the
> duplicated
> > description:
> >
> > > /**
> > > * @return the FileSystem that owns this Path
> > > * @throws IOException thrown if the file system could not
> be retrieved
> > > */
> > > public FileSystem getFileSystem() throws IOException {
> > > return FileSystem.get(this.toUri());
> > > }
> > >
> > > It looks like these two options are similar. From the
> developer's
> > perspective, I would prefer using @return tag comment, i.e.
> option 2.
> > Having an explicit @return tag makes it easier for me to
> find the return
> > value quickly.
> >
> > This issue is very common, it has been used as a Noise
> Comments example in
> > Uncle Bob's famous "Clean Code" book on page 64 but
> unfortunately without
> > giving any clear recommendation about how to solve it. From
> Stackoverflow,
> > we can find an interesting discussion about this issue and
> developer's
> > thoughts behind it:
> >
> >
> https://stackoverflow.com/questions/10088311/javadoc-return-tag-comment-duplication-necessary
> > .
> > Javadoc 16 provides even a new feature to solve this common
> issue.
> >
> > Since @return is recommended to use for the javadoc, I would
> suggest Flink
> > community following it and therefore open this discussion to
> know your
> > thoughts. Hopefully we can come to an agreement about this.
> Many thanks.
> >
> > Best regards
> > Jing
> >
>
Re: [Discussion] Avoid redundancy of Javadoc @return tag comment
Posted by Jing Ge <ji...@ververica.com>.
Since images do not work with pony mail, enclosed please find them as
attachments.
Best regards
Jing
On Thu, Oct 14, 2021 at 6:43 PM Jing Ge <ji...@ververica.com> wrote:
> Agree. If the description contains more information than the return tag
> comment. Part of content overlap is acceptable. Otherwise I think it
> depends on the visibility and influence of the API. Agree again, it is not
> a big issue for internal interfaces and classes. But for some core public
> APIs that will be used by many application developers, such redundancy
> looks unprofessional and sometimes even confused, e.g. using read mode in
> Intellij Idea, it will look like:
>
> [image: image.png]
> [image: image.png]
>
> When I read these for the first time, I was confused, why again and again?
> Is there anything wrong with my Intellij idea, maybe the rendering has a
> bug? After I realized it was the "paragon of redundancy" - word from
> Robert C. Martin, you could imagine my feeling about the code quality at
> that time :-).
>
> I would suggest doing clean-up for some core public APIs that have an
> impact on application developers.
>
> best regards
> Jing
>
> On Thu, Oct 14, 2021 at 3:24 PM Piotr Nowojski <pn...@apache.org>
> wrote:
>
>> +1 for trying to clean this up, but I'm not entirely sure in which
>> direction. Usually I was fixing this towards option 1. I agree, in your
>> example option 2. looks much better, but usually the javadoc contains a
>> bit
>> more information, not only copy/pasted @return text. For example:
>>
>> /**
>> * Send out a request to a specified coordinator and return the
>> response.
>> *
>> * @param operatorId specifies which coordinator to receive the
>> request
>> * @param request the request to send
>> * @return the response from the coordinator
>> */
>> CompletableFuture<CoordinationResponse> sendCoordinationRequest
>>
>> Sometimes this can be split quite nicely between @return and main java
>> doc:
>>
>> /**
>> * Try to transition the execution state from the current state to the
>> new state.
>> *
>> * @param currentState of the execution
>> * @param newState of the execution
>> * @return true if the transition was successful, otherwise false
>> */
>> private boolean transitionState(ExecutionState currentState,
>> ExecutionState newState);
>>
>> but that's not always the case.
>>
>> At the same time I don't have hard feelings either direction. After all it
>> doesn't seem to be that big of an issue even if we leave it as is.
>>
>> Best,
>> Piotrek
>>
>> czw., 14 paź 2021 o 14:25 Jing Ge <ji...@ververica.com> napisał(a):
>>
>> > Hi Flink developers,
>> >
>> > It might be a good idea to avoid the redundant javadoc comment found in
>> > some classes, e.g. org.apache.flink.core.fs.Path w.r.t. the @Return tag
>> > comment on some methods.
>> >
>> > To make the discussion clear, let's focus on a concrete example(there
>> are
>> > many more):
>> >
>> > > /**
>> > > * Returns the FileSystem that owns this Path.
>> > > *
>> > > * @return the FileSystem that owns this Path
>> > > * @throws IOException thrown if the file system could not be
>> retrieved
>> > > */
>> > > public FileSystem getFileSystem() throws IOException {
>> > > return FileSystem.get(this.toUri());
>> > > }
>> > >
>> > >
>> > In order to remove the redundancy, there are two options:
>> >
>> > option 1: keep the description and remove the @Return tag comment:
>> >
>> > > /**
>> > > * Returns the FileSystem that owns this Path.
>> > > *
>> > > * @throws IOException thrown if the file system could not be
>> retrieved
>> > > */
>> > > public FileSystem getFileSystem() throws IOException {
>> > > return FileSystem.get(this.toUri());
>> > > }
>> > >
>> > > option 2: keep the @return tag comment and remove the duplicated
>> > description:
>> >
>> > > /**
>> > > * @return the FileSystem that owns this Path
>> > > * @throws IOException thrown if the file system could not be
>> retrieved
>> > > */
>> > > public FileSystem getFileSystem() throws IOException {
>> > > return FileSystem.get(this.toUri());
>> > > }
>> > >
>> > > It looks like these two options are similar. From the developer's
>> > perspective, I would prefer using @return tag comment, i.e. option 2.
>> > Having an explicit @return tag makes it easier for me to find the return
>> > value quickly.
>> >
>> > This issue is very common, it has been used as a Noise Comments example
>> in
>> > Uncle Bob's famous "Clean Code" book on page 64 but unfortunately
>> without
>> > giving any clear recommendation about how to solve it. From
>> Stackoverflow,
>> > we can find an interesting discussion about this issue and developer's
>> > thoughts behind it:
>> >
>> >
>> https://stackoverflow.com/questions/10088311/javadoc-return-tag-comment-duplication-necessary
>> > .
>> > Javadoc 16 provides even a new feature to solve this common issue.
>> >
>> > Since @return is recommended to use for the javadoc, I would suggest
>> Flink
>> > community following it and therefore open this discussion to know your
>> > thoughts. Hopefully we can come to an agreement about this. Many thanks.
>> >
>> > Best regards
>> > Jing
>> >
>>
>
Re: [Discussion] Avoid redundancy of Javadoc @return tag comment
Posted by Jing Ge <ji...@ververica.com>.
Agree. If the description contains more information than the return tag
comment. Part of content overlap is acceptable. Otherwise I think it
depends on the visibility and influence of the API. Agree again, it is not
a big issue for internal interfaces and classes. But for some core public
APIs that will be used by many application developers, such redundancy
looks unprofessional and sometimes even confused, e.g. using read mode in
Intellij Idea, it will look like:
[image: image.png]
[image: image.png]
When I read these for the first time, I was confused, why again and again?
Is there anything wrong with my Intellij idea, maybe the rendering has a
bug? After I realized it was the "paragon of redundancy" - word from Robert
C. Martin, you could imagine my feeling about the code quality at that time
:-).
I would suggest doing clean-up for some core public APIs that have an
impact on application developers.
best regards
Jing
On Thu, Oct 14, 2021 at 3:24 PM Piotr Nowojski <pn...@apache.org> wrote:
> +1 for trying to clean this up, but I'm not entirely sure in which
> direction. Usually I was fixing this towards option 1. I agree, in your
> example option 2. looks much better, but usually the javadoc contains a bit
> more information, not only copy/pasted @return text. For example:
>
> /**
> * Send out a request to a specified coordinator and return the
> response.
> *
> * @param operatorId specifies which coordinator to receive the request
> * @param request the request to send
> * @return the response from the coordinator
> */
> CompletableFuture<CoordinationResponse> sendCoordinationRequest
>
> Sometimes this can be split quite nicely between @return and main java doc:
>
> /**
> * Try to transition the execution state from the current state to the
> new state.
> *
> * @param currentState of the execution
> * @param newState of the execution
> * @return true if the transition was successful, otherwise false
> */
> private boolean transitionState(ExecutionState currentState,
> ExecutionState newState);
>
> but that's not always the case.
>
> At the same time I don't have hard feelings either direction. After all it
> doesn't seem to be that big of an issue even if we leave it as is.
>
> Best,
> Piotrek
>
> czw., 14 paź 2021 o 14:25 Jing Ge <ji...@ververica.com> napisał(a):
>
> > Hi Flink developers,
> >
> > It might be a good idea to avoid the redundant javadoc comment found in
> > some classes, e.g. org.apache.flink.core.fs.Path w.r.t. the @Return tag
> > comment on some methods.
> >
> > To make the discussion clear, let's focus on a concrete example(there are
> > many more):
> >
> > > /**
> > > * Returns the FileSystem that owns this Path.
> > > *
> > > * @return the FileSystem that owns this Path
> > > * @throws IOException thrown if the file system could not be retrieved
> > > */
> > > public FileSystem getFileSystem() throws IOException {
> > > return FileSystem.get(this.toUri());
> > > }
> > >
> > >
> > In order to remove the redundancy, there are two options:
> >
> > option 1: keep the description and remove the @Return tag comment:
> >
> > > /**
> > > * Returns the FileSystem that owns this Path.
> > > *
> > > * @throws IOException thrown if the file system could not be retrieved
> > > */
> > > public FileSystem getFileSystem() throws IOException {
> > > return FileSystem.get(this.toUri());
> > > }
> > >
> > > option 2: keep the @return tag comment and remove the duplicated
> > description:
> >
> > > /**
> > > * @return the FileSystem that owns this Path
> > > * @throws IOException thrown if the file system could not be retrieved
> > > */
> > > public FileSystem getFileSystem() throws IOException {
> > > return FileSystem.get(this.toUri());
> > > }
> > >
> > > It looks like these two options are similar. From the developer's
> > perspective, I would prefer using @return tag comment, i.e. option 2.
> > Having an explicit @return tag makes it easier for me to find the return
> > value quickly.
> >
> > This issue is very common, it has been used as a Noise Comments example
> in
> > Uncle Bob's famous "Clean Code" book on page 64 but unfortunately without
> > giving any clear recommendation about how to solve it. From
> Stackoverflow,
> > we can find an interesting discussion about this issue and developer's
> > thoughts behind it:
> >
> >
> https://stackoverflow.com/questions/10088311/javadoc-return-tag-comment-duplication-necessary
> > .
> > Javadoc 16 provides even a new feature to solve this common issue.
> >
> > Since @return is recommended to use for the javadoc, I would suggest
> Flink
> > community following it and therefore open this discussion to know your
> > thoughts. Hopefully we can come to an agreement about this. Many thanks.
> >
> > Best regards
> > Jing
> >
>
Re: [Discussion] Avoid redundancy of Javadoc @return tag comment
Posted by Piotr Nowojski <pn...@apache.org>.
+1 for trying to clean this up, but I'm not entirely sure in which
direction. Usually I was fixing this towards option 1. I agree, in your
example option 2. looks much better, but usually the javadoc contains a bit
more information, not only copy/pasted @return text. For example:
/**
* Send out a request to a specified coordinator and return the
response.
*
* @param operatorId specifies which coordinator to receive the request
* @param request the request to send
* @return the response from the coordinator
*/
CompletableFuture<CoordinationResponse> sendCoordinationRequest
Sometimes this can be split quite nicely between @return and main java doc:
/**
* Try to transition the execution state from the current state to the
new state.
*
* @param currentState of the execution
* @param newState of the execution
* @return true if the transition was successful, otherwise false
*/
private boolean transitionState(ExecutionState currentState,
ExecutionState newState);
but that's not always the case.
At the same time I don't have hard feelings either direction. After all it
doesn't seem to be that big of an issue even if we leave it as is.
Best,
Piotrek
czw., 14 paź 2021 o 14:25 Jing Ge <ji...@ververica.com> napisał(a):
> Hi Flink developers,
>
> It might be a good idea to avoid the redundant javadoc comment found in
> some classes, e.g. org.apache.flink.core.fs.Path w.r.t. the @Return tag
> comment on some methods.
>
> To make the discussion clear, let's focus on a concrete example(there are
> many more):
>
> > /**
> > * Returns the FileSystem that owns this Path.
> > *
> > * @return the FileSystem that owns this Path
> > * @throws IOException thrown if the file system could not be retrieved
> > */
> > public FileSystem getFileSystem() throws IOException {
> > return FileSystem.get(this.toUri());
> > }
> >
> >
> In order to remove the redundancy, there are two options:
>
> option 1: keep the description and remove the @Return tag comment:
>
> > /**
> > * Returns the FileSystem that owns this Path.
> > *
> > * @throws IOException thrown if the file system could not be retrieved
> > */
> > public FileSystem getFileSystem() throws IOException {
> > return FileSystem.get(this.toUri());
> > }
> >
> > option 2: keep the @return tag comment and remove the duplicated
> description:
>
> > /**
> > * @return the FileSystem that owns this Path
> > * @throws IOException thrown if the file system could not be retrieved
> > */
> > public FileSystem getFileSystem() throws IOException {
> > return FileSystem.get(this.toUri());
> > }
> >
> > It looks like these two options are similar. From the developer's
> perspective, I would prefer using @return tag comment, i.e. option 2.
> Having an explicit @return tag makes it easier for me to find the return
> value quickly.
>
> This issue is very common, it has been used as a Noise Comments example in
> Uncle Bob's famous "Clean Code" book on page 64 but unfortunately without
> giving any clear recommendation about how to solve it. From Stackoverflow,
> we can find an interesting discussion about this issue and developer's
> thoughts behind it:
>
> https://stackoverflow.com/questions/10088311/javadoc-return-tag-comment-duplication-necessary
> .
> Javadoc 16 provides even a new feature to solve this common issue.
>
> Since @return is recommended to use for the javadoc, I would suggest Flink
> community following it and therefore open this discussion to know your
> thoughts. Hopefully we can come to an agreement about this. Many thanks.
>
> Best regards
> Jing
>