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:20:37 GMT
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>
>>
>
>

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