You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuscany.apache.org by Luciano Resende <lu...@gmail.com> on 2008/12/04 22:46:46 UTC
Trunk Code clean up
I have noticed that Mark has been doing a great job helping us
cleaning up the 2.0 code base [1], thanks a lot Mark.
Considering others might want to help, below is a list of possible
items to look when reviewing the code, we could probably grow this
into a Tuscany Code Guideline.
Also, using a tool such as PMD (or eclipse PMD plugin) might help.
Clean code
- use correct visibility, private, default, public, avoid protected
- make methods static if not using object state
- make sure javadoc is in sync or remove that javadoc
- no javadoc on overridden methods
- test cases in same package to avoid having to over-open access to methods
- don't create artificial dependencies by using constants from another module
- don't extend/implement a 'constant' interface
- avoid creating another private layer over a public interface/spi
- remove old code, don't leave it commented out, very confusing
- use functional programming names for functions that convert an object
- add javadoc to private methods
- review class javadoc and make sure it's accurate
- inline methods used only once, or make them clean static functions
- put utility methods in a Util class with package visibility
- use static imports
- use scoped variables
- no stars in OSGi exports
- correct use of generics, see the effective Java book
Unit/Integration Tests
- put comments in test cases
- review test cases and make sure they're included in the build
- use junit 4 only, check correct use of @BeforeClass or @Before
- use static imports for assert statements
Formatting
- use Tuscany eclipse code style/formatter
- no tabs
- no excessive line wrapping
[1] http://www.mail-archive.com/search?q=mcombellack&l=commits@tuscany.apache.org&start=0
--
Luciano Resende
Apache Tuscany, Apache PhotArk
http://people.apache.org/~lresende
http://lresende.blogspot.com/
[2.x] RE: Trunk Code clean up
Posted by Mark Combellack <mc...@apache.org>.
Hi Luciano,
I've just been doing bits of code tidy up as I find them. No particular
strategy or direction.
The plugins I have been using in Eclipse are:
* Checkstyle
* FindBugs
* PMD
Currently, there are too many problems found by these tools to be able to
see which ones are "proper" problems that should be fixed and which ones are
just "noise" and can be ignored. I'll just keep tidying up and hopefully,
the number of problems can be reduced.
Thanks,
Mark
> -----Original Message-----
> From: Luciano Resende [mailto:luckbr1975@gmail.com]
> Sent: 04 December 2008 21:47
> To: dev@tuscany.apache.org
> Subject: Trunk Code clean up
>
> I have noticed that Mark has been doing a great job helping us
> cleaning up the 2.0 code base [1], thanks a lot Mark.
>
> Considering others might want to help, below is a list of possible
> items to look when reviewing the code, we could probably grow this
> into a Tuscany Code Guideline.
> Also, using a tool such as PMD (or eclipse PMD plugin) might help.
>
> Clean code
> - use correct visibility, private, default, public, avoid protected
> - make methods static if not using object state
> - make sure javadoc is in sync or remove that javadoc
> - no javadoc on overridden methods
> - test cases in same package to avoid having to over-open access to
> methods
> - don't create artificial dependencies by using constants from another
> module
> - don't extend/implement a 'constant' interface
> - avoid creating another private layer over a public interface/spi
> - remove old code, don't leave it commented out, very confusing
> - use functional programming names for functions that convert an object
> - add javadoc to private methods
> - review class javadoc and make sure it's accurate
> - inline methods used only once, or make them clean static functions
> - put utility methods in a Util class with package visibility
> - use static imports
> - use scoped variables
> - no stars in OSGi exports
> - correct use of generics, see the effective Java book
>
> Unit/Integration Tests
> - put comments in test cases
> - review test cases and make sure they're included in the build
> - use junit 4 only, check correct use of @BeforeClass or @Before
> - use static imports for assert statements
>
> Formatting
> - use Tuscany eclipse code style/formatter
> - no tabs
> - no excessive line wrapping
>
>
> [1] http://www.mail-
> archive.com/search?q=mcombellack&l=commits@tuscany.apache.org&start=0
>
> --
> Luciano Resende
> Apache Tuscany, Apache PhotArk
> http://people.apache.org/~lresende
> http://lresende.blogspot.com/
Re: Trunk Code clean up
Posted by Luciano Resende <lu...@gmail.com>.
On Mon, Dec 8, 2008 at 10:26 AM, Raymond Feng <en...@gmail.com> wrote:
> As we can expect, some of the rules are subjective. I don't see that as a
> big issue. Any attempt to make the code less error-prone and more readable
> should be appraised and appreciated.
+1
>
> BTW, a few comments inline too :-).
>
> Thanks,
> Raymond
> --------------------------------------------------
> From: "Simon Nash" <na...@apache.org>
> Sent: Sunday, December 07, 2008 9:01 AM
> To: <de...@tuscany.apache.org>
> Subject: Re: Trunk Code clean up
>
>> Luciano Resende wrote:
>>>
>>> I have noticed that Mark has been doing a great job helping us
>>> cleaning up the 2.0 code base [1], thanks a lot Mark.
>>>
>>> Considering others might want to help, below is a list of possible
>>> items to look when reviewing the code, we could probably grow this
>>> into a Tuscany Code Guideline.
>>
>> >
>> Good suggestions, but I'm not sure about a few of these. See comments
>> below.
>>
>>> Also, using a tool such as PMD (or eclipse PMD plugin) might help.
>>>
>>> Clean code
>>> - use correct visibility, private, default, public, avoid protected
>>
>> >
>> I don't see anything wrong with using protected. What else would we
>> use for methods that should only be visible to subclasses?
>>
>
> "protected" should be only used when it's necessary for subclassing :-)
>
>>> - make methods static if not using object state
>>
>> >
>> I would not make this a hard and fast rule. IMO the main reason for
>> making a method static is to allow ti to be called when an instance
>> is not available.
>>
>>> - make sure javadoc is in sync or remove that javadoc
>>> - no javadoc on overridden methods
>>> - test cases in same package to avoid having to over-open access to
>>> methods
>>> - don't create artificial dependencies by using constants from another
>>> module
>>
>> >
>> This could lead to a lot of duplicate constant definitions in different
>> modules - not necessarily a good thing.
>
> We should use good judgment for public constants. It's similar to utility
> classes.
>
+1 for Good judgmet, I guess it's more like trying to avoid adding a
dependency on another module just to use a constant.
>>
>>> - don't extend/implement a 'constant' interface
>>
>> >
>> Why not?
>
> A better way could be using "import static". I think implementing constant
> interface is a workaround before "import static" is added for Java 1.5.
>
>>
>>> - avoid creating another private layer over a public interface/spi
>>> - remove old code, don't leave it commented out, very confusing
>>
>> >
>> Sometimes there is good reason for leaving it there. This can be
>> very helpful when making a tentative change that is likely to be
>> revisited in the future.
>>
>>> - use functional programming names for functions that convert an object
>>> - add javadoc to private methods
>>> - review class javadoc and make sure it's accurate
>>> - inline methods used only once, or make them clean static functions
>>
>> >
>> I don't see any problem with an out-of-line non-static method that
>> happens to be used only once. If used appropriately, these can
>> make code easier to read.
>>
>
> Use good balance here. Organize the methods by meaningful functional blocks.
> Avoid methods that have pages of code and one-time methods that only have
> one or two lines.
>
>> Simon
>>
>>> - put utility methods in a Util class with package visibility
>>> - use static imports
>>> - use scoped variables
>>> - no stars in OSGi exports
>>> - correct use of generics, see the effective Java book
>>>
>>> Unit/Integration Tests
>>> - put comments in test cases
>>> - review test cases and make sure they're included in the build
>>> - use junit 4 only, check correct use of @BeforeClass or @Before
>>> - use static imports for assert statements
>>>
>>> Formatting
>>> - use Tuscany eclipse code style/formatter
>>> - no tabs
>>> - no excessive line wrapping
>>>
>>>
>>> [1]
>>> http://www.mail-archive.com/search?q=mcombellack&l=commits@tuscany.apache.org&start=0
>>>
>>
>>
>
--
Luciano Resende
Apache Tuscany, Apache PhotArk
http://people.apache.org/~lresende
http://lresende.blogspot.com/
Re: Trunk Code clean up
Posted by Raymond Feng <en...@gmail.com>.
As we can expect, some of the rules are subjective. I don't see that as a
big issue. Any attempt to make the code less error-prone and more readable
should be appraised and appreciated.
BTW, a few comments inline too :-).
Thanks,
Raymond
--------------------------------------------------
From: "Simon Nash" <na...@apache.org>
Sent: Sunday, December 07, 2008 9:01 AM
To: <de...@tuscany.apache.org>
Subject: Re: Trunk Code clean up
> Luciano Resende wrote:
>> I have noticed that Mark has been doing a great job helping us
>> cleaning up the 2.0 code base [1], thanks a lot Mark.
>>
>> Considering others might want to help, below is a list of possible
>> items to look when reviewing the code, we could probably grow this
>> into a Tuscany Code Guideline.
> >
> Good suggestions, but I'm not sure about a few of these. See comments
> below.
>
>> Also, using a tool such as PMD (or eclipse PMD plugin) might help.
>>
>> Clean code
>> - use correct visibility, private, default, public, avoid protected
> >
> I don't see anything wrong with using protected. What else would we
> use for methods that should only be visible to subclasses?
>
"protected" should be only used when it's necessary for subclassing :-)
>> - make methods static if not using object state
> >
> I would not make this a hard and fast rule. IMO the main reason for
> making a method static is to allow ti to be called when an instance
> is not available.
>
>> - make sure javadoc is in sync or remove that javadoc
>> - no javadoc on overridden methods
>> - test cases in same package to avoid having to over-open access to
>> methods
>> - don't create artificial dependencies by using constants from another
>> module
> >
> This could lead to a lot of duplicate constant definitions in different
> modules - not necessarily a good thing.
We should use good judgment for public constants. It's similar to utility
classes.
>
>> - don't extend/implement a 'constant' interface
> >
> Why not?
A better way could be using "import static". I think implementing constant
interface is a workaround before "import static" is added for Java 1.5.
>
>> - avoid creating another private layer over a public interface/spi
>> - remove old code, don't leave it commented out, very confusing
> >
> Sometimes there is good reason for leaving it there. This can be
> very helpful when making a tentative change that is likely to be
> revisited in the future.
>
>> - use functional programming names for functions that convert an object
>> - add javadoc to private methods
>> - review class javadoc and make sure it's accurate
>> - inline methods used only once, or make them clean static functions
> >
> I don't see any problem with an out-of-line non-static method that
> happens to be used only once. If used appropriately, these can
> make code easier to read.
>
Use good balance here. Organize the methods by meaningful functional blocks.
Avoid methods that have pages of code and one-time methods that only have
one or two lines.
> Simon
>
>> - put utility methods in a Util class with package visibility
>> - use static imports
>> - use scoped variables
>> - no stars in OSGi exports
>> - correct use of generics, see the effective Java book
>>
>> Unit/Integration Tests
>> - put comments in test cases
>> - review test cases and make sure they're included in the build
>> - use junit 4 only, check correct use of @BeforeClass or @Before
>> - use static imports for assert statements
>>
>> Formatting
>> - use Tuscany eclipse code style/formatter
>> - no tabs
>> - no excessive line wrapping
>>
>>
>> [1]
>> http://www.mail-archive.com/search?q=mcombellack&l=commits@tuscany.apache.org&start=0
>>
>
>
Re: Trunk Code clean up
Posted by Simon Nash <na...@apache.org>.
Luciano Resende wrote:
> I have noticed that Mark has been doing a great job helping us
> cleaning up the 2.0 code base [1], thanks a lot Mark.
>
> Considering others might want to help, below is a list of possible
> items to look when reviewing the code, we could probably grow this
> into a Tuscany Code Guideline.
>
Good suggestions, but I'm not sure about a few of these. See comments
below.
> Also, using a tool such as PMD (or eclipse PMD plugin) might help.
>
> Clean code
> - use correct visibility, private, default, public, avoid protected
>
I don't see anything wrong with using protected. What else would we
use for methods that should only be visible to subclasses?
> - make methods static if not using object state
>
I would not make this a hard and fast rule. IMO the main reason for
making a method static is to allow ti to be called when an instance
is not available.
> - make sure javadoc is in sync or remove that javadoc
> - no javadoc on overridden methods
> - test cases in same package to avoid having to over-open access to methods
> - don't create artificial dependencies by using constants from another module
>
This could lead to a lot of duplicate constant definitions in different
modules - not necessarily a good thing.
> - don't extend/implement a 'constant' interface
>
Why not?
> - avoid creating another private layer over a public interface/spi
> - remove old code, don't leave it commented out, very confusing
>
Sometimes there is good reason for leaving it there. This can be
very helpful when making a tentative change that is likely to be
revisited in the future.
> - use functional programming names for functions that convert an object
> - add javadoc to private methods
> - review class javadoc and make sure it's accurate
> - inline methods used only once, or make them clean static functions
>
I don't see any problem with an out-of-line non-static method that
happens to be used only once. If used appropriately, these can
make code easier to read.
Simon
> - put utility methods in a Util class with package visibility
> - use static imports
> - use scoped variables
> - no stars in OSGi exports
> - correct use of generics, see the effective Java book
>
> Unit/Integration Tests
> - put comments in test cases
> - review test cases and make sure they're included in the build
> - use junit 4 only, check correct use of @BeforeClass or @Before
> - use static imports for assert statements
>
> Formatting
> - use Tuscany eclipse code style/formatter
> - no tabs
> - no excessive line wrapping
>
>
> [1] http://www.mail-archive.com/search?q=mcombellack&l=commits@tuscany.apache.org&start=0
>
Re: Trunk Code clean up
Posted by Dan Becker <da...@gmail.com>.
Great tips Luciano. I've added them to our Tuscany Development Guide
Coding guidelines [1]. (This may take a few moments to propagate from
the wiki to the exported HTML.)
[1]
http://tuscany.apache.org/sca-java-development-guide.html#SCAJavaDevelopmentGuide-CodingGuidelines
Luciano Resende wrote:
> I have noticed that Mark has been doing a great job helping us
> cleaning up the 2.0 code base [1], thanks a lot Mark.
>
> Considering others might want to help, below is a list of possible
> items to look when reviewing the code, we could probably grow this
> into a Tuscany Code Guideline.
> Also, using a tool such as PMD (or eclipse PMD plugin) might help.
>
> Clean code
> - use correct visibility, private, default, public, avoid protected
>
> Unit/Integration Tests
> - put comments in test cases
>
> Formatting
> - use Tuscany eclipse code style/formatter
>
>
> [1] http://www.mail-archive.com/search?q=mcombellack&l=commits@tuscany.apache.org&start=0
>
--
Thanks, Dan Becker