hama-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Jungblut <thomas.jungb...@googlemail.com>
Subject Re: Code Quality
Date Thu, 14 Jun 2012 15:50:38 GMT
Sonar looks good, although I think we are going to have a hard time
face-lifting all the modules it is really needed.
We can not afford too many technical dept in our future, we have the time
to make things right and correct.

2012/6/14 Suraj Menon <surajsmenon@apache.org>

> +1 for better code review and documentation standards.
>
> On Thu, Jun 14, 2012 at 11:30 AM, Tommaso Teofili <
> tommaso.teofili@gmail.com
> > wrote:
>
> > 2012/6/14 Thomas Jungblut <thomas.jungblut@googlemail.com>
> >
> > > Yea, it would be great if we can find some automatism, maybe in maven
> > like
> > > RAT that is failing the build if too many warnings or errors have been
> > > added. Does Sonar provide something equal or is is something plugged
> into
> > > jenkins (would be some improvement anyways)?
> > >
> >
> > you can see an example of Apache Nutch code analysis here for "critical"
> > bugs :
> > https://analysis.apache.org/drilldown/violations/81593?severity=CRITICAL
> > We may want to request this facility too.
> >
> >
> >
> > > Documentation of generics or general API is very sparse, you're right.
> We
> > > have to improve on this one as well.
> > >
> > > +1 for the code review.
> > >
> >
> > Great!
> > Tommaso
> >
> >
> > >
> > > 2012/6/14 Tommaso Teofili <tommaso.teofili@gmail.com>
> > >
> > > > 2012/6/14 Tommaso Teofili <tommaso.teofili@gmail.com>
> > > >
> > > > > Hi Thomas,
> > > > >
> > > > > big +1 to your comments, I also encountered some of the things you
> > > > > mentioned, that's why I opened .
> > > > >
> > > >
> > > > missed to paste the link to
> > > https://issues.apache.org/jira/browse/HAMA-572
> > > >
> > > >
> > > > > I think that we should now release 0.5.0 and then plan for an
> > > appropriate
> > > > > API / code review.
> > > > > Just for the sake of an example, the BSP interface (a very
> important
> > > one)
> > > > > has generics which don't have neither binding to interfaces nor
> > > > > documentation to explain what K, V, etc. mean.
> > > > >
> > > > > We may want to use the ASF Sonar instance to enable a semi
> automatic
> > > way
> > > > > of checking for this kind of bugs.
> > > > > My 2 cents,
> > > > > Tommaso
> > > > >
> > > > > 2012/6/14 Thomas Jungblut <thomas.jungblut@googlemail.com>
> > > > >
> > > > >> Hi all,
> > > > >>
> > > > >> I'm currently going through all packages and cleaning up code
by
> > > eclipse
> > > > >> warnings. There I found some huge bugs, for example:
> > > > >>
> > > > >> public final void add(List<Metric<? extends Number>>
metrics){
> > > > >> >   metrics.addAll(metrics);
> > > > >> > }
> > > > >>
> > > > >>
> > > > >> Which is basically not intended, but wanted to reference a field
> > name
> > > > >> "metrics". This part is just adding the same collection to itself.
> > > > >> Also I have seen many not using override annotations or using
> > > generics.
> > > > >> There are also many open resources that are not properly closed
> with
> > > > >> finally, thus possible descriptor leaks.
> > > > >> There are several places, also I did some faults because several
> > > > warnings
> > > > >> are not turned on in the latest eclipse version.
> > > > >>
> > > > >> I would advise you (at least the comitters) to turn on the code
> > > > >> inspections
> > > > >> in your IDE.
> > > > >> I have turned on several things in eclipse: (can be found in
the
> > > project
> > > > >> preferences or global preferences under
> > > Java/Compiler/"Errors/Warnings")
> > > > >>
> > > > >> From top to bottom:
> > > > >>
> > > > >> Non-static access to static member (WARNING)
> > > > >> Indirect access to static member (WARNING)
> > > > >> Method with a constructor name (WARNING)
> > > > >> Parameter assignment (WARNING)
> > > > >> Method can be static (WARNING)
> > > > >>
> > > > >> Serializable class without serialID (WARNING)
> > > > >> Assignment has no effect (WARNING)
> > > > >> finally does not complete normally (WARNING)
> > > > >> Using a char array in string concat (WARNING)
> > > > >> hidden catch block (WARNING)
> > > > >> Inexact type match for varags argument (WARNING)
> > > > >> Nullpointer access (WARNING)
> > > > >> Compare identical values (WARNING)
> > > > >> class overrides equals but not hashcode (WARNING)
> > > > >> dead code (WARNING)
> > > > >>
> > > > >> Type parameter hides another type (WARNING)
> > > > >> Method does not override package visible method (WARNING)
> > > > >> interface method conflicts with protected object method (WARNING)
> > > > >>
> > > > >> Deprecated API (WARNING) // can be neglected by using annotations
> if
> > > not
> > > > >> other possible though
> > > > >> Forbidden references (ERROR)
> > > > >> Discouraged references (WARNING)
> > > > >>
> > > > >> Value of local variable is not used (WARNING) // please delete
it
> if
> > > > never
> > > > >> read
> > > > >> Unused import (WARNING) // please format and organize imports
> before
> > > > >> making
> > > > >> a patch (in eclipse CTRL+SHIFT+F and CTRL+O)
> > > > >> Unused private member (WARNING) // remove if never used
> > > > >> Unnecessary cast or instanceof operation (WARNING)
> > > > >> Unused break or continue label (WARNING)
> > > > >>
> > > > >> Unchecked generic type operation (WARNING)
> > > > >> Usage of raw types (WARNING) // sometimes can not be avoided,
can
> be
> > > > >> neglected via annotation
> > > > >> Generic type parameter declared with a final type bound (WARNING)
> > > > >>
> > > > >> Missing override annotation (WARNING)
> > > > >> Annotation is used as super interface (WARNING)
> > > > >> Unhandled token in SuppressWarnings (WARNING)
> > > > >>
> > > > >> I will put this up in our wiki and I want that everybody who
> > commits a
> > > > >> patch is checking against these guidelines, I don't want to add
> > > Findbugs
> > > > >> because the false positive rate is too high.
> > > > >> Would be great if everybody will care about that, because static
> > code
> > > > >> analysis is very great and if it's offered we should use it.
> > > > >>
> > > > >> --
> > > > >> Thomas Jungblut
> > > > >> Berlin <thomas.jungblut@gmail.com>
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Thomas Jungblut
> > > Berlin <thomas.jungblut@gmail.com>
> > >
> >
>



-- 
Thomas Jungblut
Berlin <thomas.jungblut@gmail.com>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message