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 <[email protected]> > +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]> > > > > > > -- Thomas Jungblut Berlin <[email protected]>
