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....@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.govoruk...@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 <voze...@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.govoruk...@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 <voze...@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.shangar...@gmail.com> wrote: >> > > > >> > > > > Alexey, >> > > > > +1. >> > > > > I personally also follow this style. >> > > > > On Thu, May 3, 2018 at 12:45 PM, Alexey Goncharuk < >> > > > > alexey.goncha...@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....@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.govoruk...@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. >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >> >>
