style issues(show me the code)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

style issues(show me the code)

Adam Heath-2
I'm working on some stuff that will encapsulate the style issues I
have been commenting on.  Then, if the styles are not actually what
others want, we can change the code.  Then, developers can run the
tools against their changes, to see if they pass muster.  This is a
way to automate it; let the computer do what it does best.

Currently, I'm implementing a java diff algorithm; I tried to find one
that is compatible with apache, but everything seems to just want to
reimplement GNU diff; I'm reading *just*
http://c2.com/cgi/wiki?DiffAlgorithm as a base.

Once I have that, I'll then start working on an xslt, one for each xml
schema(possible shared), which will then read the source xml, output
new xml formatted appropriately, then diff to the original, and show
the output.  This will require developers to be able to read diff
output, but that isn't hard.

I'll also be looking for java code formatting tools; anyone have any
suggestions that are apache compatible(has to be free, won't use
commercial things at all(don't get me started on confluence)).
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Adam Heath-2
Adam Heath wrote:
> Currently, I'm implementing a java diff algorithm; I tried to find one
> that is compatible with apache, but everything seems to just want to
> reimplement GNU diff; I'm reading *just*
> http://c2.com/cgi/wiki?DiffAlgorithm as a base.

Hmm.  Ean just called me to talk about work stuff, and he suggested I
look at eclipse.  While the license of eclipse may be questionable(can
other comment on that), it does give me some more things to do google
searches on; any java ide that can show diffs.  Again, others can
offer suggestions here.

Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Scott Gray-2
In reply to this post by Adam Heath-2
On 4/02/2010, at 11:41 PM, Adam Heath wrote:

> I'll also be looking for java code formatting tools; anyone have any
> suggestions that are apache compatible(has to be free, won't use
> commercial things at all(don't get me started on confluence)).

PMD (http://pmd.sourceforge.net/) uses a BSD style license and may be worth a look.

Regards
Scott

smime.p7s (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Adam Heath-2
Scott Gray wrote:
> On 4/02/2010, at 11:41 PM, Adam Heath wrote:
>
>> I'll also be looking for java code formatting tools; anyone have any
>> suggestions that are apache compatible(has to be free, won't use
>> commercial things at all(don't get me started on confluence)).
>
> PMD (http://pmd.sourceforge.net/) uses a BSD style license and may be worth a look.

Hmm.  Looking at controversial rulesets makes me interested; I bet the
other rulesets are equally enjoyable.  Thanks, will try it out.
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Erwan de FERRIERES
In reply to this post by Scott Gray-2
Le 05/02/2010 08:33, Scott Gray a écrit :

> On 4/02/2010, at 11:41 PM, Adam Heath wrote:
>
>> I'll also be looking for java code formatting tools; anyone have any
>> suggestions that are apache compatible(has to be free, won't use
>> commercial things at all(don't get me started on confluence)).
>
> PMD (http://pmd.sourceforge.net/) uses a BSD style license and may be worth a look.
>
> Regards
> Scott
I have already the PMD task done, just add those lines in the build.xml
file, and add the relevant libraries in framework/base/lib

        <path id="cq.classpath">
         <pathelement location="."/>
         <fileset dir="framework/base/lib/">
             <include name="*.jar"/>
         </fileset>
     </path>
         <target name="pmd">
             <taskdef name="pmd"
classname="net.sourceforge.pmd.ant.PMDTask" classpathref="cq.classpath"/>
             <pmd shortFilenames="true" rulesetfiles="basic">
                 <formatter type="xml" toFile="runtime/pmd.xml" />
                 <fileset dir="." includes="**/*.java" />
             </pmd>
         </target>

You can also use checkstyle, which will compare code to rules
(http://checkstyle.sourceforge.net)

--
Erwan de FERRIERES
www.nereide.biz
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Adam Heath-2
In reply to this post by Adam Heath-2
Adam Heath wrote:

> Scott Gray wrote:
>> On 4/02/2010, at 11:41 PM, Adam Heath wrote:
>>
>>> I'll also be looking for java code formatting tools; anyone have any
>>> suggestions that are apache compatible(has to be free, won't use
>>> commercial things at all(don't get me started on confluence)).
>> PMD (http://pmd.sourceforge.net/) uses a BSD style license and may be worth a look.
>
> Hmm.  Looking at controversial rulesets makes me interested; I bet the
> other rulesets are equally enjoyable.  Thanks, will try it out.

Oh, PMD, how I love thee, let me count the ways.  Oh, whoops, I just
had a StackOverFlow.  I guess that means I've finally found my true
love.  Let's get married and make beautiful babies together.  Then,
they can all become leaders of all the countries of the world, and we
will achieve world dominiation.

In a word, I <blink>LOVE</blink> PMD.

Based on the example rules I read about on their site, here are some
things that we could scan for(code issues):

==
String var;
// this should use UtilValidate
if (var != null && var.length > 0);
==
Collection var;
// this should use UtilValidate
if (var != null && var.isEmpty());
==
public boolean equals(Object o) {
    // This throws a ClassCastException, which is not
    // what the contract for this method allows.
    MyClass other = (MyClass) o;
==
// This loop should stay an iterator/while combo.  PMD
// can detect the remove call.  Any other loops can be
// recommended for using enhanced-for
Iterator it = collection.iterator();
while (it.hasNext()) {
    // code block
    it.remove();
    // code block
}
==
// should use valueOf
new Integer(5);
==
// should be new BigDecimal(".1")
new BigDecimal(.1);
==
// when pulling from the database, the string variant
// should be used, not the double variant.  double
// values are not exact.
double d = .1;
new BigDecimal(d);
==
// don't concat vars in append() calls
new StringBuilder().append("foo" + 1 + "bar");
==

ps: The BigDecimal checks are not something I knew about.  Those
actually already exist for PMD, and I was surprised to read about
them.
http://pmd.sourceforge.net/rules/basic.html#AvoidDecimalLiteralsInBigDecimalConstructor

pps: Upon further scanning, PMD doesn't appear to do space checks.
Bother.  So I am still looking for something to do that.
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Erwan de FERRIERES
Le 05/02/2010 17:20, Adam Heath a écrit :

> pps: Upon further scanning, PMD doesn't appear to do space checks.
> Bother.  So I am still looking for something to do that.
>
space checks can be made by checkstyle :
http://checkstyle.sourceforge.net/config_whitespace.html

--
Erwan de FERRIERES
www.nereide.biz
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Adam Heath-2
Erwan de FERRIERES wrote:
> Le 05/02/2010 17:20, Adam Heath a écrit :
>
>> pps: Upon further scanning, PMD doesn't appear to do space checks.
>> Bother.  So I am still looking for something to do that.
>>
> space checks can be made by checkstyle :
> http://checkstyle.sourceforge.net/config_whitespace.html

Sorry, no, it's LGPL.
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Ean Schuessler
Adam Heath wrote:
> Sorry, no, it's LGPL.
>  
Do the test suites have to be checked in to the repository? We do use
JIRA for bugtracking.

--
Ean Schuessler, CTO
[hidden email]
214-720-0700 x 315
Brainfood, Inc.
http://www.brainfood.com

Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Adam Heath-2
Ean Schuessler wrote:
> Adam Heath wrote:
>> Sorry, no, it's LGPL.
>>  
> Do the test suites have to be checked in to the repository? We do use
> JIRA for bugtracking.

checkstyle(which is the LGPL thing being discussed) needs to be run on
each developers workstation; that's the best place to have it
effective.  If each developer were required to download the library,
then that would be a pain in the ass.

This same reason is why I haven't pushed coverage reports(using
cobertura, which is GPL) more.  I am open to other tools, that have a
java-based api I can hook into, if others know of any.  But, before
anything gets added to the ofbiz repository, it must be an apache
compatible license.  Just saying that it's installed on some other
apache-infra type machine is not good enough, imho.

>

Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

rajsaini
There are other Apache projects using checkstyle but not sure how. See
the tiles link below.

http://tiles.apache.org/2.0/framework/tiles-jsp/checkstyle.html

For code coverage you can use Clover which is commercial but they give
the free developer license to open source project similar to the  
IntelliJ IDEA  and it is also used in other Apache projects.

http://tapestry.apache.org/tapestry4/tapestry-contrib/clover/index.html

Thanks,

Raj


Adam Heath wrote:

> Ean Schuessler wrote:
>  
>> Adam Heath wrote:
>>    
>>> Sorry, no, it's LGPL.
>>>  
>>>      
>> Do the test suites have to be checked in to the repository? We do use
>> JIRA for bugtracking.
>>    
>
> checkstyle(which is the LGPL thing being discussed) needs to be run on
> each developers workstation; that's the best place to have it
> effective.  If each developer were required to download the library,
> then that would be a pain in the ass.
>
> This same reason is why I haven't pushed coverage reports(using
> cobertura, which is GPL) more.  I am open to other tools, that have a
> java-based api I can hook into, if others know of any.  But, before
> anything gets added to the ofbiz repository, it must be an apache
> compatible license.  Just saying that it's installed on some other
> apache-infra type machine is not good enough, imho.
>
>  
>
>
>  

Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Jacopo Cappellato-4
In reply to this post by Adam Heath-2

On Feb 5, 2010, at 10:47 PM, Adam Heath wrote:

>
> checkstyle(which is the LGPL thing being discussed) needs to be run on
> each developers workstation; that's the best place to have it
> effective.  If each developer were required to download the library,
> then that would be a pain in the ass.

Could we add a pre commit hook to check the style of commits?

Jacopo

Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Jacques Le Roux-2-2
This sounds like the best proposition on this subject so far

Jacques

From: "Jacopo Cappellato" <[hidden email]>

> On Feb 5, 2010, at 10:47 PM, Adam Heath wrote:
>
>>
>> checkstyle(which is the LGPL thing being discussed) needs to be run on
>> each developers workstation; that's the best place to have it
>> effective.  If each developer were required to download the library,
>> then that would be a pain in the ass.
>
> Could we add a pre commit hook to check the style of commits?
>
> Jacopo
>
Reply | Threaded
Open this post in threaded view
|

Re: style issues(show me the code)

Ean Schuessler
In reply to this post by rajsaini
Raj Saini wrote:
> For code coverage you can use Clover which is commercial but they give
> the free developer license to open source project similar to the
> IntelliJ IDEA  and it is also used in other Apache projects.
>
> http://tapestry.apache.org/tapestry4/tapestry-contrib/clover/index.html
We've been using Cobertura internally which is Free Software (though
GPL). The main problem for OFBiz with regard to code coverage is that
none of the coverage tools will show you the utilization of simple
methods, which constitute a big chunk of OFBiz' behavior.

--
Ean Schuessler, CTO
[hidden email]
214-720-0700 x 315
Brainfood, Inc.
http://www.brainfood.com