2012/6/14 Thomas Jungblut <[email protected]> > 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 <[email protected]> > > > 2012/6/14 Tommaso Teofili <[email protected]> > > > > > 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 <[email protected]> > > > > > >> 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 <[email protected]> > > >> > > > > > > > > > > > > -- > Thomas Jungblut > Berlin <[email protected]> >
