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 Tue, 08 May 2018 17:34:37 GMT
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.
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> >
>>
>>

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