ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nikolay Izhikov <nizhi...@apache.org>
Subject Re: Coding guidelines. Useless JavaDoc comments.
Date Thu, 15 Aug 2019 07:15:30 GMT
Hello, Anton.

> I'd like to propose to have only non-obvious params explained at
> non-public method's Javadoc.

Non-obvious for whom?
Please, remember about guys who just came into Ignite community.




В Чт, 15/08/2019 в 10:05 +0300, Anton Vinogradov пишет:
> My +1 to optional Javadoc at private methods, fields, and classes.
> The reviewer should guaranty that everything is clear.
> But, in case everything is clear without redundant Javadoc there is no need
> to have it.
> 
> So, my proposal is to allow /** */ for non-public fields and methods (to
> keep readability between documented and obvious fields/methods).
> Also, I'd like to propose to have only non-obvious params explained at
> non-public method's Javadoc.
> 
> On Sun, Aug 11, 2019 at 9:13 AM Павлухин Иван <vololo100@gmail.com>
wrote:
> 
> > Maxim,
> > 
> > > 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? :-)
> > 
> > You caught me. I left my comments for that PR
> > 
> > .Pavel and all,
> > 
> > > I think that API of our core internal things like PageMemory, WAL, all
> > 
> > existing managers and processors should be as well documented as possible.
> > 
> > No doubts here.
> > 
> > > Documentation should be a result of a proper code review.
> > 
> > I suppose it does not mean that documentation should be written after
> > a review. I suppose it means that we should not have a poor
> > documentation *after* a review. Overall, Pavel's last message conforms
> > well with my opinion on the subject.
> > 
> > чт, 8 авг. 2019 г. в 18:34, Pavel Kovalenko <jokserfn@gmail.com>:
> > > 
> > > I can agree that some part of javadocs we have is useless. It relates to
> > > DTOs, getters/setters without side-effects, short self-descriptive
> > 
> > methods.
> > > In an ideal world, proper modularization of architecture, leading to
> > > KISS/SOLID/DRY/etc. principles, writing self-documented code should
> > 
> > result
> > > in javadocs disappearing, as they become not needed.
> > > We live in a not ideal world. We don't have good architecture and can't
> > > always lead to mentioned principles, because we need sometimes sacrifice
> > > readability for optimization, fixing a critical bug, etc.
> > > I think that API of our core internal things like PageMemory, WAL, all
> > > existing managers and processors should be as well documented as
> > 
> > possible.
> > > If a developer uses some module / manager / processor without looking
> > > inside, reading the only description of public methods, it's a good sign
> > > that this part of the functionality is well documented.
> > > Internal implementation should be also clear for a developer who likes to
> > > make a change inside it. Every optimization, avoiding race-condition, not
> > > obvious thing and especially crutch must be documented as detailed as
> > > possible.
> > > Documentation should be a result of a proper code review. If a reviewer
> > 
> > has
> > > questions regarding any code line it should be either refactored to make
> > > this thing obvious or well documented.
> > > If a class or method is self-documented and obvious there is no need to
> > > document it anyway.
> > > if each person takes the code review as seriously as possible,
> > > documentation and code will be better automatically.
> > > Mandatory documentation in places where it's really not needed looks
> > 
> > like a
> > > burden. A developer will avoid write it properly everywhere or do it
> > 
> > "just
> > > for check" and this will influence on documentation with the negative
> > 
> > side.
> > > Flexible approach with mandatory / optional javadocs with good code
> > 
> > review
> > > will result in readability improvement overall.
> > > 
> > > 
> > > чт, 8 авг. 2019 г. в 17:52, Maxim Muzafarov <maxmuzaf@gmail.com>:
> > > 
> > > > 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
> > 
> > 
> > 
> > --
> > Best regards,
> > Ivan Pavlukhin
> > 

Mime
View raw message