Checkstyle / PMD / Findbugs

[Sean Owen <sr...@gmail.com>]

As you might see, the number of such warnings has come down from thousands
to hundreds. Some I think we can just continue to fix ad-hoc (like really
long lines), or disable (like not liking variables starting with a capital
letter).

It's an interesting time to discuss the warnings that remain.

1. One common one is "overrideable method called in constructor". This is a
reasonable point.

class A {
  A() {
    stuff();
  }
  void stuff() {
    // important initialization
  }
}

class B extends A {
  void stuff() {
    // mess around and don't initialize
  }
}

B breaks A by subclassing. Making the method or class final solves it, which
is my preferred solution where I do not see any reasonable case for
subclassing. Or, the constructor can be rearranged to call this logic
directly instead of via an overrideable method.


2. Another is complaining about methods that throw 7 or more exceptions.
There are a number like this and it strikes me that it can't really be the
best thing. They could be wrapped in a generic exception, or, in cases where
the caller can't reasonably do anything, try to log and handle the condition
internally.


3. Last one I see a lot is the "really long method" warning. I agree with it
but not sure how much it's worth chopping up methods. Good to think of when
refactoring though.

Re: Checkstyle / PMD / Findbugs

[Ted Dunning <te...@gmail.com>]

A lot of these cases are in the numerical methods and should be refactorable
pretty easily since they usually look like a selection of one of several
special methods.  It would be even easier to simply delete them which is
probably the right thing to do in some cases.

On Fri, Sep 10, 2010 at 8:34 AM, Drew Farris <dr...@apache.org> wrote:

> > 3. Last one I see a lot is the "really long method" warning. I agree with
> it
> > but not sure how much it's worth chopping up methods. Good to think of
> when
> > refactoring though.
>
> I think keeping these warnings around until such methods can be
> refactored makes sense.

Re: Checkstyle / PMD / Findbugs

[Drew Farris <dr...@apache.org>]

On Fri, Sep 10, 2010 at 5:52 AM, Sean Owen <sr...@gmail.com> wrote:
> As you might see, the number of such warnings has come down from thousands
> to hundreds.

This is really great. It's pretty cool to see the big drop on the hudson page:
https://hudson.apache.org/hudson/view/Mahout/job/Mahout-Quality/

Does anyone have an idea why you can't view the warning details in
hudson? The following page gives counts but the detail tag panes
always are empty for me:
https://hudson.apache.org/hudson/view/Mahout/job/Mahout-Quality/267/pmdResult/

> 1. One common one is "overrideable method called in constructor". This is a
> reasonable point.

> B breaks A by subclassing. Making the method or class final solves it, which
> is my preferred solution where I do not see any reasonable case for
> subclassing. Or, the constructor can be rearranged to call this logic
> directly instead of via an overrideable method.

Sounds like private or final would make sense here, unless there's no
other method that needs to call the initialization method, in which
case it is likely the logic should be placed in the constructor
directly.

> 2. Another is complaining about methods that throw 7 or more exceptions.
> There are a number like this and it strikes me that it can't really be the
> best thing. They could be wrapped in a generic exception, or, in cases where
> the caller can't reasonably do anything, try to log and handle the condition
> internally.

It seems like it would be reasonable to create one or more wrapper
exceptions as the need may arise. Perhaps MahoutException is a start.
Algorithm specific exceptions like ClusteringException or
ClassifierException could inherit from MahoutException and would read
better, but I wonder whether they are overkill.

> 3. Last one I see a lot is the "really long method" warning. I agree with it
> but not sure how much it's worth chopping up methods. Good to think of when
> refactoring though.

I think keeping these warnings around until such methods can be
refactored makes sense.

Drew

Re: Checkstyle / PMD / Findbugs

[Isabel Drost <is...@apache.org>]

On Fri, 10 Sep 2010 Sean Owen <sr...@gmail.com> wrote:
> As you might see, the number of such warnings has come down from
> thousands to hundreds.

Yeah!


> 1. One common one is "overrideable method called in constructor".
> This is a reasonable point.
> B breaks A by subclassing. Making the method or class final solves
> it, which is my preferred solution where I do not see any reasonable
> case for subclassing.

I'd prefer marking the method itself as either final or private - still
leaves more freedom to the user who may see reasonable cases for
subclassing that the initial developer did not think of.


> 2. Another is complaining about methods that throw 7 or more
> exceptions. There are a number like this and it strikes me that it
> can't really be the best thing. They could be wrapped in a generic
> exception, or, in cases where the caller can't reasonably do
> anything, try to log and handle the condition internally.
 
Writing such a method, me personally I'd prefer throwing some generic
exception* over silently catching the exception - at least in cases
where the error condition cannot reasonably be fixed in the method it
occurred in.


* It should retain enough information to allow for logfile analysis.

 
> 3. Last one I see a lot is the "really long method" warning. I agree
> with it but not sure how much it's worth chopping up methods. Good to
> think of when refactoring though.

+1

Isabel