Throwable.initCause helper method

[Adam Heath-2]

I'm thinking about the following proposed method in UtilMisc.
Included in the diff is the new method, and one example usage.  If it
is used more often, the size of the code will become smaller.

However, I'm not entirely certain if it would be useful, and it's such
a very small helper method, I'm wondering whether the utility is
actually worth it.  What do you guys think?


=== framework/base/src/org/ofbiz/base/util/UtilMisc.java
==================================================================
--- framework/base/src/org/ofbiz/base/util/UtilMisc.java (revision 8717)
+++ framework/base/src/org/ofbiz/base/util/UtilMisc.java (local)
@@ -46,6 +46,11 @@
 
     public static final BigDecimal ZERO_BD = BigDecimal.ZERO;
 
+    public static final <T extends Throwable> T initCause(T throwable, Throwable cause) {
+        throwable.initCause(cause);
+        return throwable;
+    }
+
     /**
      * Get an iterator from a collection, returning null if collection is null
      * @param col The collection to be turned in to an iterator
=== framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java
==================================================================
--- framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java (revision 8717)
+++ framework/base/src/org/ofbiz/base/util/template/FreeMarkerWorker.java (local)
@@ -97,7 +97,7 @@
             resources = loader.getResources("freemarkerTransforms.properties");
         } catch (IOException e) {
             Debug.logError(e, "Could not load list of freemarkerTransforms.properties", module);
-            throw (InternalError) new InternalError(e.getMessage()).initCause(e);
+            throw UtilMisc.initCause(new InternalError(e.getMessage()), e);
         }
         while (resources.hasMoreElements()) {
             URL propertyURL = resources.nextElement();

[Adrian Crum]

I remember you complaining a while back about developers who wrap
exceptions - because it makes code harder to debug. (I agree with that
view, btw.) Has that view changed?

Why not add a throws clause to the enclosing FreemarkerWorker method?

-Adrian

Adam Heath wrote:
> I'm thinking about the following proposed method in UtilMisc.
> Included in the diff is the new method, and one example usage.  If it
> is used more often, the size of the code will become smaller.
>
> However, I'm not entirely certain if it would be useful, and it's such
> a very small helper method, I'm wondering whether the utility is
> actually worth it.  What do you guys think?
>
>

[Adam Heath-2]

Adrian Crum wrote:
> I remember you complaining a while back about developers who wrap
> exceptions - because it makes code harder to debug. (I agree with that
> view, btw.) Has that view changed?

try {
} catch (FooException e) {
    e.printStackTrace(); // Or Debug.logError(e)
    throw new BarException(e.getMessage());
}

That's what I complained about.  Utility code should *never* print(or
log) an exception.  It should always throw it to the calling code, and
let it decide.  Of course, then you have to figure out how much is
actually utility, and how much is higher-level.

> Why not add a throws clause to the enclosing FreemarkerWorker method?

Because that exposes the inner workings of the method in question.

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> I remember you complaining a while back about developers who wrap
>> exceptions - because it makes code harder to debug. (I agree with that
>> view, btw.) Has that view changed?
>
> try {
> } catch (FooException e) {
>     e.printStackTrace(); // Or Debug.logError(e)
>     throw new BarException(e.getMessage());
> }
>
> That's what I complained about.  Utility code should *never* print(or
> log) an exception.  It should always throw it to the calling code, and
> let it decide.  Of course, then you have to figure out how much is
> actually utility, and how much is higher-level.

Oh. Thanks for the clarification.

>> Why not add a throws clause to the enclosing FreemarkerWorker method?
>
> Because that exposes the inner workings of the method in question.

Huh?

Using your example above, the calling code could make a decision based
on the exception thrown. For example, if FooException is thrown, try an
alternate method. But that decision can't be made, because FooException
is never thrown.

This is especially bad in some OFBiz classes where EVERY class method
throws the same home-grown exception. There is no way for calling code
to know what to do when an error is encountered.

-Adrian

[Jacques Le Roux]

From: "Adrian Crum" <[hidden email]>
To: <[hidden email]>
Sent: Thursday, May 28, 2009 8:07 PM
Subject: Re: Throwable.initCause helper method

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> I remember you complaining a while back about developers who wrap
>>> exceptions - because it makes code harder to debug. (I agree with that
>>> view, btw.) Has that view changed?
>>
>> try {
>> } catch (FooException e) {
>>     e.printStackTrace(); // Or Debug.logError(e)
>>     throw new BarException(e.getMessage());
>> }
>>
>> That's what I complained about.  Utility code should *never* print(or
>> log) an exception.  It should always throw it to the calling code, and
>> let it decide.  Of course, then you have to figure out how much is
>> actually utility, and how much is higher-level.
>
> Oh. Thanks for the clarification.
>
>>> Why not add a throws clause to the enclosing FreemarkerWorker method?
>>
>> Because that exposes the inner workings of the method in question.
>
> Huh?
>
> Using your example above, the calling code could make a decision based on the exception thrown. For example, if FooException is
> thrown, try an alternate method. But that decision can't be made, because FooException is never thrown.

I agree with Adrian. This is the standard way of using exceptions in Java. Of course there is still this frontier between utilities
and "main-stream code" and some part of the code have to tell what is really happening... This report also depends of who or what is
using the utility eventually. Are we missing something very special here (inner workings?) ?

Jacques

> This is especially bad in some OFBiz classes where EVERY class method throws the same home-grown exception. There is no way for
> calling code to know what to do when an error is encountered.
>
> -Adrian
>

[David E Jones-3]

On May 28, 2009, at 11:26 AM, Adam Heath wrote:

> Adrian Crum wrote:
>> I remember you complaining a while back about developers who wrap
>> exceptions - because it makes code harder to debug. (I agree with  
>> that
>> view, btw.) Has that view changed?
>
> try {
> } catch (FooException e) {
>    e.printStackTrace(); // Or Debug.logError(e)
>    throw new BarException(e.getMessage());
> }

It's usually good to pass it up the stack, but if the  
Debug.logError(e) was removed (BTW, we should never have any  
e.printStackTrace() calls since they don't go through Log4J) then the  
original location of the error would be lost.

In other words, there should ALWAYS be a nested exception inside the  
new exception thrown. If that isn't possible then it MUST be logged,  
otherwise it is impossible to pin down the real source of the error  
(and yes, that has happened various times in the history of OFBiz...).

-David


> That's what I complained about.  Utility code should *never* print(or
> log) an exception.  It should always throw it to the calling code, and
> let it decide.  Of course, then you have to figure out how much is
> actually utility, and how much is higher-level.
>
>> Why not add a throws clause to the enclosing FreemarkerWorker method?
>
> Because that exposes the inner workings of the method in question.
>

[Adam Heath-2]

Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> I remember you complaining a while back about developers who wrap
>>> exceptions - because it makes code harder to debug. (I agree with that
>>> view, btw.) Has that view changed?
>>
>> try {
>> } catch (FooException e) {
>>     e.printStackTrace(); // Or Debug.logError(e)
>>     throw new BarException(e.getMessage());
>> }
>>
>> That's what I complained about.  Utility code should *never* print(or
>> log) an exception.  It should always throw it to the calling code, and
>> let it decide.  Of course, then you have to figure out how much is
>> actually utility, and how much is higher-level.
>
> Oh. Thanks for the clarification.
>
>>> Why not add a throws clause to the enclosing FreemarkerWorker method?
>>
>> Because that exposes the inner workings of the method in question.
>
> Huh?
>
> Using your example above, the calling code could make a decision based
> on the exception thrown. For example, if FooException is thrown, try an
> alternate method. But that decision can't be made, because FooException
> is never thrown.
>
> This is especially bad in some OFBiz classes where EVERY class method
> throws the same home-grown exception. There is no way for calling code
> to know what to do when an error is encountered.

Let's say that you have an existing method, that currently throws
GeneralException.  Then, later on, you add a feature to let it cache
it's results to disk.  This means you end up using java.io.* classes,
which suddenly start throwing IOException.  Now, should you change the
abi, adding a throws IOException to the method, or should you do an
initCause thing?  Remember, if you add a new throws, it will break all
source that is compiled against the new version.

If you go the new throws route, then all other methods on the stack
need to be alter as well, each in turn adding a new throws clause.
That very quickly becomes a maintenance nightmare.

However, let's say you decide it is worth it, and change all the
methods in the stack to have new catch blocks, or new throws.  Time
goes by, and you change this method to now try to do a network
connection to some other cache server, to try and load data.  Now it
has to throw a SocketException.  So you go thru the same round of code
changes.

If, instead, you had chosen to have a single per-api exception as the
only throws, then you wouldn't have to modify code all the time,
whenever you add new features.  Most call points wouldn't care about
why something failed(they wouldn't look at the initCause), they'd just
pass the exception up the stack.

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> I remember you complaining a while back about developers who wrap
>>>> exceptions - because it makes code harder to debug. (I agree with that
>>>> view, btw.) Has that view changed?
>>> try {
>>> } catch (FooException e) {
>>>     e.printStackTrace(); // Or Debug.logError(e)
>>>     throw new BarException(e.getMessage());
>>> }
>>>
>>> That's what I complained about.  Utility code should *never* print(or
>>> log) an exception.  It should always throw it to the calling code, and
>>> let it decide.  Of course, then you have to figure out how much is
>>> actually utility, and how much is higher-level.
>> Oh. Thanks for the clarification.
>>
>>>> Why not add a throws clause to the enclosing FreemarkerWorker method?
>>> Because that exposes the inner workings of the method in question.
>> Huh?
>>
>> Using your example above, the calling code could make a decision based
>> on the exception thrown. For example, if FooException is thrown, try an
>> alternate method. But that decision can't be made, because FooException
>> is never thrown.
>>
>> This is especially bad in some OFBiz classes where EVERY class method
>> throws the same home-grown exception. There is no way for calling code
>> to know what to do when an error is encountered.
>
> Let's say that you have an existing method, that currently throws
> GeneralException.  Then, later on, you add a feature to let it cache
> it's results to disk.  This means you end up using java.io.* classes,
> which suddenly start throwing IOException.  Now, should you change the
> abi, adding a throws IOException to the method, or should you do an
> initCause thing?  Remember, if you add a new throws, it will break all
> source that is compiled against the new version.
>
> If you go the new throws route, then all other methods on the stack
> need to be alter as well, each in turn adding a new throws clause.
> That very quickly becomes a maintenance nightmare.
>
> However, let's say you decide it is worth it, and change all the
> methods in the stack to have new catch blocks, or new throws.  Time
> goes by, and you change this method to now try to do a network
> connection to some other cache server, to try and load data.  Now it
> has to throw a SocketException.  So you go thru the same round of code
> changes.
>
> If, instead, you had chosen to have a single per-api exception as the
> only throws, then you wouldn't have to modify code all the time,
> whenever you add new features.  Most call points wouldn't care about
> why something failed(they wouldn't look at the initCause), they'd just
> pass the exception up the stack.

That's a good example. It seems we made a case for the two approaches.

-Adrian

[Jacques Le Roux]

Thanks for your detailed explanation Adam,

Jacques

From: "Adam Heath" <[hidden email]>

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> I remember you complaining a while back about developers who wrap
>>>> exceptions - because it makes code harder to debug. (I agree with that
>>>> view, btw.) Has that view changed?
>>>
>>> try {
>>> } catch (FooException e) {
>>>     e.printStackTrace(); // Or Debug.logError(e)
>>>     throw new BarException(e.getMessage());
>>> }
>>>
>>> That's what I complained about.  Utility code should *never* print(or
>>> log) an exception.  It should always throw it to the calling code, and
>>> let it decide.  Of course, then you have to figure out how much is
>>> actually utility, and how much is higher-level.
>>
>> Oh. Thanks for the clarification.
>>
>>>> Why not add a throws clause to the enclosing FreemarkerWorker method?
>>>
>>> Because that exposes the inner workings of the method in question.
>>
>> Huh?
>>
>> Using your example above, the calling code could make a decision based
>> on the exception thrown. For example, if FooException is thrown, try an
>> alternate method. But that decision can't be made, because FooException
>> is never thrown.
>>
>> This is especially bad in some OFBiz classes where EVERY class method
>> throws the same home-grown exception. There is no way for calling code
>> to know what to do when an error is encountered.
>
> Let's say that you have an existing method, that currently throws
> GeneralException.  Then, later on, you add a feature to let it cache
> it's results to disk.  This means you end up using java.io.* classes,
> which suddenly start throwing IOException.  Now, should you change the
> abi, adding a throws IOException to the method, or should you do an
> initCause thing?  Remember, if you add a new throws, it will break all
> source that is compiled against the new version.
>
> If you go the new throws route, then all other methods on the stack
> need to be alter as well, each in turn adding a new throws clause.
> That very quickly becomes a maintenance nightmare.
>
> However, let's say you decide it is worth it, and change all the
> methods in the stack to have new catch blocks, or new throws.  Time
> goes by, and you change this method to now try to do a network
> connection to some other cache server, to try and load data.  Now it
> has to throw a SocketException.  So you go thru the same round of code
> changes.
>
> If, instead, you had chosen to have a single per-api exception as the
> only throws, then you wouldn't have to modify code all the time,
> whenever you add new features.  Most call points wouldn't care about
> why something failed(they wouldn't look at the initCause), they'd just
> pass the exception up the stack.
>