hama-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Suraj Menon <surajsme...@apache.org>
Subject Re: Code Quality
Date Thu, 14 Jun 2012 16:17:25 GMT
We would have to do some division of labor. Most of these code-cleaning
efforts cause nightmares during merging.
How about we start by making necessary changes to the patches we upload
from now. So say if BSPMaster.java is a file in my patch, I have to clean
the code, improve documentation based on the standards set for the whole
file. This way we would have most of the commonly modified code cleaned up
asap. And then when we have a code-freeze for the release, we should be
fixing parts of code that only few would be changing. We can also divide
the work by packages. Just throwing few suggestions.

-Suraj

On Thu, Jun 14, 2012 at 11:50 AM, Thomas Jungblut <
thomas.jungblut@googlemail.com> wrote:

> 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 <surajsmenon@apache.org>
>
> > +1 for better code review and documentation standards.
> >
> > On Thu, Jun 14, 2012 at 11:30 AM, Tommaso Teofili <
> > tommaso.teofili@gmail.com
> > > wrote:
> >
> > > 2012/6/14 Thomas Jungblut <thomas.jungblut@googlemail.com>
> > >
> > > > 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 <tommaso.teofili@gmail.com>
> > > >
> > > > > 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>
> > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Thomas Jungblut
> > > > Berlin <thomas.jungblut@gmail.com>
> > > >
> > >
> >
>
>
>
> --
> Thomas Jungblut
> Berlin <thomas.jungblut@gmail.com>
>

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