ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Garrett Alley <garrett.al...@gridgain.com>
Subject Re: Coding guidelines. Useless JavaDoc comments.
Date Thu, 08 Aug 2019 15:18:11 GMT
Hello Igniters,

[A quick introduction: My name is Garrett and I manage documentation at
GridGain. I'm relatively new here — I've been at GridGain for 4 months —
but I've been in Documentation for most of my career.]

I put together some _proposed_ guidelines for Javadocs as a starting point
for us to discuss, *modify*, and hopefully eventually adopt.

These are based on what I've seen succeed at other companies/on the
internet:

https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=123900577

For me, the reason I prefer optional comments is that I don't want users to
have to wade through the obvious stuff to find the useful/insightful
information. If we can reduce the amount of comments, but still retain the
useful information, the readers will be rewarded.

Thanks!

===

Garrett Alley
Documentation
GridGain Systems


On Thu, Aug 8, 2019 at 7:52 AM Maxim Muzafarov <maxmuzaf@gmail.com> wrote:

> Ivan,
>
> It is not a problem to check Javadocs at the moment code syle checking
> performed, but do we really need this? And the human-factor you
> mentioned above is also related to the `self-descriptive` names. I
> assume, that someone now is desiring to use single-letter variables
> and single-letter class names to save space an time. We will always
> have such an opinion race.
>
> I'd prefer to leave the current situation with Javadoc as it is and
> just to ask to apply the patch [1][2]. Can you? :-)
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12051
> [2] https://github.com/apache/ignite/pull/6760
>
> On Thu, 8 Aug 2019 at 17:24, Павлухин Иван <vololo100@gmail.com> wrote:
> >
> > Maxim,
> >
> > My main concern here is a human factor. Humans are usually not so good
> > in keeping documentation always in sync with a code. Examples from an
> > actual PR:
> > /**
> > * @param nodeId The remote node id.
> > * @param channel The channel to notify listeners with.
> > */
> > private void onChannelOpened0(UUID nodeId, GridIoMessage initMsg,
> > Channel channel)
> >
> > First, there is a mismatch between number of parameters in javadoc and
> > code. Second, e.g. nodeId name can be made self-descriptive rmtNodeId
> > name.
> >
> > Mandatory javadocs do not imply good javadocs. Good javadocs do not
> > imply javadocs for every method and field. For me, mandatory and good
> > javadocs are like communism. Sounds quite nice in theory, but not
> > feasible in practice.
> >
> > чт, 8 авг. 2019 г. в 16:55, Denis Garus <garus.d.g@gmail.com>:
> > >
> > > Thank you, Maxim.
> > >
> > > >>On what we are trying to save by making Javadoc optional?
> > >
> > > Space and time.
> > >
> > > I doubt that we need a verbal description of what private method make.
> > > Why we need it if we just can read the code?
> > >
> > > Bright examples:
> > >
> > > /**
> > > * @param helper Helper to attach to kernal context.
> > > */
> > > private void addHelper(Object helper) {
> > >     ctx.addHelper(helper);
> > > }
> > >
> > > /**
> > > * Gets "on" or "off" string for given boolean value.
> > > *
> > > * @param b Boolean value to convert.
> > > * @return Result string.
> > > */
> > > private String onOff(boolean b) {
> > >     return b ? "on" : "off";
> > > }
> > >
> > > You have to support both code of method and their JavaDoc description,
> but
> > > it doesn't get any sake.
> > >
> > > >>Let's just help them to read the source code.
> > >
> > > But at the same time, public method IgniteKernal#start doesn't have any
> > > description except enumeration of attributes with apparent notes.
> > > Maybe to give it a verbal description needs one or two pages of the
> screen.
> > > Does it make sense?
> > >
> > > чт, 8 авг. 2019 г. в 15:41, Maxim Muzafarov <maxmuzaf@gmail.com>:
> > >
> > > > Igniters,
> > > >
> > > > I'm -1 with making Javadoc optional (except tests).
> > > >
> > > > Here is my patch [1] with PR [2] which fixes all the Javadoc comments
> > > > for the IgniteKernal class mentioned above. Please, take a look. The
> > > > review is very appreciated.
> > > >
> > > > On what we are trying to save by making Javadoc optional? In my
> humble
> > > > opinion - Javadoc comments are mostly concern users who debug the
> > > > Ignite when they have faced with some unhandled exception or related
> > > > to the community members with an average involvement (produces a few
> > > > small patches during the year) but not to the experienced one. Let's
> > > > just help them to read the source code.
> > > >
> > > > [1] https://issues.apache.org/jira/browse/IGNITE-12051
> > > > [2] https://github.com/apache/ignite/pull/6760
> > > >
> > > >
> > > >
> > > > On Wed, 7 Aug 2019 at 13:54, Andrey Kuznetsov <stkuzma@gmail.com>
> wrote:
> > > > >
> > > > > +1 for making javadoc comments optional.
> > > > >
> > > > > - Empty and tautological comments are kind of garbage that reduce
> > > > > readability.
> > > > > - It's better to leave the entity undocumented, than write
> > > > > unexpressive/misleading comment.
> > > > > - Even classes may not require javadocs, e.g. simple DTOs.
> > > > >
> > > > > ср, 7 авг. 2019 г., 13:39 Denis Garus <garus.d.g@gmail.com>:
> > > > >
> > > > > > Thx for feedback!
> > > > > >
> > > > > > >> we have to write proper javadoc for all production
classes,
> > > > including
> > > > > > internal.
> > > > > >
> > > > > > Nikolay, I cannot agree with it.
> > > > > >
> > > > > > What should be the best comment for the next fields?
> > > > > > /** */
> > > > > > private static final long serialVersionUID = 0L;
> > > > > > or
> > > > > > /** */
> > > > > > @LoggerResource
> > > > > > private IgniteLogger log;
> > > > > >
> > > > > > There are more than 8000 lines of /** */ only at the ignite-core
> > > > module (do
> > > > > > not include tests).
> > > > > >
> > > > > > Any comments will be redundant and just noise. Obvious comment
> learns
> > > > > > readers skip all comments.
> > > > > >
> > > > > >
> > > > > > >> identical distance/padding/margin between fields in
a class -
> is
> > > > really
> > > > > > cool
> > > > > >
> > > > > > Vyacheslav, but we have a blank line between fields. Why is
one
> blank
> > > > line
> > > > > > not enough?
> > > > > >
> > > > > > ср, 7 авг. 2019 г. в 12:58, Павлухин Иван
<vololo100@gmail.com>:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > Denis, thank you for starting this discussion!
> > > > > > >
> > > > > > > My opinion here is that having a good javadoc for every
class
> and
> > > > > > > method is not feasible in the real world. I am quite curious
> to see a
> > > > > > > non-trivial project which follows it. Also, all comments
and
> javadocs
> > > > > > > are prone to become misleading when code changes (human
> factor). In
> > > > my
> > > > > > > experience good method and variable names and clear code
> organization
> > > > > > > often are more helpful than javadocs.
> > > > > > >
> > > > > > > ср, 7 авг. 2019 г. в 12:49, Vyacheslav Daradur <
> daradurvs@gmail.com
> > > > >:
> > > > > > > >
> > > > > > > > I agree that useless comments look weird in the codebase.
> > > > > > > >
> > > > > > > > But, identical distance/padding/margin between fields
in a
> class -
> > > > is
> > > > > > > > really cool, and helps read the class very fast.
> > > > > > > >
> > > > > > > > On Wed, Aug 7, 2019 at 12:26 PM Nikolay Izhikov <
> > > > nizhikov@apache.org>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello, Denis.
> > > > > > > > >
> > > > > > > > > Thanks for starting this discussion.
> > > > > > > > >
> > > > > > > > > I think we have to write proper javadoc for all
production
> > > > classes,
> > > > > > > including internal.
> > > > > > > > > We should fix useless javadoc you provide.
> > > > > > > > > We should not accept patches without good javadocs.
> > > > > > > > >
> > > > > > > > > As for the tests, seems, we can make javadoc
optional.
> > > > > > > > >
> > > > > > > > > What do you think?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > В Ср, 07/08/2019 в 11:41 +0300, Denis Garus
пишет:
> > > > > > > > > > Igniters!
> > > > > > > > > >
> > > > > > > > > > I think it's time to change coding guidelines
in part of
> > > > JavaDoc
> > > > > > > comments
> > > > > > > > > > [1]:
> > > > > > > > > > > > Every method, field or initializer
public, private or
> > > > protected
> > > > > > > in
> > > > > > > > > >
> > > > > > > > > > top-level,
> > > > > > > > > > > > inner or anonymous type should
have at least minimal
> > > > Javadoc
> > > > > > > comments
> > > > > > > > > >
> > > > > > > > > > including
> > > > > > > > > > > > description and description of
parameters using
> @param,
> > > > @return
> > > > > > > and
> > > > > > > > > >
> > > > > > > > > > @throws Javadoc tags,
> > > > > > > > > > > > where applicable.
> > > > > > > > > >
> > > > > > > > > > Let's look at JavaDoc comments in the IgniteKernal
class:
> > > > > > > > > >
> > > > > > > > > > Why?
> > > > > > > > > >
> > > > > > > > > > /** */ - 15 matches.
> > > > > > > > > >
> > > > > > > > > > What can you get new from these comments?
> > > > > > > > > >
> > > > > > > > > > /** Periodic starvation check interval.
*/
> > > > > > > > > > private static final long PERIODIC_STARVATION_CHECK_FREQ
> =
> > > > 1000 *
> > > > > > 30;
> > > > > > > > > >
> > > > > > > > > > /** Long jvm pause detector. */
> > > > > > > > > > private LongJVMPauseDetector longJVMPauseDetector;
> > > > > > > > > >
> > > > > > > > > > /** Scheduler. */
> > > > > > > > > >     private IgniteScheduler scheduler;
> > > > > > > > > >
> > > > > > > > > > /** Stop guard. */
> > > > > > > > > >     private final AtomicBoolean stopGuard
= new
> > > > AtomicBoolean();
> > > > > > > > > >
> > > > > > > > > > /**
> > > > > > > > > >      * @param cfg Configuration to use.
> > > > > > > > > >      * @param utilityCachePool Utility cache
pool.
> > > > > > > > > >      * @param execSvc Executor service.
> > > > > > > > > >      * @param sysExecSvc System executor
service.
> > > > > > > > > >      * @param stripedExecSvc Striped executor.
> > > > > > > > > >      * @param p2pExecSvc P2P executor service.
> > > > > > > > > >      * @param mgmtExecSvc Management executor
service.
> > > > > > > > > >      * @param igfsExecSvc IGFS executor
service.
> > > > > > > > > >      * @param dataStreamExecSvc data stream
executor
> service.
> > > > > > > > > >      * @param restExecSvc Reset executor
service.
> > > > > > > > > >      * @param affExecSvc Affinity executor
service.
> > > > > > > > > >      * @param idxExecSvc Indexing executor
service.
> > > > > > > > > >      * @param callbackExecSvc Callback executor
service.
> > > > > > > > > >      * @param qryExecSvc Query executor
service.
> > > > > > > > > >      * @param schemaExecSvc Schema executor
service.
> > > > > > > > > >      * @param customExecSvcs Custom named
executors.
> > > > > > > > > >      * @param errHnd Error handler to use
for
> notification
> > > > about
> > > > > > > startup
> > > > > > > > > > problems.
> > > > > > > > > >      * @param workerRegistry Worker registry.
> > > > > > > > > >      * @param hnd Default uncaught exception
handler
> used by
> > > > thread
> > > > > > > pools.
> > > > > > > > > >      * @throws IgniteCheckedException Thrown
in case of
> any
> > > > errors.
> > > > > > > > > >      */
> > > > > > > > > >     public void start(
> > > > > > > > > >         final IgniteConfiguration cfg,
> > > > > > > > > >         ExecutorService utilityCachePool,
> > > > > > > > > >         final ExecutorService execSvc,
> > > > > > > > > >         final ExecutorService svcExecSvc,
> > > > > > > > > >         final ExecutorService sysExecSvc,
> > > > > > > > > >         final StripedExecutor stripedExecSvc,
> > > > > > > > > >         ExecutorService p2pExecSvc,
> > > > > > > > > >         ExecutorService mgmtExecSvc,
> > > > > > > > > >         ExecutorService igfsExecSvc,
> > > > > > > > > >         StripedExecutor dataStreamExecSvc,
> > > > > > > > > >         ExecutorService restExecSvc,
> > > > > > > > > >         ExecutorService affExecSvc,
> > > > > > > > > >         @Nullable ExecutorService idxExecSvc,
> > > > > > > > > >         IgniteStripedThreadPoolExecutor
callbackExecSvc,
> > > > > > > > > >         ExecutorService qryExecSvc,
> > > > > > > > > >         ExecutorService schemaExecSvc,
> > > > > > > > > >         @Nullable final Map<String, ?
extends
> ExecutorService>
> > > > > > > > > > customExecSvcs,
> > > > > > > > > >         GridAbsClosure errHnd,
> > > > > > > > > >         WorkersRegistry workerRegistry,
> > > > > > > > > >         Thread.UncaughtExceptionHandler
hnd,
> > > > > > > > > >         TimeBag startTimer
> > > > > > > > > >     )
> > > > > > > > > >
> > > > > > > > > > These comments look ugly and useless, and
that is the
> main
> > > > class of
> > > > > > > core.
> > > > > > > > > > Why do they need us?
> > > > > > > > > > Let us change the coding guidelines in part
of JavaDoc
> > > > comments:
> > > > > > > > > > Every method public API should have at least
minimal
> Javadoc
> > > > > > > comments,
> > > > > > > > > > including description and description of
parameters using
> > > > @param,
> > > > > > > @return,
> > > > > > > > > > and @throws Javadoc tags,
> > > > > > > > > > where applicable.
> > > > > > > > > > For internal API, JavaDoc comments should
be optional.
> It's up
> > > > to a
> > > > > > > > > > contributor or reviewer.
> > > > > > > > > >
> > > > > > > > > > What are you think?
> > > > > > > > > >
> > > > > > > > > > 1.
> > > > > > > > > >
> > > > > > >
> > > > > >
> > > >
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-JavadocComments
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Best Regards, Vyacheslav D.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Best regards,
> > > > > > > Ivan Pavlukhin
> > > > > > >
> > > > > >
> > > >
> >
> >
> >
> > --
> > Best regards,
> > Ivan Pavlukhin
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message