ignite-dev mailing list archives

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

Params like
* @param tx Cache transaction.
* @param val Value to set.
* @param oldVal Old value.
* @param topVer Topology version.
* @param taskName Task name.
seems to be obvious to everyone.
No reason to hove the documented.

In case you doubt it obvious, just document it.

On Thu, Aug 15, 2019 at 10:12 AM Nikolay Izhikov <nizhikov@apache.org>
wrote:

> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message