+1 for better code review and documentation standards. On Thu, Jun 14, 2012 at 11:30 AM, Tommaso Teofili <[email protected] > wrote:
> 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]> > > >
