hama-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Jungblut <thomas.jungb...@googlemail.com>
Subject Re: Code Quality
Date Thu, 14 Jun 2012 16:28:17 GMT
Of course we have to distinct and prioritize the work. It's not about a
"stop the world" barrier.
We can first of all solve the critical bugs in our code (not closed streams
for example).
Some parts of the system will never be touched again, but may contain
really bad style. Like the task handling for example.

Most of these code-cleaning efforts cause nightmares during merging.


I'm so sorry :/

2012/6/14 Suraj Menon <surajsmenon@apache.org>

> 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>
> >
>



-- 
Thomas Jungblut
Berlin <thomas.jungblut@gmail.com>

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