#328 ThrowsCount does not account for exception hierarchy

Future
closed
nobody
5
2016-02-28
2004-10-23
No

ThrowsCount simply counts the number of declared
exceptions of a method, and adds an audit entry if this
number exceeds the configured maximum.

However, the rationale is to encourage a coding style
where exceptions are part of a class hierarchy.

ThrowsCount does not consider exception hierarchies.

Instead of counting the number of declared exceptions,
it should count the number of common ancestor classes
of the declared exceptions that are direct children of
java.lang.Exception and only create an audit entry if the
number of such ancestors exceed the configured
maximum.

I attached an example that demonstrates the problem.
Let's configure the check like so:

<module name="ThrowsCount">
<property name="max" value="2"/>
</module>

Now consider this method:

public void testThrowsCount() throws ExceptionX,
ExceptionY, InterruptedException
{...}

It declares to throw three exceptions. However,
ExceptionX and ExceptionY have the common parent
class AbstractException. Thus there are only two
common ancestors that are directly derived from
java.lang.Exception - specifically
java.lang.InterruptedException and
test.throwscount.AbstractException. No audit entry
should be created.

The way ThrowsCount is implemented right now, the
method would need to declare to throw
AbstractException which then would have to be caught
explicitly. That is precisely what we do not want.

Discussion

  • Oleg Sukhodolsky

    Logged In: YES
    user_id=746148

    The check behaves exactly as it was designing, thus this is a
    new RFE, but not a bug.

    Also, I'd say that requested behavior is not perfect too:
    e.g. if I have complicated hierarhy with one common ancestor
    (AbstractException) which extends Exception then does that
    mean that all exceptions listend in throws clause should be
    treated as one? Also, if my methods throws several IO
    excpetions does that means that this is just one
    (IOException) exception?
    I think that the check should enforce throwing fewer
    exception because the code which has to catch all such
    exceptions looks ugly, but if we will allow suggested behavior
    user still will has to write such code, or catch common
    ancestor of the declared exception (this approach looks like
    catching Exception or Throwable) which is not a good idea.

     
  • Jürgen Failenschmid

    Logged In: YES
    user_id=358315

    I don't understand o_sukhodolsky claim that ThrowsCount
    performs as per design. Here's a quote from the
    documentation:

    "Rationale: Exceptions form part of a methods interface.
    Declaring a method to throw too many differently rooted
    exceptions makes exception handling onerous and leads to
    poor programming practices such as catch (Exception). This
    check forces developers to put exceptions into a heirachy
    [sic] such that in the simplest case, only one type of
    exception need be checked for by a caller but allows any sub-
    classes to be caught specifically if necessary. "

    Note the word "rooted". It is sometimes necessary to have
    many detailed exceptions raised by a method. The total
    number of detailed exceptions is not the issue. A complexity
    issue arises only if those exceptions are rooted differently
    with two many ancestors. In other words, the rationale is to
    minimize the number of exception hierarchies, at the same
    time providing senders the choice of being able to treat each
    subclassed exception separately, if that is required. In the
    example, the method raises java.lang.InterruptedException,
    which cannot be rooted at the discretion of the programmer.

    So, how should the method in the example be declared
    without causing an audit entry from ThrowsCount based on
    the check's rationale?

     
  • Oleg Sukhodolsky

    Logged In: YES
    user_id=746148

    I agree that "rooted" may be interpreted as "with different
    ancestors which extends Exception", but the check currently
    just skipped this :( and thus it behaves as it was desined by
    its author)

    In my comment I just tried to say that "rooted" may also be
    interpreted differently (e.g. ancestors which extends some
    other class (not an Exception)). So, we should consider this
    when we start to implement this feature.

     
  • Roman Ivanov

    Roman Ivanov - 2016-02-28
    • status: open --> closed
    • Group: --> Future
     
  • Roman Ivanov

    Roman Ivanov - 2016-02-28

    Checkstyle is not aware of class hierarchy at all.
    we can not do this

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks