Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
Style Checking and Enforcement for Java Code
338 views
Skip to first unread message
Gregory Szorc
Jan 6, 2012, 12:05:31 AM1/6/12
to
There has been a lot of Java code landing in mozilla-central lately. I
was wondering what people think of defining Mozilla style and coding
guidelines for Java and then empowering people to enforce these through
tools.
Currently, the best we seem to have is a wiki entry [1] and we leave it
up to the review process to catch transgressions. This is a good start.
But, no matter how good of a reviewer you are, you'll never been as good
as a dedicated tool. Besides, as a reviewer, you should be using your
brain cycles on assessing the quality of code, not compiling it.
I think we can do better. I'm not sure what others will think, but here
is my proposal to get the conversation started:
1) Create "blessed" configurations for popular tools like Checkstyle [2]
2) Add these configurations to mozilla-central and hook up build targets
so they can be executed easily. Run them by default during builds (when
it makes sense) to raise awareness of violations (much like compiler
warnings aren't hidden during silent builds). Perhaps they are disabled
on the build farm initially because it would be a lot of work/overhead
to deploy them. But, they would eventually run during all builds.
3) Encourage action from tool output (likely enforced through review
process)
4) Tailor the configurations to our liking over the coming weeks/months
5) Down the road, establish a flag day where builds go orange if the
automated analysis produces unacceptable errors. Many tools allow you to
assign a severity to a transgression. We can initially narrowly define
the "orange set" so there is little pain to initially enable build failures.
6) Over time, gradually make the policies more strict and fix warnings
as we go. One idea would be to bump up the strictness of each warning
type in each release cycle until all warnings cross the threshold and
cause build failures.
7) Arrive at a state where we are happy with the strictness and there
are no static analysis warnings [and the code is more uniform and less
bug-prone].
One thing to note is that most Java tools allow you to suppress warnings
on a line, file, or package level, much like how C/C++ compiler warnings
work with #pragma or CLI flags, so it is likely no policy would be
totalitarian. You do pay a small price for working around them, but I
personally think a reminder before you break the law is a good thing.
While this could apply to other languages in the tree, I'd like to limit
the discussion to Java because a) there isn't much Java in the tree and
thus the problem is more manageable b) there are a bunch of new-to-Java
coders who would probably benefit significantly from static analysis
assistance c) the tools for Java are great (probably the best of any
language).
Thoughts?
[1] https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle
[2] http://checkstyle.sourceforge.net/
was wondering what people think of defining Mozilla style and coding
guidelines for Java and then empowering people to enforce these through
tools.
Currently, the best we seem to have is a wiki entry [1] and we leave it
up to the review process to catch transgressions. This is a good start.
But, no matter how good of a reviewer you are, you'll never been as good
as a dedicated tool. Besides, as a reviewer, you should be using your
brain cycles on assessing the quality of code, not compiling it.
I think we can do better. I'm not sure what others will think, but here
is my proposal to get the conversation started:
1) Create "blessed" configurations for popular tools like Checkstyle [2]
2) Add these configurations to mozilla-central and hook up build targets
so they can be executed easily. Run them by default during builds (when
it makes sense) to raise awareness of violations (much like compiler
warnings aren't hidden during silent builds). Perhaps they are disabled
on the build farm initially because it would be a lot of work/overhead
to deploy them. But, they would eventually run during all builds.
3) Encourage action from tool output (likely enforced through review
process)
4) Tailor the configurations to our liking over the coming weeks/months
5) Down the road, establish a flag day where builds go orange if the
automated analysis produces unacceptable errors. Many tools allow you to
assign a severity to a transgression. We can initially narrowly define
the "orange set" so there is little pain to initially enable build failures.
6) Over time, gradually make the policies more strict and fix warnings
as we go. One idea would be to bump up the strictness of each warning
type in each release cycle until all warnings cross the threshold and
cause build failures.
7) Arrive at a state where we are happy with the strictness and there
are no static analysis warnings [and the code is more uniform and less
bug-prone].
One thing to note is that most Java tools allow you to suppress warnings
on a line, file, or package level, much like how C/C++ compiler warnings
work with #pragma or CLI flags, so it is likely no policy would be
totalitarian. You do pay a small price for working around them, but I
personally think a reminder before you break the law is a good thing.
While this could apply to other languages in the tree, I'd like to limit
the discussion to Java because a) there isn't much Java in the tree and
thus the problem is more manageable b) there are a bunch of new-to-Java
coders who would probably benefit significantly from static analysis
assistance c) the tools for Java are great (probably the best of any
language).
Thoughts?
[1] https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle
[2] http://checkstyle.sourceforge.net/
Doug Turner
Jan 6, 2012, 12:43:54 AM1/6/12
to Gregory Szorc, dev-pl...@lists.mozilla.org
Nice idea Gregory! I don't mind this at all, but the people writing Java code (mobile peoples mostly) have higher priority tasks at the moment. If you want to write the tooling to do this, patches happily accepted! However, I'd focus more on tooling that directly improves developer time - such as bug 715242. Or fixing the 100+ build warnings that we have. :D
Regards,
Doug Turner
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
Regards,
Doug Turner
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
Kartikaya Gupta
Jan 6, 2012, 9:01:42 AM1/6/12
to Doug Turner, Gregory Szorc, dev-pl...@lists.mozilla.org
On 12-01-06 00:43 , Doug Turner wrote:
> However, I'd focus more on tooling that directly improves developer time - such as bug 715242.
FWIW, In the long run, I feel enforcing good code style will be much
> However, I'd focus more on tooling that directly improves developer time - such as bug 715242.
more beneficial than fixing bug 715242. And more importantly, it will
result in a higher quality product delivered to end users.
kats
Kartikaya Gupta
Jan 6, 2012, 9:01:42 AM1/6/12
to Doug Turner, dev-pl...@lists.mozilla.org, Gregory Szorc
On 12-01-06 00:43 , Doug Turner wrote:
> However, I'd focus more on tooling that directly improves developer time - such as bug 715242.
Mark Finkle
Jan 6, 2012, 9:55:26 AM1/6/12
to Gregory Szorc
On 01/06/2012 12:05 AM, Gregory Szorc wrote:
> There has been a lot of Java code landing in mozilla-central lately. I
> was wondering what people think of defining Mozilla style and coding
> guidelines for Java and then empowering people to enforce these through
> tools.
My first thought is "less is more." I don't want to start thread wars on
> There has been a lot of Java code landing in mozilla-central lately. I
> was wondering what people think of defining Mozilla style and coding
> guidelines for Java and then empowering people to enforce these through
> tools.
the righteousness of bracing styles. Let's focus on things that actually
yield better quality code.
Given that, I'm OK with starting the process. In fact, I have been
playing around with static analysis tools for Java (Android) like
FindBugs[1] and PMD[2]. I even found a few bugs (patched and fixed)
using the tools. Perhaps getting those tools into our tool chain would
be more valuable. Finding potential null pointer exceptions and
threading issues, as well as, unneeded imports and unused variables
could have higher value.
[1] http://findbugs.sourceforge.net
[2] http://pmd.sourceforge.net
Chris Peterson
Jan 6, 2012, 12:52:32 PM1/6/12
to Gregory Szorc
On 1/5/12 9:05 PM, Gregory Szorc wrote:
> There has been a lot of Java code landing in mozilla-central lately. I
> was wondering what people think of defining Mozilla style and coding
> guidelines for Java and then empowering people to enforce these through
> tools.
This is a good idea, though breaking the build for style violations
> There has been a lot of Java code landing in mozilla-central lately. I
> was wondering what people think of defining Mozilla style and coding
> guidelines for Java and then empowering people to enforce these through
> tools.
might be premature. <:)
I think important first steps would be:
1. Escalate our Java coding style guidelines from the Fennec wiki [1] to
Mozilla's developer wiki [2] to increase visibility. I didn't know about
the Fennec wiki's guidelines until recently!
2. Codify the guidelines in a Checkstyle config (and possibly vim and
emacs formats, too).
[1] https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle#Java
[2] https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
chris
Gregory Szorc
Jan 6, 2012, 1:05:38 PM1/6/12
to Chris Peterson
I don't suppose there is a single authoritative repository with all the
style/analysis/etc configs for different tools? I think it would be
pretty splendid if I could just clone a repo and instantly have access
to "policy" files for C/C++/JS/Java/Python/etc. Google has this as part
of their style guide [1] and it is very handy. Another benefit is I
trust policy defined in a checked in file more than what I find on a wiki.
[1] https://code.google.com/p/google-styleguide/source/browse/trunk
Greg
style/analysis/etc configs for different tools? I think it would be
pretty splendid if I could just clone a repo and instantly have access
to "policy" files for C/C++/JS/Java/Python/etc. Google has this as part
of their style guide [1] and it is very handy. Another benefit is I
trust policy defined in a checked in file more than what I find on a wiki.
[1] https://code.google.com/p/google-styleguide/source/browse/trunk
Greg
Jeff Walden
Jan 6, 2012, 2:23:05 PM1/6/12
to
On 01/05/2012 11:05 PM, Gregory Szorc wrote:
> There has been a lot of Java code landing in mozilla-central lately. I
> was wondering what people think of defining Mozilla style and coding
> guidelines for Java and then empowering people to enforce these through
> tools.
Fewer style discussions is a win, I say. (If it were up to me, Mozilla would have one style [well, maybe two given below], and everybody'd have to suck it up.) But then again, I work on non-Java code, so my opinion means little.
> There has been a lot of Java code landing in mozilla-central lately. I
> was wondering what people think of defining Mozilla style and coding
> guidelines for Java and then empowering people to enforce these through
> tools.
I thought (perhaps wrongly? been years since I wrote Java much) that pretty much everyone followed Sun's conventions -- lowercase package names, InterCaps classes, camelCaps methods and fields, four-space indentation, opening brace at end of line, closing brace alone on a line, probably other things that I can't remember from the last time I touched Java code. Why wouldn't we choose to do (and already be doing) the same?
Jeff
Kartikaya Gupta
Jan 6, 2012, 3:33:39 PM1/6/12
to
On 12-01-06 14:23 , Jeff Walden wrote:
> I thought (perhaps wrongly? been years since I wrote Java much) that
> pretty much everyone followed Sun's conventions -- lowercase package
> names, InterCaps classes, camelCaps methods and fields, four-space
> indentation, opening brace at end of line, closing brace alone on a
> line, probably other things that I can't remember from the last time I
> touched Java code. Why wouldn't we choose to do (and already be doing)
> the same?
Because some of the people writing Java code at Mozilla (at least for
> I thought (perhaps wrongly? been years since I wrote Java much) that
> pretty much everyone followed Sun's conventions -- lowercase package
> names, InterCaps classes, camelCaps methods and fields, four-space
> indentation, opening brace at end of line, closing brace alone on a
> line, probably other things that I can't remember from the last time I
> touched Java code. Why wouldn't we choose to do (and already be doing)
> the same?
Android) don't have a lot of previous experience in Java and aren't
familiar with these conventions. To some extent they also conflict with
existing Mozilla C++/JS conventions.
kats
Gregory Szorc
Jan 6, 2012, 6:33:13 PM1/6/12
to
On 1/5/12 9:43 PM, Doug Turner wrote:
> If you want to write the tooling to do this, patches happily
> accepted!
I threw together a Checkstyle XML config file which I /think/ captures
> If you want to write the tooling to do this, patches happily
> accepted!
most of Sun's coding conventions and the variations listed from the
Android wiki tree. It spews lots of violations, but many of these are
very simple things that could be corrected with an automated run through
Eclipse's source reformatter. But, that would require an Eclipse style
config file...
I checked in the code at
https://github.com/mozilla-services/mozilla-style. I can move that to
the main Mozilla GitHub account if people want (if not an HG repo on
mozilla.org). Otherwise, feel free to fork and submit pull requests.
I'll also submit some bugs to get build system targets working.
Greg
Chris Peterson
Jan 6, 2012, 7:33:57 PM1/6/12
to Gregory Szorc
> I threw together a Checkstyle XML config file which I /think/ captures
> most of Sun's coding conventions and the variations listed from the
> Android wiki tree. It spews lots of violations, but many of these are
> very simple things that could be corrected with an automated run through
Thanks, Greg. Your checkstyle config looks very good. There are some
> most of Sun's coding conventions and the variations listed from the
> Android wiki tree. It spews lots of violations, but many of these are
> very simple things that could be corrected with an automated run through
pedantic checks that we'll probably want to relax for our code [1].
I would recommend commenting out the checkstyle rules we currently fail
(with FIXME comments). This will make integrating checkstyle easier
while preventing new style infractions. Once the mechanism is in place,
we can open a bug to debate which failing rules should be fixed or
ignored. :)
[1] Such as AvoidInlineConditionals, EmptyBlock, FinalParameters,
JavadocPackage, and VisibilityModifier.
chris
Gregory Szorc
Jan 6, 2012, 8:07:16 PM1/6/12
to
On 1/6/12 4:33 PM, Chris Peterson wrote:
>> I threw together a Checkstyle XML config file which I /think/ captures
>> most of Sun's coding conventions and the variations listed from the
>> Android wiki tree. It spews lots of violations, but many of these are
>> very simple things that could be corrected with an automated run through
>
> Thanks, Greg. Your checkstyle config looks very good. There are some
> pedantic checks that we'll probably want to relax for our code [1].
>
> I would recommend commenting out the checkstyle rules we currently fail
> (with FIXME comments). This will make integrating checkstyle easier
> while preventing new style infractions. Once the mechanism is in place,
> we can open a bug to debate which failing rules should be fixed or
> ignored. :)
https://bugzilla.mozilla.org/show_bug.cgi?id=716137 filed to integrate
>> I threw together a Checkstyle XML config file which I /think/ captures
>> most of Sun's coding conventions and the variations listed from the
>> Android wiki tree. It spews lots of violations, but many of these are
>> very simple things that could be corrected with an automated run through
>
> Thanks, Greg. Your checkstyle config looks very good. There are some
> pedantic checks that we'll probably want to relax for our code [1].
>
> I would recommend commenting out the checkstyle rules we currently fail
> (with FIXME comments). This will make integrating checkstyle easier
> while preventing new style infractions. Once the mechanism is in place,
> we can open a bug to debate which failing rules should be fixed or
> ignored. :)
with the build system.
As far as commenting out rules, I think we should instead lower the
severity of violations. I feel it would only make matters worse if valid
issues were covered up completely and then suddenly sprung on people
later. Instead, we can expose them with a lower severity and people can
filter out the ones they don't care about (the tooling makes this pretty
painless).
Jeff Walden
Jan 7, 2012, 1:45:58 AM1/7/12
to
On 01/06/2012 02:33 PM, Kartikaya Gupta wrote:
> Because some of the people writing Java code at Mozilla (at least for Android) don't have a lot of previous experience in Java and aren't familiar with these conventions.
This I buy. No time like now for those people to learn, tho. :-)
> Because some of the people writing Java code at Mozilla (at least for Android) don't have a lot of previous experience in Java and aren't familiar with these conventions.
> To some extent they also conflict with existing Mozilla C++/JS conventions.
But again, not my turf, so my opinion means little. :-)
Jeff
michae...@live.com
Jan 7, 2012, 6:48:01 AM1/7/12
to jwald...@mit.edu, dev-pl...@lists.mozilla.org
Hi, just one little question: isn't the Sun/Oracle Java Code Convention[1] a little bit outdated? Of course there are quite a lot of still valid points but there are also some strange recommondations (e.g. switch).
[1] http://www.oracle.com/technetwork/java/codeconv-138413.html
---
Sent from my mobile phone..
Jeff
Cameron McCormack
Jan 7, 2012, 6:13:47 PM1/7/12
to michae...@live.com, dev-pl...@lists.mozilla.org, jwald...@mit.edu
michae...@live.com:
incorrectly. If you look at the PDF, the switch formatting
recommendation seems much more normal.
> Hi, just one little question: isn't the Sun/Oracle Java Code
> Convention[1] a little bit outdated? Of course there are quite a lot
> of still valid points but there are also some strange recommondations
> (e.g. switch).
Note that the "View HTML" version of that page seems to be formatted
> Convention[1] a little bit outdated? Of course there are quite a lot
> of still valid points but there are also some strange recommondations
> (e.g. switch).
incorrectly. If you look at the PDF, the switch formatting
recommendation seems much more normal.
0 new messages