Mailing-List: contact axis-dev-help@xml.apache.org; run by ezmlm
Precedence: bulk
Reply-To: axis-dev@xml.apache.org
Message-ID: 
 <CB1FF0A474AEA84EA0206D5B05F6A4CB025D7C93@S1001EXM02.macromedia.com>
From: Tom Jordahl <tomj@macromedia.com>
To: "'axis-dev@xml.apache.org'" <axis-dev@xml.apache.org>
Subject: RE: checkstyle on axis
Date: Fri, 30 Aug 2002 17:15:41 -0400
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"


A few observations/questions about checkstyle:

- It complains about public member elements in classes, for instance there are a ton of complains about the Holder types, which have a public "value" member.  And we like it that way.

- It complains about redundant 'public' and 'abstract' modifiers in interfaces, which confuses me a bit.  The interfaces it complains about are a bunch of Sun JAX-RPC classes, and you would think that *they* would know if these belonged there or not.

- I like the unused import errors, along with the brace-style complains, we should fix those (with the tool Steve mentioned if possible).

- It complains about empty catch blocks, which I might be OK but I kind of think that in most instances this is as intended.  See encoding.ser.BaseSeriaizerFactory.java for lots of examples.

- The error "'&&' should be on a new line." is complaining about the form that I consider the right thing, the && at the end of the line instead of the beginning.  I guess I would be willing to change if that Java style guide says different.


So, my recommended plan would be to :
- turn off the public member variable errors 
- turn off the redundant modifiers errors (at least for the javax.* files)
- Fix the braces across the code base
- Fix the imports across the code base
- Possibly fix the conditionals (&& || at the beginning of the line)

This should bring the number of errors down considerably, to something that we can then manage.

Opinions?  

--
Tom Jordahl
Macromedia Server Development


-----Original Message-----
From: Steve Loughran [mailto:steve_l@iseran.com]
Sent: Thursday, August 29, 2002 5:19 PM
To: axis-dev
Subject: checkstyle on axis


Axis has now its own checkstyle run; go ant checkstyle in the base dir with
checkstyle.jar downloaded from checkstyle.sf.net and placed into
ANT_HOME\lib. You need xalan there for the next stage of the build; a
transform of the XML report into HTML.

If you run the task in the Jedit command window BTw, you can even go from
the rolling error log to the lines, though 4000+ errors does give jedit
problems.

I have done the .* imports, JLS issues and errant tabs. If any more crop up
now, we can go through the CVS logs to see who put them there :)

This leaves the key diffs between axis source and the sun style rules as

-nowhere near enough javadocs (that is turned off in checkstyle as it was
too distracting)

-not enough use of braces after if()  and  else clauses. I know it's extra
work, I know it bloats methods, but it is important for maintenance.

-inconsistent variable naming. A bit of the _something style, some of the
m_something; these should be cleaned up in a refactoring IDE.

-too much scope given for things. Example, all the logs that classes create
that dont have private scope. This should be tightend up on a case by case
basis.

-lines too long. 68 chars is the rule, we have many over 100, some over 250.
Again, this causes maintenance and readability issues.

-different brace rules, traces of other styles in there.

Having now been into bits of the code I normally ignore, the lack of javadoc
comments and general readability in places worries me. The deliverables of
an open source project are not just the binaries and end user docs, they are
the source and the source docs, and Axis is weak there. If the source is
hard to work with, people wont work with it.

>From now on I'm only going to clean up files on a case by case basis, as and
when I work with them. It would seem a good habit to start by javadocing a
file when you get to work with it, then re-lay out the bracing; commit that
change then maybe do some of the other cleanups before anything else. The
aim should be that not only is the total number of checkstyle errors reduced
(4049 ignoring javadocs), but that whole files dont raise a single warning.

-Steve