You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuscany.apache.org by Daniel Kulp <da...@iona.com> on 2006/04/20 13:25:36 UTC
Code style questions....
Ant brought up a point in the latest JIRA issue I filed for the axis2
binding. I just wanted to bring it up in it's own thread for
discussion.
As part of the Jira, I was removing all the eclipse warnings, but I also
was cleaning up the code to meet certain checkstyle and pmd rules. Of
course, the question is "where did those rules come from?" Well, for
the most part, I used the Celtix checkstyle and pmd rulesets. However,
I made the following changes:
1) HiddenField - for celtix, we NEVER allow hidden fields. However,
tuscany seems to use them in constructors and setters and a few other
places. I've changed the HiddenField check to allow them in the
constructors and setters. We can completely turn off this check if you
want. Personally, I hate hidden fields, but it's a voteable thing.
2) Line length. Celtix uses 110 which comes from IONA's coding standard.
(actually, IONA's coding standard still officially says 80, but no one
follows that and the checkstyle enforcing is all at 110) For the
axis2 binding, I had to set it at 115 as there is a class name that is
105 characters long that when you add "import " to the beginning makes it
too long. That said, the "eclipse" setup thing you have in etc is set
to 150 characters. The idea setup doesn't set anything. Personally, I
think 150 is WAY too long. I hate having to scroll to the right to see
the last 2 or so parameters on the method sigs. I'd be OK with 115 or
120. (although I still prefer 110)
What are others thoughts?
I've attached the checkstyle.xml and the pmd .ruleset files. Please take
a quick look and see if anything else jumps out at you. Information on
them are available at:
http://checkstyle.sourceforge.net/
http://pmd.sourceforge.net/
One thing to keep in mind: checkstyle is MUCH faster than pmd. The
more checks you add to pmd, the slower it gets. Thus, if checkstyle can
check it, don't have pmd check it. That's why you see most of the pmd
rules commented out.
Also, the maven checkstyle plugin can enforce it at build time. The pmd
one cannot in the current release. The latest SNAPSHOT can. It's
currently being voted on. If you look at the celtix binding pom.xml,
you can see it enforces the checkstyle rules. If we can agree on the
various rules, I'd LIKE to add the same rules to other areas of the code
as they are cleaned up. The jsonrpc binding is all set. The axis2
binding patch is in jira. Thus, the entire binding section could be
enabled.
Thanks!
--
J. Daniel Kulp
Principal Engineer
IONA
P: 781-902-8727 C: 508-380-7194
daniel.kulp@iona.com
Re: [OT] Bikesheds, was: Code style questions....
Posted by Jim Marino <ji...@gmail.com>.
Ah...I was assuming it was something like a woodshed :-)
On Apr 20, 2006, at 8:50 AM, Jeremy Boynes wrote:
> Jim Marino wrote:
>
>>
>> I missed the "bikeshed" metaphor but is that a place where "trout-
>> slapping" occurs"? ;-)
>>
>>
>
> http://en.wikipedia.org/wiki/Bikeshed_problem
>
> --
> Jeremy
>
[OT] Bikesheds, was: Code style questions....
Posted by Jeremy Boynes <jb...@apache.org>.
Jim Marino wrote:
>
> I missed the "bikeshed" metaphor but is that a place where "trout-
> slapping" occurs"? ;-)
>
http://en.wikipedia.org/wiki/Bikeshed_problem
--
Jeremy
Re: Code style questions....
Posted by Jim Marino <jm...@myromatours.com>.
On Apr 20, 2006, at 8:01 AM, Jeremy Boynes wrote:
> Comments inline.
>
> [[ side comment, I've been in code style discussions before that turn
> into a perfect illustration of a bikeshed - let's please close this
> quickly :-) ]]
I missed the "bikeshed" metaphor but is that a place where "trout-
slapping" occurs"? ;-)
> Daniel Kulp wrote:
>
>> Ant brought up a point in the latest JIRA issue I filed for the axis2
>> binding. I just wanted to bring it up in it's own thread for
>> discussion.
>>
>> As part of the Jira, I was removing all the eclipse warnings, but
>> I also
>> was cleaning up the code to meet certain checkstyle and pmd
>> rules. Of
>> course, the question is "where did those rules come from?"
>> Well, for
>> the most part, I used the Celtix checkstyle and pmd rulesets.
>> However,
>> I made the following changes:
>>
>> 1) HiddenField - for celtix, we NEVER allow hidden fields. However,
>> tuscany seems to use them in constructors and setters and a few other
>> places. I've changed the HiddenField check to allow them in the
>> constructors and setters. We can completely turn off this check
>> if you
>> want. Personally, I hate hidden fields, but it's a voteable thing.
>>
>>
>
> I'm OK with HiddenField violations and actually think there is some
> benefit in a consistent name in the private usage (implementation
> accessing the field) and public usage (JavaDoc'ed method parameter). I
> realize there is a risk of error due to someone omitting the "this"
> but
> that construct is flagged as a warning in IDEA.
>
> For information, the default IDEA codegen template for setters and
> constructors results in hidden fields.
>
>
>> 2) Line length. Celtix uses 110 which comes from IONA's coding
>> standard.
>> (actually, IONA's coding standard still officially says 80, but no
>> one
>> follows that and the checkstyle enforcing is all at 110) For the
>> axis2 binding, I had to set it at 115 as there is a class name
>> that is
>> 105 characters long that when you add "import " to the beginning
>> makes it
>> too long. That said, the "eclipse" setup thing you have in etc
>> is set
>> to 150 characters. The idea setup doesn't set anything.
>> Personally, I
>> think 150 is WAY too long. I hate having to scroll to the right
>> to see
>> the last 2 or so parameters on the method sigs. I'd be OK with
>> 115 or
>> 120. (although I still prefer 110)
>>
>> What are others thoughts?
>>
>>
>
> I am not in favour of automatic line wrapping at all. There is a
> subjective factor in how line breaks impact the readability of the
> code
> and I do not think IDEs are smart enough yet to factor that in. I
> still
> prefer to leave that to the judgement of the developer. Some guidance
> can be helpful - for example, having a margin displayed at a certain
> number of characters.
>
> Subjectively, I'm in the same ballpark as you at around 120 characters
> (that is the margin point in the IDEA setup). IIRC we set Eclipse
> to 150
> to prevent it auto-wrapping lines that did not make subjective
> sense to
> break but which just happened to creep over the 120 character bar.
>
I don't have a preference other than I would like to add checkstyle
to the build and we should be consistent, but avoid extremes when it
comes to line wrapping. Maybe we could have a check that enforced
something like not having three consecutive lines of too short or too
wide margins?
>
>> I've attached the checkstyle.xml and the pmd .ruleset files.
>> Please take
>> a quick look and see if anything else jumps out at you.
>> Information on
>> them are available at:
>> http://checkstyle.sourceforge.net/
>> http://pmd.sourceforge.net/
>>
>> One thing to keep in mind: checkstyle is MUCH faster than pmd.
>> The
>> more checks you add to pmd, the slower it gets. Thus, if
>> checkstyle can
>> check it, don't have pmd check it. That's why you see most of
>> the pmd
>> rules commented out.
>>
>> Also, the maven checkstyle plugin can enforce it at build time.
>> The pmd
>> one cannot in the current release. The latest SNAPSHOT can. It's
>> currently being voted on. If you look at the celtix binding
>> pom.xml,
>> you can see it enforces the checkstyle rules. If we can agree on the
>> various rules, I'd LIKE to add the same rules to other areas of
>> the code
>> as they are cleaned up. The jsonrpc binding is all set. The axis2
>> binding patch is in jira. Thus, the entire binding section could be
>> enabled.
>>
>>
>
> I'm not quite harsh enough to say that we should fail the build
> because
> (functionally good, well tested, cough) code does not meet code
> formatting guidelines. I would be OK with adding strict checks for
> code
> constructs that can lead to problems in the behaviour - taking the
> HiddenField example, I wouldn't want the build to fail because the
> variable had the wrong name but I would want a failure if you left out
> the "this" in a hidden assignment. Of course, that should be caught in
> unit testing but that's a different issue.
On formatting, I don't think we should be too strict but it would be
nice check some of the basic guidelines such as no braces on new lines.
>
> --
> Jeremy
>
Re: Code style questions....
Posted by Jeremy Boynes <jb...@apache.org>.
Comments inline.
[[ side comment, I've been in code style discussions before that turn
into a perfect illustration of a bikeshed - let's please close this
quickly :-) ]]
Daniel Kulp wrote:
> Ant brought up a point in the latest JIRA issue I filed for the axis2
> binding. I just wanted to bring it up in it's own thread for
> discussion.
>
> As part of the Jira, I was removing all the eclipse warnings, but I also
> was cleaning up the code to meet certain checkstyle and pmd rules. Of
> course, the question is "where did those rules come from?" Well, for
> the most part, I used the Celtix checkstyle and pmd rulesets. However,
> I made the following changes:
>
> 1) HiddenField - for celtix, we NEVER allow hidden fields. However,
> tuscany seems to use them in constructors and setters and a few other
> places. I've changed the HiddenField check to allow them in the
> constructors and setters. We can completely turn off this check if you
> want. Personally, I hate hidden fields, but it's a voteable thing.
>
I'm OK with HiddenField violations and actually think there is some
benefit in a consistent name in the private usage (implementation
accessing the field) and public usage (JavaDoc'ed method parameter). I
realize there is a risk of error due to someone omitting the "this" but
that construct is flagged as a warning in IDEA.
For information, the default IDEA codegen template for setters and
constructors results in hidden fields.
> 2) Line length. Celtix uses 110 which comes from IONA's coding standard.
> (actually, IONA's coding standard still officially says 80, but no one
> follows that and the checkstyle enforcing is all at 110) For the
> axis2 binding, I had to set it at 115 as there is a class name that is
> 105 characters long that when you add "import " to the beginning makes it
> too long. That said, the "eclipse" setup thing you have in etc is set
> to 150 characters. The idea setup doesn't set anything. Personally, I
> think 150 is WAY too long. I hate having to scroll to the right to see
> the last 2 or so parameters on the method sigs. I'd be OK with 115 or
> 120. (although I still prefer 110)
>
> What are others thoughts?
>
I am not in favour of automatic line wrapping at all. There is a
subjective factor in how line breaks impact the readability of the code
and I do not think IDEs are smart enough yet to factor that in. I still
prefer to leave that to the judgement of the developer. Some guidance
can be helpful - for example, having a margin displayed at a certain
number of characters.
Subjectively, I'm in the same ballpark as you at around 120 characters
(that is the margin point in the IDEA setup). IIRC we set Eclipse to 150
to prevent it auto-wrapping lines that did not make subjective sense to
break but which just happened to creep over the 120 character bar.
> I've attached the checkstyle.xml and the pmd .ruleset files. Please take
> a quick look and see if anything else jumps out at you. Information on
> them are available at:
> http://checkstyle.sourceforge.net/
> http://pmd.sourceforge.net/
>
> One thing to keep in mind: checkstyle is MUCH faster than pmd. The
> more checks you add to pmd, the slower it gets. Thus, if checkstyle can
> check it, don't have pmd check it. That's why you see most of the pmd
> rules commented out.
>
> Also, the maven checkstyle plugin can enforce it at build time. The pmd
> one cannot in the current release. The latest SNAPSHOT can. It's
> currently being voted on. If you look at the celtix binding pom.xml,
> you can see it enforces the checkstyle rules. If we can agree on the
> various rules, I'd LIKE to add the same rules to other areas of the code
> as they are cleaned up. The jsonrpc binding is all set. The axis2
> binding patch is in jira. Thus, the entire binding section could be
> enabled.
>
I'm not quite harsh enough to say that we should fail the build because
(functionally good, well tested, cough) code does not meet code
formatting guidelines. I would be OK with adding strict checks for code
constructs that can lead to problems in the behaviour - taking the
HiddenField example, I wouldn't want the build to fail because the
variable had the wrong name but I would want a failure if you left out
the "this" in a hidden assignment. Of course, that should be caught in
unit testing but that's a different issue.
--
Jeremy