ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vyacheslav Daradur <daradu...@gmail.com>
Subject Re: method arguments code style
Date Wed, 16 May 2018 12:58:17 GMT
Hi, I filed the ticket https://issues.apache.org/jira/browse/IGNITE-8512

I'll do this when I have enough time if it won't be done earlier.

On Mon, May 14, 2018 at 8:46 PM, Dmitry Pavlov <dpavlov.spb@gmail.com> wrote:
> 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.
>>



-- 
Best Regards, Vyacheslav D.

Mime
View raw message