ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Павлухин Иван <vololo...@gmail.com>
Subject Re: Coding guidelines. Useless JavaDoc comments.
Date Sun, 11 Aug 2019 06:13:36 GMT
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