ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitry Pavlov <dpavlov....@gmail.com>
Subject Re: method arguments code style
Date Mon, 14 May 2018 17:46:51 GMT
Hi Vyacheslav,

plugin was published in AI wiki, feel free to create ticket to add method
code style check.

Actually I'm not sure it is easy to validate code style in plugin, but I
guess you know it better.

Sincerely,
Dmitriy Pavlov

вт, 8 мая 2018 г. в 21:47, Vyacheslav Daradur <daradurvs@gmail.com>:

> Hi, Igniters!
>
> After the completion of publishing abbr-plugin [1][2] we will be able
> to automate checking of method arguments code style.
>
> It will be easy to check rules approved by the community during writing
> code.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-5698
> [2]
> http://apache-ignite-developers.2346864.n4.nabble.com/abbrevation-rules-plugin-td19356.html
>
> On Tue, May 8, 2018 at 8:34 PM, Dmitry Pavlov <dpavlov.spb@gmail.com>
> wrote:
> > Folks, I've messed with another topic, where Vladimir was going to update
> > review check-list.
> >
> > Here I've updated Coding Guidelines:
> >
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-MethodArguments
> >
> > Please review changes, so we can consider it is final.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > вт, 8 мая 2018 г. в 20:05, Dmitry Pavlov <dpavlov.spb@gmail.com>:
> >
> >> I thought that Vladimir will update.
> >>
> >> By the way, Denis M, I propose to grant access to the wiki to Dmitry G.
> >> WDYT?
> >>
> >> вт, 8 мая 2018 г. в 19:28, Dmitriy Govorukhin <
> >> dmitriy.govorukhin@gmail.com>:
> >>
> >>> Dmitriy,
> >>>
> >>> Сould you please update code style wiki page in accordance with the
> >>> results of the discussion?
> >>> On May 7 2018, at 11:00 am, Vladimir Ozerov <vozerov@gridgain.com>
> wrote:
> >>> >
> >>> > Dmitry,
> >>> > Agree, mixed style when some arguments share the same line and others
> >>> don't
> >>> > looks very bad. My proposal was to allow two styles - first when all
> >>> > arguments are on the same line splitted by 120 char limit, second
> when
> >>> all
> >>> > every arguments is on a separate line.
> >>> > Mixed style should be disallowed.
> >>> >
> >>> > On Mon, May 7, 2018 at 12:35 AM, Dmitriy Govorukhin <
> >>> > dmitriy.govorukhin@gmail.com> wrote:
> >>> >
> >>> > > Vladimir,
> >>> > > My eyes cry when I see this
> >>> > > public double getCost(Session ses, int[] masks, TableFilter[]
> filters,
> >>> > > int filter, SortOrder sortOrder,
> >>> > > HashSet<Column> cols) {
> >>> > > return SpatialTreeIndex.getCostRangeIndex(masks,table.
> >>> > > getRowCountApproximation(),
> >>> > > columns) / 10;
> >>> > > }
> >>> > >
> >>> > > Why did arguments split into 3 line?
> >>> > > It is much better readable for me
> >>> > > public double getCost(
> >>> > > Session ses,
> >>> > > int[] masks,
> >>> > > TableFilter[] filters,
> >>> > > int filter,
> >>> > > SortOrder sortOrder,
> >>> > > HashSet<Column> cols
> >>> > > ) {
> >>> > > return SpatialTreeIndex.getCostRangeIndex(masks,
> >>> > > able.getRowCountApproximation(),
> >>> > > columns) / 10;
> >>> > > }
> >>> > >
> >>> > > Do we have a serious reason to continue writing code as before?
> >>> > >
> >>> > >
> >>> > > On Thu, May 3, 2018 at 2:24 PM, Vladimir Ozerov <
> vozerov@gridgain.com
> >>> >
> >>> > > wrote:
> >>> > >
> >>> > > > My opinion is that we should allow both styles and not enforce
> any
> >>> of
> >>> > > them.
> >>> > > > I hardly can say that this
> >>> > > >
> >>> > > > public double getCost(
> >>> > > > Session ses,
> >>> > > > int[] masks,
> >>> > > > TableFilter[] filters,
> >>> > > > int filter,
> >>> > > > SortOrder sortOrder,
> >>> > > > HashSet<Column> cols
> >>> > > > ) {
> >>> > > > return SpatialTreeIndex.getCostRangeIndex(masks,
> >>> > > > table.getRowCountApproximation(), columns) / 10;
> >>> > > > }
> >>> > > >
> >>> > > > is better than this
> >>> > > > public double getCost(Session ses, int[] masks, TableFilter[]
> >>> filters,
> >>> > > > int filter, SortOrder sortOrder,
> >>> > > > HashSet<Column> cols) {
> >>> > > > return SpatialTreeIndex.getCostRangeIndex(masks,
> >>> > > > table.getRowCountApproximation(), columns) / 10;
> >>> > > > }
> >>> > > >
> >>> > > >
> >>> > > > But this
> >>> > > > public CacheLockCandidates doneRemote(
> >>> > > > GridCacheVersion ver,
> >>> > > > Collection<GridCacheVersion> pending,
> >>> > > > Collection<GridCacheVersion> committed,
> >>> > > > Collection<GridCacheVersion> rolledback
> >>> > > > )
> >>> > > >
> >>> > > >
> >>> > > > looks better than this
> >>> > > > public CacheLockCandidates doneRemote(GridCacheVersion ver,
> >>> > > > Collection<GridCacheVersion> pending,
> >>> > > > Collection<GridCacheVersion> committed,
> >>> > > > Collection<GridCacheVersion> rolledback)
> >>> > > >
> >>> > > >
> >>> > > > The very problem is that our eyes feel comfortable when we
either
> >>> move
> >>> > > them
> >>> > > > horizontally, or vertically (example 2). But with proposed
code
> >>> style you
> >>> > > > have to do zigzag movements in general case because lines
are not
> >>> aligned
> >>> > > > (example 1).
> >>> > > > Merge conflicts on multiliners are hardly of major concern
for
> us.
> >>> > > >
> >>> > > > Vladimir.
> >>> > > > On Thu, May 3, 2018 at 1:46 PM, Eduard Shangareev <
> >>> > > > eduard.shangareev@gmail.com> wrote:
> >>> > > >
> >>> > > > > Alexey,
> >>> > > > > +1.
> >>> > > > > I personally also follow this style.
> >>> > > > > On Thu, May 3, 2018 at 12:45 PM, Alexey Goncharuk <
> >>> > > > > alexey.goncharuk@gmail.com> wrote:
> >>> > > > >
> >>> > > > > > Actually, I've been following the suggested code
style for
> >>> quite a
> >>> > > > while.
> >>> > > > > > I'm ok to add this to coding guidelines, however,
I think we
> >>> should
> >>> > > > >
> >>> > > >
> >>> > > > allow
> >>> > > > > > the old style when the method signature (without
throws
> clause)
> >>> fits
> >>> > > > >
> >>> > > >
> >>> > > > the
> >>> > > > > > line.
> >>> > > > > >
> >>> > > > > > Thoughts?
> >>> > > > > > 2018-05-03 12:09 GMT+03:00 Dmitry Pavlov <
> dpavlov.spb@gmail.com
> >>> >:
> >>> > > > > > > Hi Dmitriy,
> >>> > > > > > > I like your proposal, so +1 from me.
> >>> > > > > > > I think it would make code more readable and
easy to
> >>> understand.
> >>> > > > > > > Sincerely,
> >>> > > > > > > Dmitriy Pavlov
> >>> > > > > > >
> >>> > > > > > > чт, 3 мая 2018 г. в 11:31, Dmitriy
Govorukhin <
> >>> > > > > > > dmitriy.govorukhin@gmail.com
> >>> > > > > > > > :
> >>> > > > > > >
> >>> > > > > > >
> >>> > > > > > > > Hi folks,
> >>> > > > > > > > I read
> >>> > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > > > Coding+Guidelines
> >>> > > > > ,
> >>> > > > > > > > but did not find anything about code
style for method
> >>> arguments.
> >>> > > > > > > >
> >>> > > > > > > > In many places in the code, I see different
code style,
> this
> >>> > > > creates
> >>> > > > > > > > difficulties for reading.
> >>> > > > > > > >
> >>> > > > > > > > It seems to me an example below is rather
difficult to
> >>> perceive.
> >>> > > > > > > > ```java
> >>> > > > > > > > public void foo(Object obj1,
> >>> > > > > > > > Object obj2, Object obj3,... ,Object
objN){
> >>> > > > > > > > ....
> >>> > > > > > > > }
> >>> > > > > > > > ```
> >>> > > > > > > > An example GridCacheProcessor.addCacheOnJoin(...)
> >>> > > > > > > >
> >>> > > > > > > > ```java
> >>> > > > > > > > private void addCacheOnJoin(CacheConfiguration<?,
?> cfg,
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > > > boolean
> >>> > > > > > > sql,
> >>> > > > > > > > Map<String, CacheInfo> caches,
> >>> > > > > > > > Map<String, CacheInfo> templates)
> >>> > > > > > > > ```
> >>> > > > > > > > I suggest two options for writing arguments.
> >>> > > > > > > >
> >>> > > > > > > > If arguments are placed in a line before
the page
> delimiter.
> >>> > > > > > > > ```java
> >>> > > > > > > > public void foo(Object obj1, Object obj2,
Object obj3 ,
> ...
> >>> ,
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > > > Object
> >>> > > > > > > objN){
> >>> > > > > > > > ....
> >>> > > > > > > > }
> >>> > > > > > > > ```
> >>> > > > > > > > If the arguments are not placed in the
line before the
> page
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > > > delimiter.
> >>> > > > > > > >
> >>> > > > > > > > ```java
> >>> > > > > > > > public void foo(
> >>> > > > > > > > Object obj1,
> >>> > > > > > > > Object obj2,
> >>> > > > > > > > Object obj3,
> >>> > > > > > > > ... ,
> >>> > > > > > > > Object objN
> >>> > > > > > > > ){
> >>> > > > > > > > ....
> >>> > > > > > > > }
> >>> > > > > > > > ```
> >>> > > > > > > > In my personal experience, the last example
is much
> easier
> >>> to
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> > > merge
> >>> > > > > if
> >>> > > > > > > > method arguments were changed.
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>> >
> >>>
> >>>
>
>
>
> --
> Best Regards, Vyacheslav D.
>

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