You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by Graham Russell <gr...@ham1.co.uk> on 2017/02/11 22:27:48 UTC
Code Style Guidelines
Hi all
Do we have any (written) guidance on code style?
I know we have checkstyle, but this has very few rules.
The reason for asking is that I think having a consistent style is very
important for readability (which helps writing new feature or fixing
existing code etc.).
The main things I think contribute to this are (and which are fairly easy
to detect/fix):
* (soft) line lengths (80, 100?)
* spacing between elements on a line e.g. if (..., while (...,
methodCall(arg1, arg2), "con" + "cat"
* import order, spacing and not using .*
* line length of methods (soft limit of 50?)
* line length of classes (soft limit of 500?)
From Phillippe's Java 8 email:
* Use of Optional
* Default/static methods on Interfaces
* Lambas where possible (max ~5 lines?)
I have found this: https://wiki.apache.org/jmeter/JMeterEclipse
which suggests some formatting e.g. 80 char line length and new line before
opening brace but most of the code doesn't conform to that - it was also
updated 8 years ago.
Any other ideas/thoughts?
Thanks
Graham
Re: Code Style Guidelines
Posted by Milamber <mi...@apache.org>.
Hello,
The JMeterEclipse have need to be update with a most casual code style
(current resolution of computer's screen are more greater than in 2003,
so the length of line can be more longer for example)
I'm agree with Philippe about avoid to change the current code for
formatting to keep an easy way to compare version of a file. We can just
use the code formating for the new contributions (on whole files or just
on the lines updates in an 'old' file)
Milamber
On 11/02/2017 23:14, Philippe Mouawad wrote:
> On Sat, Feb 11, 2017 at 11:27 PM, Graham Russell <gr...@ham1.co.uk> wrote:
>
>> Hi all
>>
>> Do we have any (written) guidance on code style?
>>
>> I know we have checkstyle, but this has very few rules.
>>
>> The reason for asking is that I think having a consistent style is very
>> important for readability (which helps writing new feature or fixing
>> existing code etc.).
>>
>> The main things I think contribute to this are (and which are fairly easy
>> to detect/fix):
>> * (soft) line lengths (80, 100?)
>> * spacing between elements on a line e.g. if (..., while (...,
>> methodCall(arg1, arg2), "con" + "cat"
>>
> If this is related to String concat, we should only do that in places where
> performance is not a critical point as IMU StringBuilder concat is still
> better than '+'
>
>> * import order, spacing and not using .*
>> * line length of methods (soft limit of 50?)
>> * line length of classes (soft limit of 500?)
>>
> I am ok with these proposals and providing a checkstyle and Eclipse
> formatter might be very useful.
> But if possible, I'd prefer that we do not modify too much code only for
> code style as it might make it difficult to look into particular
> regressions.
> So maybe do that on new code or when changing a file for something else..
>
>> From Phillippe's Java 8 email:
>>
> * Use of Optional
> >From the document, I understand Optional has a memory/performance impact so
> we need to take this into account where we decide to apply it.
> But I'm ok with it.
>
>
>> * Default/static methods on Interfaces
>>
> +1
>
>> * Lambas where possible (max ~5 lines?)
>>
> +1 provided reduced code is more readable than existing one. If it's
> reduced but we have to scratch our head to understand it I'm not sure it's
> a gain :-)
>
> Regarding Streams, we need to be sure of performance impact. And It appears
> Sonar has some issues analyzing such code
>
>
>> I have found this: https://wiki.apache.org/jmeter/JMeterEclipse
>> which suggests some formatting e.g. 80 char line length and new line before
>> opening brace but most of the code doesn't conform to that - it was also
>> updated 8 years ago.
>>
>> Any other ideas/thoughts?
>>
>> Thanks
>>
>> Graham
>>
>
>
Re: Code Style Guidelines
Posted by Vladimir Sitnikov <si...@gmail.com>.
Jeremiah>The project was internal and I cannot show it publicly.
Can you clarify which kind of changes did take place?
Was there a code movement (e.g. methods, variables, fields rearranged)?
Class refactoring (e.g. method moved to another class)?
There's no magic. git-blame can ignore whitespace changes just fine, so
please provide some meaningful details.
I claim that reformat (that is whitespace changes + minor cleanups like
"adding braces to if") add little to no overhead on history browsing.
Of course, code movement (method movement inside a file / across files)
create great overhead on history browsing.
If that was your case, please read
http://mail-archives.apache.org/mod_mbox/jmeter-dev/201702.mbox/%3CCAB=Je-HHoFTaGFYE4NuFcCEFWxQ3nvARnjt-ioUQ7MuxTBKjpg@mail.gmail.com%3E
once
again and note that I have never suggested to move code around.
What I suggest is to agree on whitespace policy, apply it and implement a
relevant CI check.
Jeremiah>Avoid repeating things that cause pain.
Exactly. That is why I suggest to setup a policy that will ensure that
ill-formatted code never reaches trunk.
So we reformat the code once and make sure the code is always in a good
shape.
Jeremiah>Have you? From my perspective, you haven't shown much of anything
because
Jeremiah>we don't see your images
Can you clarify if you have seen a URL listed in this message:
http://mail-archives.apache.org/mod_mbox/jmeter-dev/201702.mbox/%3CCAB%3DJe-GrVCauabFFwy_itvk5E74w8qm58XbHTLhQKs7n7nuT%2BQ%40mail.gmail.com%3E
?
I did not generate a screenshot for ReportGeneratorConfiguration (that is
part of JMeter), however I see no much point in doing that. Everyone can
just "git blame" that file and see that formatting by Felix did not break
annotate.
Jeremiah>Regardless, you cannot possibly expect everyone
Jeremiah>to use your exact tooling just to browse project history?
Bot yes and no.
Exactly the same "blame results with whitespace being ignored" can be
produced by regular command line and UI tools.
For instance, here are the tools that come with git/svn: git blame
<filename>; svn blame <filename>, git gui blame <filename>
There are options to honor/ignore whitespace changes in both git and svn.
What's the problem with history browsing?
However, the one who browses history might need some cross-reference,
navigation and other IDE features that are not available in dump console.
If that is the case, he/she picks an IDE.
There are free & open source IDEs that can do history browsing (e.g. IDEA,
Netbeans), so I see no reason to vote based on "this particular feature
does not work in Eclipse as of 2012-mm-dd" feeling.
Note: I take sebb's words "the feature is missing in Eclipse" for granted.
It might be the feature has already been implemented (e.g. for git or
whatever).
Vladimir
RE: Code Style Guidelines
Posted by "Epp, Jeremiah W (Contractor)" <je...@cas.org>.
> -----Original Message-----
> From: Vladimir Sitnikov [mailto:sitnikov.vladimir@gmail.com]
> Sent: Monday, February 13, 2017 10:20 AM
> To: dev@jmeter.apache.org
> Subject: Re: Code Style Guidelines
>
>> It kind of ruins git-blame output and tends to make understanding the
>> scope of work in any file or tree very unclear.
>
> Please provide at least one example.
>> I recently wanted to see when and why a specific function was added to a
>> project and ran afoul of exactly this issue
The project was internal and I cannot show it publicly. That doesn't make
it less valid an experience or less valuable as a point of perspective.
It's not like this sort of thing is a unique (or even especially rare)
occurrence when you're doing archaeology on legacy code. Avoid
repeating things that cause pain.
> I've provided two examples
Have you? From my perspective, you haven't shown much of anything because
we don't see your images. Regardless, you cannot possibly expect everyone
to use your exact tooling just to browse project history?
Cheers,
Wyatt
Confidentiality Notice: This electronic message transmission, including any attachment(s), may contain confidential, proprietary, or privileged information from Chemical Abstracts Service ("CAS"), a division of the American Chemical Society ("ACS"). If you have received this transmission in error, be advised that any disclosure, copying, distribution, or use of the contents of this information is strictly prohibited. Please destroy all copies of the message and contact the sender immediately by either replying to this message or calling 614-447-3600.
Re: Code Style Guidelines
Posted by Vladimir Sitnikov <si...@gmail.com>.
>It kind of ruins git-blame output and tends to make understanding the scope
>of work in any file or tree very unclear.
Please provide at least one example.
I've provided two examples (pgjdbc and JMeter) that show that reformat does
not ruin git-blame.
>I recently wanted to see when and why a specific function was added to a
project and ran afoul of exactly this issue: between two moves and a
reformat, the task went from trivial to annoying very fast.
Was that with JMeter? Can you give an example/link?
Vladimir
RE: Code Style Guidelines
Posted by "Epp, Jeremiah W (Contractor)" <je...@cas.org>.
> From: Vladimir Sitnikov [mailto:sitnikov.vladimir@gmail.com]
> Sent: Monday, February 13, 2017 4:00 AM
> To: dev@jmeter.apache.org
> Subject: Re: Code Style Guidelines
>
>> My experience is that code reformatting makes history browsing very much
>> harder.
>
> Could you give any example here? What particular use case breaks with
> single-commit reformatting?
It kind of ruins git-blame output and tends to make understanding the scope
of work in any file or tree very unclear.
I recently wanted to see when and why a specific function was added to a
project and ran afoul of exactly this issue: between two moves and a
reformat, the task went from trivial to annoying very fast.
Cheers,
Wyatt
Confidentiality Notice: This electronic message transmission, including any attachment(s), may contain confidential, proprietary, or privileged information from Chemical Abstracts Service ("CAS"), a division of the American Chemical Society ("ACS"). If you have received this transmission in error, be advised that any disclosure, copying, distribution, or use of the contents of this information is strictly prohibited. Please destroy all copies of the message and contact the sender immediately by either replying to this message or calling 614-447-3600.
Re: Code Style Guidelines
Posted by Vladimir Sitnikov <si...@gmail.com>.
sebb> But not for all the people who may wish to use our code.
There's IDEA Community edition that is available for all the people for
free.
It comes with Git/SVN support, so everybody can browse the code.
sebb>I meant see if you can easily find out when a particular line of code
was added in a source file that has been subject to lots of formatting
changes.
sebb>I'm sure there are lots of examples.
Well, apache/jmeter git mirror shows the history across reformats just fine.
For instance:
ReportGeneratorConfiguration was added on 7 Dec 2015 by Philippe, and then
reformatted on 10 Dec 2015 by Felix (see
https://github.com/apache/jmeter/commit/aece733b07e41017b60a9e218c41c264ca683d2d
)
IDEA shows proper annotations as if reformat was never performed.
So what? I did check a couple of reformat examples, and the outcome is that
formatting creates no overhead on history browsing.
Vladimir
Re: Code Style Guidelines
Posted by sebb <se...@gmail.com>.
On 13 February 2017 at 12:45, Vladimir Sitnikov
<si...@gmail.com> wrote:
> sebb>Attachments not present; they are dropped by most Apache mailing lists.
>
> Here's the same mail in Google docs with screenshots:
> https://docs.google.com/document/d/1-Yr_PEOhkLXS8ArKxRVVl-9b8l8Gj-5x0gdgmW5MyJ8/edit?usp=sharing
>
>
> sebb>AFAIK, that does not work in Eclipse
>
> Well, I might check how it looks like in Eclipse, however it does sound
> like a feature on the surface. It is not a rocket science after all.
>
> Does it really make sense to use "I cannot browse SVN history in Eclipse"
> argument?
> If that is the case, then any commit would break Eclipse in terms of
> history browsing. Does that mean we aim to avoid commits at all?
>
> By the way: IntelliJ provides opensource licenses for committers.
But not for all the people who may wish to use our code.
> sebb>Note also that JMeter uses SVN currently, which behaves differently.
> sebb>Try doing the same exercise with a JMeter file.
>
> Do you mean I should commit some temporary SVN branch with code formatting?
I meant see if you can easily find out when a particular line of code
was added in a source file that has been subject to lots of formatting
changes.
I'm sure there are lots of examples.
>
> Vladimir
Re: Code Style Guidelines
Posted by Vladimir Sitnikov <si...@gmail.com>.
sebb>Attachments not present; they are dropped by most Apache mailing lists.
Here's the same mail in Google docs with screenshots:
https://docs.google.com/document/d/1-Yr_PEOhkLXS8ArKxRVVl-9b8l8Gj-5x0gdgmW5MyJ8/edit?usp=sharing
sebb>AFAIK, that does not work in Eclipse
Well, I might check how it looks like in Eclipse, however it does sound
like a feature on the surface. It is not a rocket science after all.
Does it really make sense to use "I cannot browse SVN history in Eclipse"
argument?
If that is the case, then any commit would break Eclipse in terms of
history browsing. Does that mean we aim to avoid commits at all?
By the way: IntelliJ provides opensource licenses for committers.
sebb>Note also that JMeter uses SVN currently, which behaves differently.
sebb>Try doing the same exercise with a JMeter file.
Do you mean I should commit some temporary SVN branch with code formatting?
Vladimir
Re: Code Style Guidelines
Posted by sebb <se...@gmail.com>.
On 13 February 2017 at 08:59, Vladimir Sitnikov <sitnikov.vladimir@gmail.com
> wrote:
> sebb>My experience is that code reformatting makes history browsing very
> much harder.
>
> Could you give any example here? What particular use case breaks with
> single-commit reformatting?
>
> Just to be clear: I don't suggest to reformat the code to some completely
> alien format. I suggest to pick a format that suits most part of the
> current code.
>
> Let me show an example that "browsing history through reformats" just
> works.
> It is not a theoretical example, but a very real one.
>
> I'll pick v3/QueryExecutorImpl from pgjdbc (that is one of the core class
> that implements wire protocol, so its history browsing is important).
> Here's what Github shows for the "reformat commit":
> [image: 01_github_reformat_diff.png]
> Basically, Github shows that "everything was changed".
> Indeed the file used to have 4 space for indentation, and it was formatted
> with 2 space indentation. Braces were forced after each if/else statement,
> etc, etc.
> There are 341 differences with "do not ignore whitespace" mode, and there
> are 220 differences with "ignore whitespace" mode.
>
> Just for the reference: this file suffered relocation in the source tree
> on 20.12.2015 as a part of "ant -> maven" migration.
>
> Here's the same file/line opened in IntelliJ as of now:
> [image: 02_intellijidea_annotate.png]
>
Attachments not present; they are dropped by most Apache mailing lists.
> You can see how "annotate" attributes lines way back to 2005 even though
> almost every line was altered on 29.12.2015 formatting commit.
> It just works across reformat and maven migration.
>
> So leading/trailing whitespace changes are harmless.
>
What happens to "add space after if", "add braces to if" kind of changes?
> Of course they are listed in the annotate.
> For instance:
> [image: 03_brace_added_annotate.png]
> Lines 1574 and 1576 are a part of reformat commit that added braces around
> if.
> Does it make history browsing harder?
> I don't think so, as I can just click "annotate previous revision"
>
AFAIK, that does not work in Eclipse
> [image: 04_annotate_prev_revision.png]
> and see how the file looked like before reformatting took place:
> [image: 05_prev_revision_annotated.png]
> So having a common format is a win as
> 1) it makes code easier to read, and thus contribute
> 2) it makes code more robust in face of "goto fail" kind of bugs
>
Agreed
> 3) it still allows decent annotate/history browsing
>
I'm not convinced.
Note also that JMeter uses SVN currently, which behaves differently.
Try doing the same exercise with a JMeter file.
> Vladimir
>
>
Re: Code Style Guidelines
Posted by Vladimir Sitnikov <si...@gmail.com>.
sebb>My experience is that code reformatting makes history browsing very
much harder.
Could you give any example here? What particular use case breaks with
single-commit reformatting?
Just to be clear: I don't suggest to reformat the code to some completely
alien format. I suggest to pick a format that suits most part of the
current code.
Let me show an example that "browsing history through reformats" just works.
It is not a theoretical example, but a very real one.
I'll pick v3/QueryExecutorImpl from pgjdbc (that is one of the core class
that implements wire protocol, so its history browsing is important).
Here's what Github shows for the "reformat commit":
[image: 01_github_reformat_diff.png]
Basically, Github shows that "everything was changed".
Indeed the file used to have 4 space for indentation, and it was formatted
with 2 space indentation. Braces were forced after each if/else statement,
etc, etc.
There are 341 differences with "do not ignore whitespace" mode, and there
are 220 differences with "ignore whitespace" mode.
Just for the reference: this file suffered relocation in the source tree on
20.12.2015 as a part of "ant -> maven" migration.
Here's the same file/line opened in IntelliJ as of now:
[image: 02_intellijidea_annotate.png]
You can see how "annotate" attributes lines way back to 2005 even though
almost every line was altered on 29.12.2015 formatting commit.
It just works across reformat and maven migration.
So leading/trailing whitespace changes are harmless.
What happens to "add space after if", "add braces to if" kind of changes?
Of course they are listed in the annotate.
For instance:
[image: 03_brace_added_annotate.png]
Lines 1574 and 1576 are a part of reformat commit that added braces around
if.
Does it make history browsing harder?
I don't think so, as I can just click "annotate previous revision"
[image: 04_annotate_prev_revision.png]
and see how the file looked like before reformatting took place:
[image: 05_prev_revision_annotated.png]
So having a common format is a win as
1) it makes code easier to read, and thus contribute
2) it makes code more robust in face of "goto fail" kind of bugs
3) it still allows decent annotate/history browsing
Vladimir
Re: Code Style Guidelines
Posted by sebb <se...@gmail.com>.
On 12 February 2017 at 22:52, Vladimir Sitnikov
<si...@gmail.com> wrote:
> Philippe>But if possible, I'd prefer that we do not modify too much code
> only for code style as it might make it difficult to look into particular
> regressions.
>
> I don't think reformatting would complicate things much.
> I think it is fine to settle on some rule(s) (e.g. "space after if", or "no
> wildcard imports" or whatever) and apply the formatting in one go.
>
> On the positive side, code style violations could be made to fail the
> build. That would prevent bad style creeping into the codebase.
>
> I have experience with "global reformat" for pgjdbc project (see [1]). That
> was smooth.
> Code formatting made the code more consistent (easier to read, easier to
> update), yet it did not complicate history browsing much.
My experience is that code reformatting makes history browsing very much harder.
> Even thing like "ant -> maven" conversion (with folder reorganization) went
> fine.
>
>
> Innocent refactoring like "split big class into a couple of more focused
> ones" brings much more damage in terms of "history traceability". Regular
> tools like "blame/annotate" give no clue that "this particular method was
> moved from that particular file".
>
> That is why I think it makes sense to just reformat all the files and add
> relevant checkstyle rules when it comes to simple things like "number of
> spaces for indentation", "spaces after if", "brace placement".
>
> Felix>There would be a lot of warnings. Nobody would look at them after a
> while
>
> I agree here. The rules should result in build failure on violation, and
> the rules should be easy to configure in IDEs (e.g. autoformat code should
> produce a compliant code)
>
> [1]:
> https://github.com/pgjdbc/pgjdbc/commit/8f1c9d7bd48de29e3d6dabd0489520ec49d83b25
>
> Vladimir
Re: Code Style Guidelines
Posted by Vladimir Sitnikov <si...@gmail.com>.
Philippe>But if possible, I'd prefer that we do not modify too much code
only for code style as it might make it difficult to look into particular
regressions.
I don't think reformatting would complicate things much.
I think it is fine to settle on some rule(s) (e.g. "space after if", or "no
wildcard imports" or whatever) and apply the formatting in one go.
On the positive side, code style violations could be made to fail the
build. That would prevent bad style creeping into the codebase.
I have experience with "global reformat" for pgjdbc project (see [1]). That
was smooth.
Code formatting made the code more consistent (easier to read, easier to
update), yet it did not complicate history browsing much.
Even thing like "ant -> maven" conversion (with folder reorganization) went
fine.
Innocent refactoring like "split big class into a couple of more focused
ones" brings much more damage in terms of "history traceability". Regular
tools like "blame/annotate" give no clue that "this particular method was
moved from that particular file".
That is why I think it makes sense to just reformat all the files and add
relevant checkstyle rules when it comes to simple things like "number of
spaces for indentation", "spaces after if", "brace placement".
Felix>There would be a lot of warnings. Nobody would look at them after a
while
I agree here. The rules should result in build failure on violation, and
the rules should be easy to configure in IDEs (e.g. autoformat code should
produce a compliant code)
[1]:
https://github.com/pgjdbc/pgjdbc/commit/8f1c9d7bd48de29e3d6dabd0489520ec49d83b25
Vladimir
Re: Code Style Guidelines
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 12. Februar 2017 21:32:35 MEZ schrieb Graham Russell <gr...@ham1.co.uk>:
>On 11 February 2017 at 23:14, Philippe Mouawad
><ph...@gmail.com> wrote:
>> On Sat, Feb 11, 2017 at 11:27 PM, Graham Russell <gr...@ham1.co.uk>
>wrote:
>>
>>> Hi all
>>>
>>> Do we have any (written) guidance on code style?
>>>
>>> I know we have checkstyle, but this has very few rules.
>>>
>>> The reason for asking is that I think having a consistent style is
>very
>>> important for readability (which helps writing new feature or fixing
>>> existing code etc.).
>>>
>>> The main things I think contribute to this are (and which are fairly
>easy
>>> to detect/fix):
>>> * (soft) line lengths (80, 100?)
>>> * spacing between elements on a line e.g. if (..., while (...,
>>> methodCall(arg1, arg2), "con" + "cat"
>>>
>> If this is related to String concat, we should only do that in places
>where
>> performance is not a critical point as IMU StringBuilder concat is
>still
>> better than '+'
>
>This was mainly about white space. However, I believe StringBuilder is
>only better (performance) than "+" when inside a loop, elsewhere you
>should use "+" for readability:
>http://stackoverflow.com/questions/1532461/stringbuilder-vs-string-concatenation-in-tostring-in-java
+1
>
>>
>>> * import order, spacing and not using .*
>>> * line length of methods (soft limit of 50?)
>>> * line length of classes (soft limit of 500?)
>>>
>>
>> I am ok with these proposals and providing a checkstyle and Eclipse
>> formatter might be very useful.
>> But if possible, I'd prefer that we do not modify too much code only
>for
>> code style as it might make it difficult to look into particular
>> regressions.
>> So maybe do that on new code or when changing a file for something
>else..
>
>Yes, I agree that this should only be on new code or 'while you are in
>the area' i.e. the boy scout rule (leave the code better than you
>found it).
>
>I use IntelliJ - I will export my current JMeter code formatting rules
>and also try to create one for Eclipse which broadly aligns.
I believe the tomcat project has coding guidelines which we could use. And they have docs on how to setup your ide.
>
>If we put these into checkstyle would this fail the build, or can they
>just be warnings?
There would be a lot of warnings. Nobody would look at them after a while.
We could look at the categories with the least amount of warnings, get them to zero and then enforce them. But I believe that would not work with the boycott rule alone.
>
>I've created a wiki page to capture the key points:
>https://wiki.apache.org/jmeter/CodeStyleGuidelines
Great.
One rule I really like is to separate formatting from code changes.
Felix
>
>>
>>>
>>> From Phillippe's Java 8 email:
>>>
>> * Use of Optional
>>>
>>
>> From the document, I understand Optional has a memory/performance
>impact so
>> we need to take this into account where we decide to apply it.
>> But I'm ok with it.
>
>Certainly, this is important with performance critical parts of the
>code.
>
>>
>>
>>> * Default/static methods on Interfaces
>>>
>> +1
>>
>>> * Lambas where possible (max ~5 lines?)
>>>
>> +1 provided reduced code is more readable than existing one. If it's
>> reduced but we have to scratch our head to understand it I'm not sure
>it's
>> a gain :-)
>
>Absolutely, this is all to do with readability, if it doesn't fit in
>~5 lines then it should probably be moved to a method and named
>appropriately.
>
>>
>> Regarding Streams, we need to be sure of performance impact. And It
>appears
>> Sonar has some issues analyzing such code
>
>Yes I noticed that too, is this due to the version of Sonar we are
>using or to unidentified/unfixed bugs?
>
>>
>>
>>> I have found this: https://wiki.apache.org/jmeter/JMeterEclipse
>>> which suggests some formatting e.g. 80 char line length and new line
>before
>>> opening brace but most of the code doesn't conform to that - it was
>also
>>> updated 8 years ago.
>>>
>>> Any other ideas/thoughts?
>>>
>>> Thanks
>>>
>>> Graham
>>
>> --
>> Cordialement.
>> Philippe Mouawad.
Re: Code Style Guidelines
Posted by Graham Russell <gr...@ham1.co.uk>.
On 11 February 2017 at 23:14, Philippe Mouawad
<ph...@gmail.com> wrote:
> On Sat, Feb 11, 2017 at 11:27 PM, Graham Russell <gr...@ham1.co.uk> wrote:
>
>> Hi all
>>
>> Do we have any (written) guidance on code style?
>>
>> I know we have checkstyle, but this has very few rules.
>>
>> The reason for asking is that I think having a consistent style is very
>> important for readability (which helps writing new feature or fixing
>> existing code etc.).
>>
>> The main things I think contribute to this are (and which are fairly easy
>> to detect/fix):
>> * (soft) line lengths (80, 100?)
>> * spacing between elements on a line e.g. if (..., while (...,
>> methodCall(arg1, arg2), "con" + "cat"
>>
> If this is related to String concat, we should only do that in places where
> performance is not a critical point as IMU StringBuilder concat is still
> better than '+'
This was mainly about white space. However, I believe StringBuilder is
only better (performance) than "+" when inside a loop, elsewhere you
should use "+" for readability:
http://stackoverflow.com/questions/1532461/stringbuilder-vs-string-concatenation-in-tostring-in-java
>
>> * import order, spacing and not using .*
>> * line length of methods (soft limit of 50?)
>> * line length of classes (soft limit of 500?)
>>
>
> I am ok with these proposals and providing a checkstyle and Eclipse
> formatter might be very useful.
> But if possible, I'd prefer that we do not modify too much code only for
> code style as it might make it difficult to look into particular
> regressions.
> So maybe do that on new code or when changing a file for something else..
Yes, I agree that this should only be on new code or 'while you are in
the area' i.e. the boy scout rule (leave the code better than you
found it).
I use IntelliJ - I will export my current JMeter code formatting rules
and also try to create one for Eclipse which broadly aligns.
If we put these into checkstyle would this fail the build, or can they
just be warnings?
I've created a wiki page to capture the key points:
https://wiki.apache.org/jmeter/CodeStyleGuidelines
>
>>
>> From Phillippe's Java 8 email:
>>
> * Use of Optional
>>
>
> From the document, I understand Optional has a memory/performance impact so
> we need to take this into account where we decide to apply it.
> But I'm ok with it.
Certainly, this is important with performance critical parts of the code.
>
>
>> * Default/static methods on Interfaces
>>
> +1
>
>> * Lambas where possible (max ~5 lines?)
>>
> +1 provided reduced code is more readable than existing one. If it's
> reduced but we have to scratch our head to understand it I'm not sure it's
> a gain :-)
Absolutely, this is all to do with readability, if it doesn't fit in
~5 lines then it should probably be moved to a method and named
appropriately.
>
> Regarding Streams, we need to be sure of performance impact. And It appears
> Sonar has some issues analyzing such code
Yes I noticed that too, is this due to the version of Sonar we are
using or to unidentified/unfixed bugs?
>
>
>> I have found this: https://wiki.apache.org/jmeter/JMeterEclipse
>> which suggests some formatting e.g. 80 char line length and new line before
>> opening brace but most of the code doesn't conform to that - it was also
>> updated 8 years ago.
>>
>> Any other ideas/thoughts?
>>
>> Thanks
>>
>> Graham
>
> --
> Cordialement.
> Philippe Mouawad.
Re: Code Style Guidelines
Posted by Philippe Mouawad <ph...@gmail.com>.
On Sat, Feb 11, 2017 at 11:27 PM, Graham Russell <gr...@ham1.co.uk> wrote:
> Hi all
>
> Do we have any (written) guidance on code style?
>
> I know we have checkstyle, but this has very few rules.
>
> The reason for asking is that I think having a consistent style is very
> important for readability (which helps writing new feature or fixing
> existing code etc.).
>
> The main things I think contribute to this are (and which are fairly easy
> to detect/fix):
> * (soft) line lengths (80, 100?)
> * spacing between elements on a line e.g. if (..., while (...,
> methodCall(arg1, arg2), "con" + "cat"
>
If this is related to String concat, we should only do that in places where
performance is not a critical point as IMU StringBuilder concat is still
better than '+'
> * import order, spacing and not using .*
> * line length of methods (soft limit of 50?)
> * line length of classes (soft limit of 500?)
>
I am ok with these proposals and providing a checkstyle and Eclipse
formatter might be very useful.
But if possible, I'd prefer that we do not modify too much code only for
code style as it might make it difficult to look into particular
regressions.
So maybe do that on new code or when changing a file for something else..
>
> From Phillippe's Java 8 email:
>
* Use of Optional
>
From the document, I understand Optional has a memory/performance impact so
we need to take this into account where we decide to apply it.
But I'm ok with it.
> * Default/static methods on Interfaces
>
+1
> * Lambas where possible (max ~5 lines?)
>
+1 provided reduced code is more readable than existing one. If it's
reduced but we have to scratch our head to understand it I'm not sure it's
a gain :-)
Regarding Streams, we need to be sure of performance impact. And It appears
Sonar has some issues analyzing such code
> I have found this: https://wiki.apache.org/jmeter/JMeterEclipse
> which suggests some formatting e.g. 80 char line length and new line before
> opening brace but most of the code doesn't conform to that - it was also
> updated 8 years ago.
>
> Any other ideas/thoughts?
>
> Thanks
>
> Graham
>
--
Cordialement.
Philippe Mouawad.