ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Denis Garus <garus....@gmail.com>
Subject Re: Coding guidelines. Useless JavaDoc comments.
Date Thu, 08 Aug 2019 13:54:41 GMT
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
> > > >
> > >
>

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