Thread: PgJDBC: code reformat

Hi,

I would like to raise a discussion on coding guidelines.
As discussed, single style is good.

As there are no suggestions, I went ahead and tried "Google style, 100
characters per line".
Here are some of the results: [1], [2], [3]
I've spent ~3 hours to achieve that, notable part of which was
figuring out how to configure checkstyle.

What works:
0) IDEA reformat gives checkstyle-compatible code
1) IDEA checkstyle plugin can show violations
2) mvn checkstyle:check
3) Travis

Please vote:
+1 [ ] The style is wonderful
+0 [ ] It is good enough
-0 [ ] Generally it is ok, but ...
-1 [ ] I do not like it since ...

Google style basically means:
1) K&R Egyptian brackets
2) Line break is inserted _before_ binary operators.
For instance:
   "line 1"
   + "line 2"
3) Every variable should be declared on its own line. In other words,
"int a,b;" is forbidden
4) Certain variable & method naming conventions (<-- I have not yet
looked into this)

My idea is to see how far IDEA can reformat the code to please
checkstyle validator.
I've taken checkstyle configuration from their official sample (see
[5]) and disabled certain checks (the ones that cannot be applied
automatically like variable naming conventions).

There are still some checkstyle warnings (I think they should be
converted to errors): [4]

[1] https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
[2] https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/AbstractBlobClob.java
[3] https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java
[4] https://travis-ci.org/pgjdbc/pgjdbc/jobs/98907638
[5] https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml

--
Regards,
Vladimir Sitnikov

-0 [X] I would rather go with Sun style instead of Google style; it is contained in Eclipse as a selectable option, so
Idon't have to configure my IDE at all but can rely on auto-format on saving a file. :-) 

-Markus

-----Original Message-----
From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Vladimir Sitnikov
Sent: Samstag, 26. Dezember 2015 22:41
To: List
Subject: [JDBC] PgJDBC: code reformat

Hi,

I would like to raise a discussion on coding guidelines.
As discussed, single style is good.

As there are no suggestions, I went ahead and tried "Google style, 100 characters per line".
Here are some of the results: [1], [2], [3] I've spent ~3 hours to achieve that, notable part of which was figuring out
howto configure checkstyle. 

What works:
0) IDEA reformat gives checkstyle-compatible code
1) IDEA checkstyle plugin can show violations
2) mvn checkstyle:check
3) Travis

Please vote:
+1 [ ] The style is wonderful
+0 [ ] It is good enough
-0 [ ] Generally it is ok, but ...
-1 [ ] I do not like it since ...

Google style basically means:
1) K&R Egyptian brackets
2) Line break is inserted _before_ binary operators.
For instance:
   "line 1"
   + "line 2"
3) Every variable should be declared on its own line. In other words, "int a,b;" is forbidden
4) Certain variable & method naming conventions (<-- I have not yet looked into this)

My idea is to see how far IDEA can reformat the code to please checkstyle validator.
I've taken checkstyle configuration from their official sample (see
[5]) and disabled certain checks (the ones that cannot be applied automatically like variable naming conventions).

There are still some checkstyle warnings (I think they should be converted to errors): [4]

[1] https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
[2] https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/AbstractBlobClob.java
[3] https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java
[4] https://travis-ci.org/pgjdbc/pgjdbc/jobs/98907638
[5] https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml

--
Regards,
Vladimir Sitnikov


--
Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-jdbc

I've fixed remaining errors: https://github.com/pgjdbc/pgjdbc/tree/format_code

Vladimir

>I tend to break up multiple lines.

"Mandatory brackets" plus "open curly on the new line" would result in
too much whitespace.
We do want "mandatory brackets", don't we?

Regarding whitespace, what if using explicit blank lines to separate
logic when required?
Like this:
https://github.com/pgjdbc/pgjdbc/blob/format_code/pgjdbc/src/main/java/org/postgresql/jdbc/PgSQLXML.java#L90-L94

>There doesn't seem to be a significant difference between sun style and google style.

Technically speaking, there is.

Sun style:

if (data == null)
{
  return null;
}

Google style (this one is used by pgjdbc-ng):

if (data == null) {
  return null;
}

Vladimir

On 28/12/15 01:15, Dave Cramer wrote:
> There doesn't seem to be a significant difference between sun style
> and google style.
> Personally I'd have a lot more whitespace than either of these styles.
>
> I tend to break up multiple lines.
>
> I prefer: if ()
>              {
>                  do something
>              }
>
> vs if(){
>          do something
>      }
>
>
> but it would appear this is not generally accepted.
>
> At the end of the day I'd vote
>
> 0 as well unless I could get enough support behind more whitespace
>
>
> Dave Cramer
>
> davec@postgresintl.com <mailto:davec@postgresintl.com>
> www.postgresintl.com <http://www.postgresintl.com>
>
> On 27 December 2015 at 04:04, Vladimir Sitnikov
> <sitnikov.vladimir@gmail.com <mailto:sitnikov.vladimir@gmail.com>> wrote:
>
>     I've fixed remaining errors:
>     https://github.com/pgjdbc/pgjdbc/tree/format_code
>
>     Vladimir
>
>
>     --
>     Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org
>     <mailto:pgsql-jdbc@postgresql.org>)
>     To make changes to your subscription:
>     http://www.postgresql.org/mailpref/pgsql-jdbc
>
>
I much prefer to line the brackets up on the same side, it makes the
code a lot easier to read. Even in the days of screens with 25 lines of
80 characters - now with considerably larger monitors, it makes even
more sense.

So I very much prefer the style:

if (q)
{
     do something
}


When I had poor eyesight it was easier, and after multiple eye
operations & my eyesight is much improved - I still prefer the style
with the brackets on separate lines.

If I want to understand someone else's code sample; I refactor a copy of
their code into this style, as it greatly increases the ease of reading
it. The other style looks cluttered, and is harder to see the blocks of
code clearly


Cheers,
Gavin

On 28/12/15 03:03, Dave Cramer wrote:
> +1 for Gavin's suggestion
> and yes I think mandatory braces are required

When I started programming (back in 1968), I thought that indenting and
the like was a waste of time, as I had to explicitly punch every single
space onto punched cards,  But as my programs became more complicated, I
found my code hard to read (even with my then good eyesight). Later when
I started indenting, I found my code far easier to read & understand, so
I am extremely conscious of the value of white space - also very glad
the modern IDE's don't force you to type every single space and manually
assure that code is indented properly!!!

I always use brackets for if statements, as it make it much easier to
add additional lines without accidentally introducing a programming bug.


Cheers,
Gavin

>
> Dave Cramer
>
> davec@postgresintl.com <mailto:davec@postgresintl.com>
> www.postgresintl.com <http://www.postgresintl.com>
>
> On 27 December 2015 at 09:01, Gavin Flower
> <GavinFlower@archidevsys.co.nz <mailto:GavinFlower@archidevsys.co.nz>>
> wrote:
>
>     On 28/12/15 01:15, Dave Cramer wrote:
>
[...]
>
>     I much prefer to line the brackets up on the same side, it makes
>     the code a lot easier to read. Even in the days of screens with 25
>     lines of 80 characters - now with considerably larger monitors, it
>     makes even more sense.
>
>     So I very much prefer the style:
>
>     if (q)
>     {
>         do something
>     }
>
>
>     When I had poor eyesight it was easier, and after multiple eye
>     operations & my eyesight is much improved - I still prefer the
>     style with the brackets on separate lines.
>
>     If I want to understand someone else's code sample; I refactor a
>     copy of their code into this style, as it greatly increases the
>     ease of reading it. The other style looks cluttered, and is harder
>     to see the blocks of code clearly
>
>
>     Cheers,
>     Gavin
>
>
>
>

I did try reformatting with "opening braces on new line".
It turned out it is incompatible with checkstyle:
https://travis-ci.org/pgjdbc/pgjdbc/jobs/98976190

There are 17 "... have incorrect indentation level" failures.
Most of which are due to enum initialization in

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/hostchooser/HostRequirement.java#L16-L29

I have just two requirements:
1) IDEA's autoformat should be able to produce the desired style
(minimal modifications are bearable)
2) The style should be enforced, in other words, checkstyle (or
whatever tools is used) should be able to catch most of the styling
issues.

As Garvin/Dave/Markus vote for "braces on new line", would you please
suggest how to fix checkstyle errors?
For instance:
/home/travis/build/pgjdbc/pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java:2439:
error: 'object def lcurly' have incorrect indentation level 6,
expected level should be one of the following: 2, 4.

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2439


PS.
I do not care where the braces are.
However, this makes me cry:

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/Driver.java#L353-L373
There are probably better ways of writing that, but those "new lines
for opening brackets" make it much worse.


PPS.
>The other style looks cluttered, and is harder to see

Gavin,
I feel your pain, however I still think you are trying to solve "badly
written code" with "reformat code" approach.
That just does not work.

Compare this one:

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2462-L2512
with this one:
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L264-L311

Do you see how QueryExecutorImpl.java#L264-L311 is much readable?

PgResultSet#getFastLong is very hard to follow no matter which way you
format the braces.
I believe, "readability" comes from proper segmentation (code blocks
vs methods) and proper variable naming.

Vladimir

I actually did NOT vote for "braces on new line". In fact I hate that style and always have opening braces on the SAME
line.What I actually said was that if we decide for a near-Sun-Style it makes more sense to go with pure-sun-Style
instead,as that is built into IDEs already -- even if it produces a style a do not like much. 
-Markus

-----Original Message-----
From: Vladimir Sitnikov [mailto:sitnikov.vladimir@gmail.com]
Sent: Sonntag, 27. Dezember 2015 16:02
To: Gavin Flower; Dave Cramer; Markus KARG
Cc: List
Subject: Re: [JDBC] PgJDBC: code reformat

I did try reformatting with "opening braces on new line".
It turned out it is incompatible with checkstyle:
https://travis-ci.org/pgjdbc/pgjdbc/jobs/98976190

There are 17 "... have incorrect indentation level" failures.
Most of which are due to enum initialization in

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/hostchooser/HostRequirement.java#L16-L29

I have just two requirements:
1) IDEA's autoformat should be able to produce the desired style (minimal modifications are bearable)
2) The style should be enforced, in other words, checkstyle (or whatever tools is used) should be able to catch most of
thestyling issues. 

As Garvin/Dave/Markus vote for "braces on new line", would you please suggest how to fix checkstyle errors?
For instance:
/home/travis/build/pgjdbc/pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java:2439:
error: 'object def lcurly' have incorrect indentation level 6, expected level should be one of the following: 2, 4.

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2439


PS.
I do not care where the braces are.
However, this makes me cry:

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/Driver.java#L353-L373
There are probably better ways of writing that, but those "new lines for opening brackets" make it much worse.


PPS.
>The other style looks cluttered, and is harder to see

Gavin,
I feel your pain, however I still think you are trying to solve "badly written code" with "reformat code" approach.
That just does not work.

Compare this one:

https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2462-L2512
with this one:
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L264-L311

Do you see how QueryExecutorImpl.java#L264-L311 is much readable?

PgResultSet#getFastLong is very hard to follow no matter which way you format the braces.
I believe, "readability" comes from proper segmentation (code blocks vs methods) and proper variable naming.

Vladimir

I have no dog in this fight… but a helpful hint WRT IDEs is to make a format in Eclipse, export the XML, and check it
intothe project.  Netbeans & IDEA can import the Eclipse format. 

Then you're not limited by whatever is included by the IDE.

> On Dec 27, 2015, at 8:16 AM, Markus KARG <markus@headcrashing.eu> wrote:
>
> I actually did NOT vote for "braces on new line". In fact I hate that style and always have opening braces on the
SAMEline. What I actually said was that if we decide for a near-Sun-Style it makes more sense to go with pure-sun-Style
instead,as that is built into IDEs already -- even if it produces a style a do not like much. 
> -Markus
>
> -----Original Message-----
> From: Vladimir Sitnikov [mailto:sitnikov.vladimir@gmail.com]
> Sent: Sonntag, 27. Dezember 2015 16:02
> To: Gavin Flower; Dave Cramer; Markus KARG
> Cc: List
> Subject: Re: [JDBC] PgJDBC: code reformat
>
> I did try reformatting with "opening braces on new line".
> It turned out it is incompatible with checkstyle:
> https://travis-ci.org/pgjdbc/pgjdbc/jobs/98976190
>
> There are 17 "... have incorrect indentation level" failures.
> Most of which are due to enum initialization in
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/hostchooser/HostRequirement.java#L16-L29
>
> I have just two requirements:
> 1) IDEA's autoformat should be able to produce the desired style (minimal modifications are bearable)
> 2) The style should be enforced, in other words, checkstyle (or whatever tools is used) should be able to catch most
ofthe styling issues. 
>
> As Garvin/Dave/Markus vote for "braces on new line", would you please suggest how to fix checkstyle errors?
> For instance:
> /home/travis/build/pgjdbc/pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java:2439:
> error: 'object def lcurly' have incorrect indentation level 6, expected level should be one of the following: 2, 4.
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2439
>
>
> PS.
> I do not care where the braces are.
> However, this makes me cry:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/Driver.java#L353-L373
> There are probably better ways of writing that, but those "new lines for opening brackets" make it much worse.
>
>
> PPS.
>> The other style looks cluttered, and is harder to see
>
> Gavin,
> I feel your pain, however I still think you are trying to solve "badly written code" with "reformat code" approach.
> That just does not work.
>
> Compare this one:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2462-L2512
> with this one:
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L264-L311
>
> Do you see how QueryExecutorImpl.java#L264-L311 is much readable?
>
> PgResultSet#getFastLong is very hard to follow no matter which way you format the braces.
> I believe, "readability" comes from proper segmentation (code blocks vs methods) and proper variable naming.
>
> Vladimir
>
>
>
> --
> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-jdbc

>I actually did NOT vote for "braces on new line". In fact I hate that style and always have opening braces on the SAME
line

Markus, sorry for misreading your vote.

I've just opened
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449
and it indeed suggests placing opening braces on the SAME line.

In other words, two major guidelines (Sun & Google) suggest using the
same style as pgdbc-ng does.
Coincidence? I do not think so :)

So, what if we borrow style from -ng project?

Kevin,
Thanks for Eclipse hint.

Vladimir

> On Dec 27, 2015, at 8:30 AM, Vladimir Sitnikov <sitnikov.vladimir@gmail.com> wrote:
>
>> I actually did NOT vote for "braces on new line". In fact I hate that style and always have opening braces on the
SAMEline 
>
> Markus, sorry for misreading your vote.
>
> I've just opened
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#449
> and it indeed suggests placing opening braces on the SAME line.
>
> In other words, two major guidelines (Sun & Google) suggest using the
> same style as pgdbc-ng does.
> Coincidence? I do not think so :)
>
> So, what if we borrow style from -ng project?

It comes with a free checkstyle config :)

>
> Kevin,
> Thanks for Eclipse hint.
>
> Vladimir
>
>
> --
> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-jdbc

I've scanned through -ng's checkstyle.xml and there are couple of
issues which I do not like:

1) I do not find its origin in checkstyle/checkstyle
2) I think placing new lines before binary operators makes more sense
as it allows to seamlessly add code
3) "throwsIndent=2" is strange as it would result in confusing code if
"throws" is placed on a new line (its indentation would be exactly the
same as the one of the code block:
https://github.com/impossibl/pgjdbc-ng/blob/develop/src/support/checkstyle.xml#L93

I do not find checkstyle configuration for "Sun convention". The bad
thing is checkstyle's sun_config.xml relies on *default values* and
those default values change from one checkstyle version to another. If
we pick that, we would have to live with a single checkstyle version
for ages (or reformat code with each upgrade).

The bad thing is "checkstyle IDEA plugin" does not allow to pick a
checkstyle version.

Taking that into account, I'm inclined to my original "Google style"
formatting: https://github.com/pgjdbc/pgjdbc/commit/fbf10a285e8b306c95b6467f5f9c9b7e68cbd6bb

I'll prepare Eclipse formatting configuration.

Vladimir

Kevin Wooten wrote:
> I have no dog in this fight… but a helpful hint WRT IDEs is to make a format in Eclipse,
 > export the XML, and check it into the project.  Netbeans & IDEA can import
the Eclipse
 > format. Then you're not limited by whatever is included by the IDE.

As indicated you can create your formatting in Eclipse then export. I have done
this for years with my projects.

http://dandymadeproductions.com/temp/danap_formatting.xml

Code Reformat: :((

1. Wasted of time that could be used on other efforts. If you wish do define
    as Kevin indicated above a format then future changes one by one seems more
    reasonable.

2. ONCE a complete format at once takes place, forget trying to decipher the
    mess that will be created, on diffs. Seems like a good way to inject
    undesired code. Bad idea.

danap.

>2. ONCE a complete format at once takes place, forget trying to decipher the
   mess that will be created, on diffs. Seems like a good way to inject
   undesired code. Bad idea.
2.1) Lack of style hits me quite often as I spent time trying to
figure out which style should be used for a particular part of the
code.

2.2) The code has just been migrated to maven, so now is the perfect
timing to perform complete reformat. It won't hurt us as much as
"complete reformat right in the middle of development".

PS If anyone has on-going pull requests, you'd better go ahead and
create PR in Github as rebasing across "reformat" might be tricky.

Vladimir

>
> 2. ONCE a complete format at once takes place, forget trying to decipher the
>   mess that will be created, on diffs. Seems like a good way to inject
>   undesired code. Bad idea.
>

I should stop with my opinions here… that being said…

One large nasty,  “Reformatted code and added checkstyle”, may be to swallow.  The alternative would be numerous small
changesalong with whole file reformats, e.g. “Fixed bug in generated SQL & formatted file to follow conventions”. 

While large, the “whole reformat” method would be understandable.  Good luck finding the “bug fix” in a whole file
reformat.

> danap.
>
>
>
>
> --
> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-jdbc

>Good luck finding the “bug fix” in a whole file reformat

Do you mean "git blame" kind of thing?

If the final state is the same, then there is no difference if it
comes in a single commit or from a series of commits.
"git blame" would be broken anyway, thus we'd better break it sooner.

If you mean bugs introduced by the reformat itself, I believe there
should not be many as there were just a few places where I had to
change code manually.

All the rules that involve some brain like "variable naming", "public
static final order", etc rules are disabled and they are subject to
other PRs.

I think the way to go there is to enable the rules, execute checkstyle
and enjoy all the 100500 errors. Then we configure
<maxAllowedViolations>100500</maxAllowedViolations>. That would make
sure new code is using proper names while the old one is either
deleted eventually or updated.

> with whole file reformats, e.g. “Fixed bug in generated SQL & formatted file to follow conventions”.

I think it is undesired as it would definitely create lots of merge conflicts.

> I should stop with my opinions here… that being said…

Lack of feedback would be much worse.

Vladimir

> On Dec 27, 2015, at 11:16 AM, Vladimir Sitnikov <sitnikov.vladimir@gmail.com> wrote:
>
>> Good luck finding the “bug fix” in a whole file reformat
>
> Do you mean "git blame" kind of thing?
>

I don’t think I worded my response very well…

I think one big commit changing all files would be much better than many small commits done “as you go”.

The problem, as I see it, with many small commits is that you would be intermixing reformatting changes with actual
codechanges; this would make it nearly impossible to find the “actual" changes within reformatted code. 

> If the final state is the same, then there is no difference if it
> comes in a single commit or from a series of commits.
> "git blame" would be broken anyway, thus we'd better break it sooner.
>

Agreed.  The only negative I can really see is that you almost "break history" at a single point in the code.  By
“breakhistory” I mean a point at which you cannot simply diff back to and see just the relevant changes. This can be
overcomesimply by diff-ing older code up to just before the point of the reformat.  

As you basically mentioned, this effect will happen at some point in either method, might as well make it all at one
pointin time. 

> I think the way to go there is to enable the rules, execute checkstyle
> and enjoy all the 100500 errors. Then we configure
> <maxAllowedViolations>100500</maxAllowedViolations>. That would make
> sure new code is using proper names while the old one is either
> deleted eventually or updated.
>

If checkstyle supports that, sounds like a plausible solution, to ensure forward progress.  Just ratchet the number
downas things are fixed. 

>> with whole file reformats, e.g. “Fixed bug in generated SQL & formatted file to follow conventions”.
>
> I think it is undesired as it would definitely create lots of merge conflicts.
>
>> I should stop with my opinions here… that being said…
>
> Lack of feedback would be much worse.

Just kidding as this is danger

>
> Vladimir

On 28/12/15 04:20, Kevin Wooten wrote:
> I have no dog in this fight… but a helpful hint WRT IDEs is to make a format in Eclipse, export the XML, and check it
intothe project.  Netbeans & IDEA can import the Eclipse format. 
>
[...]

Have included my Eclipse formatting configuration.

This works for me, and I have colleagues who like my style of
programming.  Better, no one complained!


Cheers,
Gavin

On 28/12/15 04:01, Vladimir Sitnikov wrote:
> I did try reformatting with "opening braces on new line".
> It turned out it is incompatible with checkstyle:
> https://travis-ci.org/pgjdbc/pgjdbc/jobs/98976190
>
> There are 17 "... have incorrect indentation level" failures.
> Most of which are due to enum initialization in
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/hostchooser/HostRequirement.java#L16-L29
>
> I have just two requirements:
> 1) IDEA's autoformat should be able to produce the desired style
> (minimal modifications are bearable)
> 2) The style should be enforced, in other words, checkstyle (or
> whatever tools is used) should be able to catch most of the styling
> issues.
Agreed!

>
> As Garvin/Dave/Markus vote for "braces on new line", would you please
> suggest how to fix checkstyle errors?
> For instance:
> /home/travis/build/pgjdbc/pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java:2439:
> error: 'object def lcurly' have incorrect indentation level 6,
> expected level should be one of the following: 2, 4.
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2439
Never used CheckStyle.

>
> PS.
> I do not care where the braces are.
> However, this makes me cry:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/Driver.java#L353-L373
> There are probably better ways of writing that, but those "new lines
> for opening brackets" make it much worse.
>
>
> PPS.
>> The other style looks cluttered, and is harder to see
> Gavin,
> I feel your pain, however I still think you are trying to solve "badly
> written code" with "reformat code" approach.
> That just does not work.
It definite works!  At least the way I structure my code.  I will
refactor my code if it gets to look complicated, often after I have
understood the problem I'm solving better.  I use the same style of
coding for one off explorations for personal use, as I do for production
code.

>
> Compare this one:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2462-L2512
> with this one:
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L264-L311
>
> Do you see how QueryExecutorImpl.java#L264-L311 is much readable?
No.  Possibly I'm not seeing the code you want me too?

I would add more blank lines for readability, but would eliminate blank
lines after opening brackets and before closing brackets.

Also would simplify lines to ease readability and improve ability to
debug, eg

Would rewrite:
     ((V3ParameterList) parameterList).checkAllParametersSet();
as
V3ParameterList v3ParameterList =(V3ParameterList) parameterList;
v3ParameterList.checkAllParametersSet();


and would rewrite:
     if ((flags &QueryExecutor.QUERY_SUPPRESS_BEGIN)
!=0||protoConnection.getTransactionState()
!=ProtocolConnection.TRANSACTION_IDLE)
as
     boolean abc =(flags &QueryExecutor.QUERY_SUPPRESS_BEGIN) !=0;
     boolean xyz = protoConnection.getTransactionState()
!=ProtocolConnection.TRANSACTION_IDLE;
     if (abc||xyz)
(except I would use more meaningful names for 'abc' & 'xyz' above!)

I also only ever have one return per method, as that makes it easier to:
     (1) understand
     (2) maintain (add new code and/or change existing code)
     (3) debug

>
> PgResultSet#getFastLong is very hard to follow no matter which way you
> format the braces.
> I believe, "readability" comes from proper segmentation (code blocks
> vs methods) and proper variable naming.
Good code structuring and naming conventions are essential!

Absolutely NO coding style can make up for bad code structuring and
naming conventions!!!

If my nested if statements start getting unwieldy, I will move parts
into separate methods - as this makes it easier to read and the naming
of the methods help document what I'm trying to do.  Plus I will break
down complicated conditionals into pre-calculated booleans (see example
above) for better readability.

Sometimes I'll have a method with only a few lines of code, if it is
invoked from multiple places and has strategic significance.  The
optimising complier will in-line code that gets invoked frequently, so
no loss in performance doing so.

I spend far more time reading my code than writing it, so I'm prepared
to put much more effort into writing and reformatting code, than most
people appear to do.

>
> Vladimir

VS> Do you see how QueryExecutorImpl.java#L264-L311 is much readable?
DC> Same problem here, one of them shows comments

I was intentionally linking to the part with elaborated comments.
It was to justify the point of "code with well written comments is
understandable no matter where braces are", and "weird code is weird
no matter where braces are".
Just in case: http://commadot.com/wtf-per-minute/

Vladimir

On 29/12/15 01:22, Dave Cramer wrote:
>
>     On 28/12/15 04:01, Vladimir Sitnikov wrote:
>
[...]
>
>
>         Compare this one:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2462-L2512
>         with this one:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L264-L311
>
>         Do you see how QueryExecutorImpl.java#L264-L311 is much readable?
>
>     No.  Possibly I'm not seeing the code you want me too?
>
> Same problem here, one of them shows comments. I believe the first one.

Comments can be good, but are orthogonal to the question of code style.

>
>     I would add more blank lines for readability, but would eliminate
>     blank lines after opening brackets and before closing brackets.
>
>     Also would simplify lines to ease readability and improve ability
>     to debug, eg
>
>
> So there are very good reasons in Java not to do what you are
> proposing below. Mostly because it creates objects which have to be
> garbage collected.

Hmm...

I suspected that the Just-in-Time compiler (I knew from my previous
research that they are truly vicious when it comes to optimisation!!!)
would end with up with the same code, and actually ran a test that
proved it.

I ran multiple loops with and without an intermediate variable, the
variation between runs of the same type was vastly greater than the
difference in the means - each run was about 34 seconds.

I looked at the output of -XX:CompileCommand=print and found that the
optimised code was identical, even though the initial byte code was
different!

In essence, adding an intermediate variable makes no difference in the
long run.

So making code more readable does NOT lead to any noticeable reduction
in performance when the coded is executed many times.  I strongly
suspect that the difference in code executed just once in a run, would
be insignificant compared to the noise in measuring the run times in
practice.

Therefore, since programmer time is costly, it make more sense to put
effort into code readability, than into premature micro optimisations.
Which is basically what most texts books on optimisation say.

If you want, I can send you the gory details in an OpenDocument file
(e.g. one produced natively by LibreOffice).

> While I agree that it is more readable. I think driver code has a
> responsibility to be as curt as possible with respect to creating objects.

In practice, the changes I have suggested do not have that problem.

>     Would rewrite:
>         ((V3ParameterList) parameterList).checkAllParametersSet();
>     as
>     V3ParameterList v3ParameterList =(V3ParameterList) parameterList;
>     v3ParameterList.checkAllParametersSet();
>
>
>     and would rewrite:
>         if ((flags &QueryExecutor.QUERY_SUPPRESS_BEGIN)
>     !=0||protoConnection.getTransactionState()
>     !=ProtocolConnection.TRANSACTION_IDLE)
>     as
>         boolean abc =(flags &QueryExecutor.QUERY_SUPPRESS_BEGIN) !=0;
>         boolean xyz = protoConnection.getTransactionState()
>     !=ProtocolConnection.TRANSACTION_IDLE;
>         if (abc||xyz)
>     (except I would use more meaningful names for 'abc' & 'xyz' above!)
>
>     I also only ever have one return per method, as that makes it
>     easier to:
>         (1) understand
>         (2) maintain (add new code and/or change existing code)
>         (3) debug
>
>
>         PgResultSet#getFastLong is very hard to follow no matter which
>         way you
>         format the braces.
>         I believe, "readability" comes from proper segmentation (code
>         blocks
>         vs methods) and proper variable naming.
>
>
>
>
> Dave Cramer
>
> davec@postgresintl.com <mailto:davec@postgresintl.com>
> www.postgresintl.com <http://www.postgresintl.com/>
>

JMH is an awesome harness for constructing exactly these sorts of performance tests:
http://openjdk.java.net/projects/code-tools/jmh/

If we care about driver performance, it might be interesting to construct
some representative tests and include them as part of the build process, to
ensure that metrics we care about don't get regressions.  Obviously this would be
some work that needs volunteers interested in working on it, just an idea to think on.


And while we're bikeshedding, I personally would -1 the "only one exit per function".

Sometimes it's nice to bail early e.g. :

if (offset == length) { return 0; }
... do work ...
return result;


> On Dec 28, 2015, at 2:02 PM, Dave Cramer <pg@fastcrypt.com> wrote:
>
>
> On 28 December 2015 at 16:16, Gavin Flower <GavinFlower@archidevsys.co.nz> wrote:
> On 29/12/15 01:22, Dave Cramer wrote:
>
>     On 28/12/15 04:01, Vladimir Sitnikov wrote:
>
> [...]
>
>
>         Compare this one:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java#L2462-L2512
>         with this one:
>
https://github.com/pgjdbc/pgjdbc/blob/197175039068446a15c30d2b5e949f1eae08515d/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L264-L311
>
>         Do you see how QueryExecutorImpl.java#L264-L311 is much readable?
>
>     No.  Possibly I'm not seeing the code you want me too?
>
> Same problem here, one of them shows comments. I believe the first one.
>
> Comments can be good, but are orthogonal to the question of code style.
>
>
>     I would add more blank lines for readability, but would eliminate
>     blank lines after opening brackets and before closing brackets.
>
>     Also would simplify lines to ease readability and improve ability
>     to debug, eg
>
>
> So there are very good reasons in Java not to do what you are proposing below. Mostly because it creates objects
whichhave to be garbage collected. 
>
> Hmm...
>
> I suspected that the Just-in-Time compiler (I knew from my previous research that they are truly vicious when it
comesto optimisation!!!) would end with up with the same code, and actually ran a test that proved it. 
>
> I ran multiple loops with and without an intermediate variable, the variation between runs of the same type was
vastlygreater than the difference in the means - each run was about 34 seconds. 
>
> I looked at the output of -XX:CompileCommand=print and found that the optimised code was identical, even though the
initialbyte code was different! 
>
> In essence, adding an intermediate variable makes no difference in the long run.
>
> So making code more readable does NOT lead to any noticeable reduction in performance when the coded is executed many
times. I strongly suspect that the difference in code executed just once in a run, would be insignificant compared to
thenoise in measuring the run times in practice. 
>
> Therefore, since programmer time is costly, it make more sense to put effort into code readability, than into
prematuremicro optimisations.  Which is basically what most texts books on optimisation say. 
>
> If you want, I can send you the gory details in an OpenDocument file (e.g. one produced natively by LibreOffice).
>
> While I agree that it is more readable. I think driver code has a responsibility to be as curt as possible with
respectto creating objects. 
>
> In practice, the changes I have suggested do not have that problem.
>
>
> I stand corrected. Thanks!
>
>
>     Would rewrite:
>         ((V3ParameterList) parameterList).checkAllParametersSet();
>     as
>     V3ParameterList v3ParameterList =(V3ParameterList) parameterList;
>     v3ParameterList.checkAllParametersSet();
>
>
>     and would rewrite:
>         if ((flags &QueryExecutor.QUERY_SUPPRESS_BEGIN)
>     !=0||protoConnection.getTransactionState()
>     !=ProtocolConnection.TRANSACTION_IDLE)
>     as
>         boolean abc =(flags &QueryExecutor.QUERY_SUPPRESS_BEGIN) !=0;
>         boolean xyz = protoConnection.getTransactionState()
>     !=ProtocolConnection.TRANSACTION_IDLE;
>         if (abc||xyz)
>     (except I would use more meaningful names for 'abc' & 'xyz' above!)
>
>     I also only ever have one return per method, as that makes it
>     easier to:
>         (1) understand
>         (2) maintain (add new code and/or change existing code)
>         (3) debug
>
>
>         PgResultSet#getFastLong is very hard to follow no matter which
>         way you
>         format the braces.
>         I believe, "readability" comes from proper segmentation (code
>         blocks
>         vs methods) and proper variable naming.
>
>
>
>
> Dave Cramer
>
> davec@postgresintl.com <mailto:davec@postgresintl.com>
> www.postgresintl.com <http://www.postgresintl.com/>

Believe me or not, I've pushed "reformat" commit to master.

Just for the reference: Eclipse turned out to be very tricky to
configure, so if you use IDEA, I would still recommend (see [1])
importing "IDEA" configuration, not Eclipse's one.
In fact, I was not able to identify a single "configuration xml with
all Eclipse formatting settings".

[1]: https://github.com/pgjdbc/pgjdbc#support-for-ides

Vladimir

Kudos! :-)
-Markus

-----Original Message-----
From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Vladimir Sitnikov
Sent: Dienstag, 29. Dezember 2015 14:52
To: Dave Cramer; Markus KARG
Cc: Steven Schlansker; Gavin Flower; List
Subject: Re: [JDBC] PgJDBC: code reformat

Believe me or not, I've pushed "reformat" commit to master.

Just for the reference: Eclipse turned out to be very tricky to configure, so if you use IDEA, I would still recommend
(see[1]) importing "IDEA" configuration, not Eclipse's one. 
In fact, I was not able to identify a single "configuration xml with all Eclipse formatting settings".

[1]: https://github.com/pgjdbc/pgjdbc#support-for-ides

Vladimir


--
Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org) To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-jdbc

On 29/12/15 11:39, Steven Schlansker wrote:
> JMH is an awesome harness for constructing exactly these sorts of performance tests:
> http://openjdk.java.net/projects/code-tools/jmh/
>
> If we care about driver performance, it might be interesting to construct
> some representative tests and include them as part of the build process, to
> ensure that metrics we care about don't get regressions.  Obviously this would be
> some work that needs volunteers interested in working on it, just an idea to think on.
>
>
> And while we're bikeshedding, I personally would -1 the "only one exit per function".
>
> Sometimes it's nice to bail early e.g. :
>
> if (offset == length) { return 0; }
> ... do work ...
> return result;
[...]

Sorry , don't have time to reply in depth, possibly in about a week
(holiday).

Only one exit per function works best when it is an absolute rule - with
possibly very rare exceptions, and I've yet to see a valid exception in
over 40 years of programming.

In practice, having multiple returns is unnecessary & makes code harder
to read.

Remember the Just-in-Time compiler is very good at optimising, even way
back in the JDK 1.6 days, and we're now up to 1.8!


Cheers,
Gavin