hama-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tommaso Teofili <tommaso.teof...@gmail.com>
Subject Re: Code Quality
Date Thu, 14 Jun 2012 15:14:53 GMT
Hi Thomas,

big +1 to your comments, I also encountered some of the things you
mentioned, that's why I opened .
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>
>

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