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....@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....@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. >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> > >>> >>> -- Best Regards, Vyacheslav D.
